-
Notifications
You must be signed in to change notification settings - Fork 132
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
Add haloupdate unit test #820
Conversation
- Add halochk unit test - Add "unknown" and "noupdate" checks to ice_boundary - Remove field_loc_Wface, not used anywhere, not supported - Update cice_decomp.csh script To Do: validate tripole and tripoleT, add unit test to test suite
- Reduce redundant tripole buffer copies in serial/ice_boundary.F90 - Generalize iSrc wraparound calculation in ice_boundary.F90 - Add open, cyclic, tripole, and tripoleT set_nml files - Update unittest suite
- Add tripoleT support to haloUpdate_stress - Add abort check that nx_global is even for tripole grids - Update 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 fonts for nx_global in line 278 (which you did not change) of ug_implementation.rst are inconsistent. Other than that, this all looks good to me. The new documentation is very helpful - thanks for figuring this out, fixing the problems, and adding a unit test.
Good catch. I think I made the formatting in the tripole section more consistent now. Happy to make other changes if something else is preferred. |
@@ -1552,7 +1608,7 @@ subroutine ice_HaloUpdate2DR8(array, halo, & | |||
!*** correct for offsets | |||
iSrc = iSrc - ioffset | |||
jSrc = jSrc - joffset | |||
if (iSrc == 0) iSrc = nxGlobal | |||
if (iSrc < 1 ) iSrc = iSrc + nxGlobal |
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 didn't realize this could go negative.
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.
It probably doesn't at the moment, but the new implementation is more general in case greater padding or other tripole (or similar) setups with larger offsets cause it to go negative at some point. I figured it didn't hurt to make the change while I was there and noticed it.
PR checklist
Short (1 sentence) summary of your PR:
Add haloupdate unit test
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.
Full test suite bit-for-bit on cheyenne, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#86f54d1539c801e8bf7f320bceccfbe14521df82
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:
Add haloUpdate unit test
Add logic for field_loc and field_type = noupdate and unknown in haloUpdate code
Delete field_loc_Wface parameter, never used and not supported
Generalize iSrc wraparound computation in haloUpdate
Add tripoleT support to haloUpdate_stress
Fix bug in tripole implementation and remove redundant tripole copies in serial version of ice_boundary.F90
Add check that nx_global must be even for tripole grids
Update cice decomp tool
Update unittest_suite, add halochk unit tests
Update documentation