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.

Sign up for GitHub

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

Jump to bottom

Fix box2001 forcing, restart fields on land, bathymetry default value, omp_suite #65

Merged
merged 6 commits into from
Mar 10, 2022
Merged

Fix box2001 forcing, restart fields on land, bathymetry default value, omp_suite #65

merged 6 commits into from
Mar 10, 2022

Conversation

apcraig
Copy link
Owner

@apcraig apcraig commented Mar 8, 2022

PR checklist

  • Short (1 sentence) summary of your PR:
    Fix box2001 forcing, restart fields on land, bathymetry default value, omp_suite
  • 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 suites run on cheyenne, results as expected
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit except box2001
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on Icepack or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes, modify omp_suite to better cover different decompositions
    • 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:

These changes should be migrated to main sooner than later.

  • Update box2001 so it's bit-for-bit with different blocks/decomps/pe counts.
  • Initialize bathymetry values at all gridcells when bathymetry_format='default' and use_bathymetry=.false.
  • Update comparelog to exclude extraneous icepack output
  • Zero out certain non-zero fields when writing restart files to deal with land-block-elimination and restart file comparisons
  • Update omp_suite to test different decomps when testing different threads

@apcraig
Fix several problems noted in PR #64. This should
15717e7 
address the problems with the boxrestore test errors.
- Update box2001 so it's bit-for-bit with different blocks/decomps/pe counts.
- Initialize bathymetry values at all gridcells when bathymetry_format='default' and use_bathymetry=.false.
@apcraig
update comparelog to exclude extraneous icepack output, help prod com…
ef94181 
…parisons
@apcraig
- Zero out certain fields on land on restart files. Some fields have …
cfb505b 
…non-zero values

  over land by default and this causes problems with land block elimination and
  comparisons of different decompositions.  Does not affect science.
- Update omp_suite to use different block sizes in comparisons
eclare108213
eclare108213 approved these changes Mar 9, 2022
View reviewed changes
cicecore/cicedynB/general/ice_forcing.F90
uocn(i,j,iblk) = p2*real(jglob(j), kind=dbl_kind) &
/ real(ny_global,kind=dbl_kind) - p1
vocn(i,j,iblk) = -p2*real(iglob(i), kind=dbl_kind) &
/ real(nx_global,kind=dbl_kind) + p1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is BFB on square domains -- but definitely fixing a bug in the denominators, thanks for catching it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also noticed the bug in the forcing, not just when I was reviewing the paper. So it wasn't working on multiple blocks and the forcing was incorrect.

cicecore/cicedynB/infrastructure/ice_grid.F90 Outdated
@@ -4228,6 +4228,7 @@ subroutine get_bathymetry
depth(k) = depth(k-1) + thick(k)
enddo

bathymetry = 0._dbl_kind
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use c0 here?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brain freeze, I'll fix it.

@apcraig
Copy link
Owner Author

apcraig commented Mar 10, 2022

Full suite results on cheyenne, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#090aa9280b9c91d648f07da5802307724983a13c

All tests pass. The following difference exist with current main

  • Cases with snw, zsal, bgc have masked values on land now, does not change answers.
  • dynpicard is not bit-for-bit, capping was changed to 1.0
  • box2001 is not bit-for-bit, fixed forcing
  • many box tests are not bit-for-bit, add *aice to forcing
  • several pgi tests fail due to sensitive compiler optimization
  • gx1prod is not bit-for-bit, not sure why. gx1 and QC tests pass otherwise.

The following are known problems

@apcraig apcraig mentioned this pull request Mar 10, 2022
Closed
@apcraig apcraig merged commit 21768d8 into cgridDEV Mar 10, 2022
@apcraig apcraig deleted the cgtestH branch August 17, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eclare108213 eclare108213 eclare108213 approved these changes

@dabail10 dabail10 dabail10 approved these changes

@JFLemieux73 JFLemieux73 Awaiting requested review from JFLemieux73

@phil-blain phil-blain Awaiting requested review from phil-blain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apcraig @eclare108213 @dabail10