Skip to content
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

Merged
merged 13 commits into from
Mar 13, 2023
Merged

Add haloupdate unit test #820

merged 13 commits into from
Mar 13, 2023

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Mar 10, 2023

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?

    • bit for bit. Fixes a bug in serial tripole implementation that was never triggered in testing
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on Icepack or any other models?

    • Yes
    • No
  • Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.

    • Yes
    • No
  • Does this PR add any new test cases?

    • Yes
    • No
  • 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.)

    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • 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

- 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
Copy link
Contributor

@eclare108213 eclare108213 left a 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.

@apcraig
Copy link
Contributor Author

apcraig commented Mar 12, 2023

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@apcraig apcraig merged commit 75b792c into CICE-Consortium:main Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants