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

Access alternative congelation ice formulation in icepack #965

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

eclare108213
Copy link
Contributor

@eclare108213 eclare108213 commented Aug 3, 2024

  • Short (1 sentence) summary of your PR:
    Provides namelist and brief documentation for new, alternative congelation ice formulation in Icepack,
    https://github.com/CICE-Consortium/Icepack/pull/494/files
  • Developer(s):
    @eclare108213
  • 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.

base_suite comparison of the new code using default settings with the original code prior to modifications:

391 measured results of 391 total results
377 of 391 tests PASSED
2 of 391 tests PENDING
4 of 391 tests MISSING data
8 of 391 tests FAILED

The 8 failed tests are expected on chicoma (due to large processor counts; missing forcing data), and there are always 2 tests left in pending status when all have finished. The tests with missing data are the new ones for testing the congel_freeze='one-step' option.

QC testing was done comparing the 'two-step' and new 'one-step' option and this passed, see additional information below.

  • How much do the PR code changes differ from the unmodified code?
    • bit for bit when congel_freeze = 'two-step' (default)
    • different at roundoff level
    • more substantial when congel_freeze = 'one-step'
  • 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 document the changes in detail, including why the changes are made. This will become part of the PR commit log.

Add support for 'one-step' congelation ice formation in addition to current default 'two-step' option. Added a new namelist, congel_freeze to control the congelation option. This CICE update requires an update to Icepack to support the new congel_freeze option. The default setting for congel_freeze is set to 'two-step' for backwards compatibility. The new 'one-step' option will be evaluated and may become the default option in the future.

Testing indicates the default setting is bit-for-bit identical with the current main.

@eclare108213 eclare108213 changed the title access alternative congelation ice formulation in icepack Access alternative congelation ice formulation in icepack Aug 3, 2024
@apcraig
Copy link
Contributor

apcraig commented Aug 7, 2024

I ran a QC test and it passed. This is what I did. Using derecho, intel, and 128 cores. I checked out the current main branch of Icepack and CICE and then merged in the icongel and congel branches. I then setup two qc cases, one with "one-step", the other with "two-step". The congel_freeze namelist was the only difference. I attach the images. "qc1" is one-step, "qc2" is two-step.

./cice.t-test.py $WORKDIR/CICE_RUNS/derecho_intel_smoke_gx1_128x1_medium_qc.qc1s $WORKDIR/CICE_RUNS/derecho_intel_smoke_gx1_128x1_medium_qc.qc2s
INFO:__main__:Running QC test on the following directories:
INFO:__main__:  /glade/derecho/scratch/tcraig/CICE_RUNS/derecho_intel_smoke_gx1_128x1_medium_qc.qc1s
INFO:__main__:  /glade/derecho/scratch/tcraig/CICE_RUNS/derecho_intel_smoke_gx1_128x1_medium_qc.qc2s
INFO:__main__:Number of files: 1825
INFO:__main__:2 Stage Test Passed
INFO:__main__:Quadratic Skill Test Passed for Northern Hemisphere
INFO:__main__:Quadratic Skill Test Passed for Southern Hemisphere
INFO:__main__:Creating map of the data (ice_thickness_derecho_intel_smoke_gx1_128x1_medium_qc.qc1.png)
INFO:__main__:Creating map of the data (ice_thickness_derecho_intel_smoke_gx1_128x1_medium_qc.qc2.png)
INFO:__main__:Creating map of the data (ice_thickness_derecho_intel_smoke_gx1_128x1_medium_qc.qc1_minus_derecho_intel_smoke_gx1_128x1_medium_qc.qc2.png)
INFO:__main__:
INFO:__main__:Quality Control Test PASSED

ice_thickness_derecho_intel_smoke_gx1_128x1_medium_qc qc1

ice_thickness_derecho_intel_smoke_gx1_128x1_medium_qc qc2

ice_thickness_derecho_intel_smoke_gx1_128x1_medium_qc qc1_minus_derecho_intel_smoke_gx1_128x1_medium_qc qc2

@apcraig
Copy link
Contributor

apcraig commented Aug 7, 2024

The two-step is the old implementation. One-step is the new and improved implementation. Do we want to make congel_freeze='one-step' the default?

@dabail10
Copy link
Contributor

dabail10 commented Aug 7, 2024

Let me test it in coupled first.

@apcraig
Copy link
Contributor

apcraig commented Aug 7, 2024

Should we merge this week with old 'two-step' default then update the default next week or after if we gain confidence?

@dabail10
Copy link
Contributor

dabail10 commented Aug 7, 2024

What happened with the gh-actions? I am happy to merge this with two-step as the default.

@apcraig
Copy link
Contributor

apcraig commented Aug 7, 2024

What happened with the gh-actions? I am happy to merge this with two-step as the default.

We need to merge the Icepack PR first, then update this PR which depends on it. Then GH actions should be OK. I will merge take care of those steps now and we can decide if we want to move away from two-step as the default later.

implementation, port to casper, updated documentation.
@apcraig
Copy link
Contributor

apcraig commented Aug 7, 2024

I just updated the Icepack version on this branch/PR. This should be ready to merge, will do some additional checks and wait for GH actions to run.

@apcraig apcraig marked this pull request as ready for review August 7, 2024 20:08
@apcraig
Copy link
Contributor

apcraig commented Aug 7, 2024

I updated the PR documentation and this PR should be ready to merge. @eclare108213, @dabail10, let me know if there are any concerns. Otherwise I'll merge before the weekend.

@anton-seaice
Copy link
Contributor

Thanks for this @eclare108213 and @apcraig - I agree we shouldn't change the default.

@eclare108213
Copy link
Contributor Author

I also agree, we should leave the default as 'two-step' until at least one of the modeling centers has a chance to evaluate the change in a coupled system. This PR is ready to merge. Thanks @apcraig for your help!

@apcraig apcraig merged commit 350d34a into CICE-Consortium:main Aug 8, 2024
2 checks passed
NickSzapiro-NOAA pushed a commit to NOAA-EMC/CICE that referenced this pull request Sep 25, 2024
…rtium#965)

Add support for 'one-step' congelation ice formation in addition to current default 'two-step' option. Added a new namelist, congel_freeze to control the congelation option. This CICE update requires an update to Icepack to support the new congel_freeze option. The default setting for congel_freeze is set to 'two-step' for backwards compatibility. The new 'one-step' option will be evaluated and may become the default option in the future.

Testing indicates the default setting is bit-for-bit identical with the current main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants