Skip to content

Fix issue in PR#1294 to correctly account for IC_NUMERICS option + regression test for this new ice numerics feature#1413

Merged
JessicaMeixner-NOAA merged 3 commits into
NOAA-EMC:developfrom
sbanihash:feature/add_reg_plus_fixes
Apr 29, 2025
Merged

Fix issue in PR#1294 to correctly account for IC_NUMERICS option + regression test for this new ice numerics feature#1413
JessicaMeixner-NOAA merged 3 commits into
NOAA-EMC:developfrom
sbanihash:feature/add_reg_plus_fixes

Conversation

@sbanihash
Copy link
Copy Markdown
Collaborator

@sbanihash sbanihash commented Apr 18, 2025

Pull Request Summary

Adds IC_NUMERICS to the w3iogrmd.F90, plus addition of a regression test that uses the IC_NUMERICS option (ww3_tic1.1/IC4_M10_icenum)
Changes mod_def file which can be seen in matrix comparisons

Description

This PR addresses issue #1412 by adding the IC_NUMERICS to the w3iogrmd.F90 to make sure mod_def file accounts for the numerics option, and issue #1399 by adding a regression test (ww3_tic1.1/IC4_M10_icenum)

  • Mention any labels that should be added: bug +new feature

Issue(s) addressed

fixes issue #1412
addresses #1399

Commit Message

Bug fix for #1412 for IC_NUMERICS
Addition of regtest that turns on the IC_NUMERICS in IC4_M10 test case

Check list

Testing

How were these changes tested?
Matrix runs and comparisons for the changes made in w3iogrmd.F90 were conducted and results are attached.
Regtest for IC_NUMERICS has also been added as ww3_tic1.1/IC4_M10_icenum. (this additional test will also result in diffs in your matrix runs)

Are the changes covered by regression tests? Yes, see above
Have the matrix regression tests been run (if yes, please note HPC and compiler)?
yes, Hera intel.

Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

With expected mod_def file differences
matrixCompSummary.txt
matrixCompFull.txt
matrixDiff.txt

Taking out all mod_def differences, these are the other known diffs we get:

matdiff_minus_mod_def

@erinethomas
Copy link
Copy Markdown
Contributor

@sbanihash thanks for finding and making a PR to fix this!

@sbanihash sbanihash changed the title Fixes issue in PR#1294 to correctly account for IC_NUMERICS option + regression test for this new ice numerics feature Fix issue in PR#1294 to correctly account for IC_NUMERICS option + regression test for this new ice numerics feature Apr 21, 2025
@sbanihash sbanihash marked this pull request as draft April 23, 2025 14:01
@sbanihash sbanihash marked this pull request as ready for review April 23, 2025 17:11
Comment thread model/src/w3iogrmd.F90
STEXU, STEYU, STEDU, IICEHMIN, IICEHINIT, IICEDISP, &
ICESCALES(1:4), CALTYPE, CMPRTRCK, IICEHFAC, IICEHDISP,&
IICEDDISP, IICEFDISP, BTBETA, &
IICEDDISP, IICEFDISP, BTBETA,IC_NUMERICS, &
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @JessicaMeixner-NOAA for your review. New variable is added.

@JessicaMeixner-NOAA JessicaMeixner-NOAA added the mod_def change the binary mod_def file changes with the PR label Apr 28, 2025
@JessicaMeixner-NOAA
Copy link
Copy Markdown
Collaborator

Regression test output is consistent with @sbanihash results. New test: ww3_tic1.1/./work_IC4_M10_icenum + normal diffs + mod_def differences.

matrixCompFull.txt
matrixCompSummary.txt
matrixDiff.txt

@sbanihash
Copy link
Copy Markdown
Collaborator Author

@erinethomas @dabail10 The testing on our end is complete. Can you please test this branch within your coupled CESM and E3SM runs which was mentioned in the original PR and let us know if it works as expected?

Thanks

@dabail10
Copy link
Copy Markdown

I have run these changes in my CESM3 sandbox and they are running as expected.

@erinethomas
Copy link
Copy Markdown
Contributor

This is also working as expected in E3SM!

@sbanihash
Copy link
Copy Markdown
Collaborator Author

Thank you @erinethomas and @dabail10 for confirming. @JessicaMeixner-NOAA I think we are ready for this PR to be merged.

@JessicaMeixner-NOAA JessicaMeixner-NOAA merged commit d6c52ea into NOAA-EMC:develop Apr 29, 2025
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mod_def change the binary mod_def file changes with the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants