-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename cicedynB to cicedyn, update test suites #790
Conversation
smoke gx3 1x1x100x116x1 reprosum,run10day | ||
smoke gx1 32x1x16x16x32 reprosum,run10day | ||
smoke gx3 1x1x100x116x1 reprosum,run10day,gridcd | ||
smoke gx1 32x1x16x16x32 reprosum,run10day,gridcd | ||
smoke gx3 1x1x100x116x1 reprosum,run10day,gridc | ||
smoke gx1 32x1x16x16x32 reprosum,run10day,gridc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these seems like the same tests added in first_suite
, is that what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is confusing. What is the purpose of first_suite? I thought it was intended to be a simplified base_suite for quicker turnaround during development.
@@ -0,0 +1 @@ | |||
cicedyn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with adding a symlink for now, but maybe we could add a note in the release notes that this is temporary and will be removed in a future version ?
smoke gx3 1x1x100x116x1 reprosum,run10day | ||
smoke gx1 32x1x16x16x32 reprosum,run10day | ||
smoke gx3 1x1x100x116x1 reprosum,run10day,gridcd | ||
smoke gx1 32x1x16x16x32 reprosum,run10day,gridcd | ||
smoke gx3 1x1x100x116x1 reprosum,run10day,gridc | ||
smoke gx1 32x1x16x16x32 reprosum,run10day,gridc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is confusing. What is the purpose of first_suite? I thought it was intended to be a simplified base_suite for quicker turnaround during development.
set fcnt = `sed -n '/\t/p' $file | wc -l` | ||
@ cnt = $cnt + $fcnt | ||
if ($fcnt > 0) then | ||
echo "TAB found: $fcnt $file" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about what this is doing. It only affects testing with github actions? If there are tabs in the source code, then github actions fails? And this just flags where they are so they can be fixed? (i.e. it doesn't fix them)
@eclare108213, that is correct regarding the tab checking. I'm tired of finding and fixing tabs. It affects the indentation and I don't think we want them in general. I figure if we can trap them early, it's a win. Regarding first_suite. first_suite is a list of tests that appear in other suites where results are needed for bfbcomp. The idea is that when running a suite of tests, running first_suite first can speed up the results by producing the baselines first. It also reduces the chance that bfbcomp are going to fail because the baseline is not there. The changes to the test suites are relatively minimal even though it looks like a lot
One other thing is fixed in this PR. The suites were NOT running in the order the user specified prior to now (to my surprise). This is now fixed and should help testing overall. Also, just an FYI. If the same test appears in multiple suites, that's OK, it will only be created once and run once. quick_suite is the short test suite. There are several test suites now, maybe we need to document their purpose a little better. I will add some details to the documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional documentation is very helpful. Thanks!
All cheyenne intel tests pass. https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#a28d12c14b9e8231f2edee093c009c215cda908d Other compilers also seem to pass fine. Will set this PR as ready. |
Will resolve the conflicts after merging #778. Will probably have some new conflicts there too due to renaming of cicedynB directory. |
- Rename cicedynB directory to cicedyn (See CICE-Consortium#660) - Add softlink for cicedynB - Update path in cice.build - Update documentation - Fix sig1, sig2, sigP grid on history file (See CICE-Consortium#789) - Fix Fortran warning messages for long lines - Fix test suite order in cice.setup - Update test suites to reduce bfbcomp failures due to time outs - Add tests to first_suite to help - Add dyneap and dynpicard decomp tests, add set_nml.dyneap - Add TAB check in github actions in all .F90 and .c files github actions will fail if source files have TABs
I rebased this PR and will run another test suite. |
Testing looks good, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#c176d3aa6d966569cf0d31964d6d20e5e1444dbf. I think we can merge this PR. I'll merge tomorrow unless I hear any concerns. |
For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers
PR checklist
Short (1 sentence) summary of your PR:
Rename cicedynB to cicedyn, update test suites, fix sig1, sig2, sigP grid on history file for C-grid
Developer(s):
apcraig
Suggest PR reviewers from list in the column to the right.
Please copy the PR test results link or provide a summary of testing completed below.
All tests bit-for-bit, new decomp tests with eap and vp pass, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#a28d12c14b9e8231f2edee093c009c215cda908d
Last results, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#c176d3aa6d966569cf0d31964d6d20e5e1444dbf
How much do the PR code changes differ from the unmodified code?
Does this PR create or have dependencies on Icepack or any other models?
Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
Does this PR add any new test cases?
Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
Please provide any additional information or relevant details below:
Rename cicedynB directory to cicedyn (See C-grid Code Cleanup #660)
Fix sig1, sig2, sigP grid on history file (See netcdf metadata is not right for some C grid output variables #789)
Fix Fortran warning messages for long lines
Fix test suite order in cice.setup
Update test suites to reduce bfbcomp failures due to time outs
Add dyneap and dynpicard decomp tests, add set_nml.dyneap
Add TAB check in github actions in all .F90 and .c files github actions will fail if source files have TABs
Closes #789