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

generalize logic for coupling frazil fluxes #458

Merged
merged 5 commits into from
Sep 13, 2023

Conversation

eclare108213
Copy link
Contributor

@eclare108213 eclare108213 commented Sep 9, 2023

Draft PR for sanity review and further testing in Icepack and CICE.

  • Short (1 sentence) summary of your PR:
    Adds a new namelist parameter (cpl_frazil) and generalizes the logic for coupling frazil fluxes
  • 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.

These changes have been tested in E3SM.

Base_suite with regression:
178 measured results of 178 total results
178 of 178 tests PASSED
0 of 178 tests PENDING
0 of 178 tests MISSING data
0 of 178 tests FAILED
https://github.com/CICE-Consortium/Test-Results/wiki/e4bc61184b.chicoma.intel.23-09-11.142102.0

  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • 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/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

Adds namelist parameter cpl_frazil, needed to enable E3SM's style of coupling.
Uses update_ocn_f in icepack_parameters rather than icedrv_flux.
Removes update_ocn_f from icedrv/step_therm2 argument lists and makes it optional in icepack_step_therm2.
Changes frazil dfresh, dsalt logic to include cpl_frazil option but the calculations should be BFB backward compatible.
Updates interfaces.include and documentation.

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

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

Overall, this looks right. Just one change needed and one comment. If you'd like, I can run a full test suite with Icepack and CICE when it's ready, just let me know.

@@ -2140,6 +2138,9 @@ subroutine icepack_step_therm2 (dt, ncat, nltrcr, &
!-----------------------------------------------------------------

if (icepack_chkoptargflag(first_call)) then
if (present(update_ocn_f)) then
call icepack_init_parameters(update_ocn_f_in=update_ocn_f)
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This new code needs to be outside icepack_chkoptargflag. That interface is for checking whether optional arguments that are needed are passed, and the settings reflect never checking to always checking. The icepack_init_parameters call should be outside that if block. If a user passes update_ocn_f in, then the code should update the value in parameters. Otherwise, just continue. And it might vary by gridcell or timestep (even if it shouldn't).

@@ -369,6 +369,9 @@ forcing_nml
"", "``minus1p8``", "constant ocean freezing temperature (:math:`-1.8^{\circ} C`)", ""
"", "``mushy``", "matches mushy-layer thermo (ktherm=2)", ""
"``trestore``", "integer", "sst restoring time scale (days)", "90"
"``cpl_frazil``", "``external``", "frazil water/salt fluxes are handled outside of Icepack", "``fresh_ice_correction``"
"", "``internal``", "send full frazil water/salt fluxes for mushy physics", ""
"", "``fresh_ice_correction``", "correct fresh-ice frazil water/salt fluxes for mushy physics", ""
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be alphabetical to be consistent with the rest of the document (hopefully). So external, fresh_ice_correction, then internal. Should just have to swap the order of internal and fresh_ice_correction is you want to follow that format.

@eclare108213
Copy link
Contributor Author

Base_suite tests passed. @apcraig please go ahead and run your complete set of tests -- thank you. I'll begin porting the driver changes into CICE.

@eclare108213 eclare108213 marked this pull request as ready for review September 11, 2023 14:54
@apcraig
Copy link
Contributor

apcraig commented Sep 11, 2023

@apcraig
Copy link
Contributor

apcraig commented Sep 11, 2023

@eclare108213, I think this can be merged if you feel it's ready.

@eclare108213 eclare108213 merged commit 7952807 into CICE-Consortium:main Sep 13, 2023
1 check passed
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