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

Merge E3SM Icepack cice-consortium/E3SM-icepack-initial-integration to Consortium #460

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Sep 26, 2023

PR checklist

  • Short (1 sentence) summary of your PR:
    Merge E3SM Icepack cice-consortium/E3SM-icepack-initial-integration to Consortium

  • Developer(s):
    apcraig, eclare108213, dabail19

  • 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.
    underway....

  • How much do the PR code changes differ from the unmodified code?

    • bit for bit, except modal results
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?

    • Yes, will need to update CICE
    • 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:

  • Update dEdd shortwave, snicar, Snicar-ad port to Icepack from MPAS-SI E3SM-Project/Icepack#7

    • Update snicar-ad table lookups, hardcode tables, provide additional options
    • Update dEdd implementation
    • Support dEdd 3-band and 5-band data
    • Support full snicar tables, hardcoded into model, requires USE_SNICARHC cpp
    • Support smaller test data table
    • Add icepack_shortwave_data file/module to store snicar table data
  • Update modal aerosols, this changes answers in Icepack standalone

  • Update public interfaces

    • remove single nspint parameter, add 3 and 5 band nspint parameters
    • add icepack_init_radiation
    • add icepack_salinity_profile
    • add icepack_enthalpy mush
    • add icepack_warnings_getall
    • modify icepack_prep_radiation, remove scalar parameters (ncat, nilyr, nslyr)
    • modify icepack_step_radiation, remove scalar parameters (ncat, nblyr, nilyr, nslyr, dEdd_algae, calendar_type, days_per_year, nextsw_cday, modal_aero), remove snicar table data arguments
    • add Tf argument to icepack_compute_tracers, icepack_aggregate, icepack_step_ridge,
  • Modify icepack parameters

  • Update Tf implementation to make internally consistent in multiple places, First part of adding Tf. E3SM-Project/Icepack#12

    • remove hardcoded Tocnfrz in some places
    • add tfrz_option = 'constant' which sets Tf to Tocnfrz as an option
    • add "_old" options to tfrz_option for backwards compatibility (will be removed)
  • Modify iDin computation in subroutine compute_microS_mushy

  • Fix fhtan computation in compute_albedos subroutine

  • Modify some internal subroutine interfaces, remove parameters from arguments, leverage module data instead

  • Rename orb interfaces, change from shr_orb to icepack_orb to better differentiate internal and external versions of the orbital code

  • Refactor some optional argument handling

  • Clean up icepack driver namelist logging

  • Clean up some formatting

  • Add ability to reuse Icepack binary in Icepack test scripts

  • Add snicar_suite test suite

  • Update documentation

NOTE: TO DO AFTER MERGE

  • Clean up "_old" tfrz_options and validate results
  • Have USE_SNICARHC cpp be "on" all the time with option to turn off for testing
  • Update CICE calls to pass frazil ice settings at initialization and remove frazil ice settings from runtime interfaces. Then update Icepack to remove those optional frazil ice arguments in runtime interfaces.

eclare108213 and others added 30 commits August 4, 2022 13:56
Update to Consortium Main Aug 17, 2022
Add calls to icepack_init_radiation to icepack driver
Add some SNICAR SSP table checks, aborts, etc
new namelist variables and options.

Add new namelist to icepack_in.  Add snicar and snicartest
options.

Minor updates to namelist output
Update snow table implementation and add SNICAR SSP Tables
Update ssp data for dEdd_snicar
Add additional SNICAR SSP fields for aerosols, BGC
apcraig and others added 9 commits August 20, 2023 09:39
Merge #86cae16d1b7c4 from CICE-Consortium/main Aug 19, 2023
…o cice-consortium/E3SM-icepack-initial-integration

Merge Icepack main #23b6c1272b50d42cad, includes thin ice enthalpy fix and zsalinity deprecation
…o e3sm230830

Update to Icepack Consortium #7952807, Sept 13, 2023
Merge Consortium Icepack #23b6c1272b50d4, Aug 30, 2023
Update to Consortium #795280797a Sept 13, 2023
Adjust icepack interface for E3SM
@apcraig
Copy link
Contributor Author

apcraig commented Sep 26, 2023

I will review the documentation and propose some changes via a PR to E3SM-Project:cice-consortium/E3SM-icepack-initial-integration. Once that's ready, we can test/review/merge this PR to the Consortium.

@@ -27,15 +27,15 @@ module icepack_itd

use icepack_kinds
use icepack_parameters, only: c0, c1, c2, c3, c15, c25, c100, p1, p01, p001, p5, puny
use icepack_parameters, only: Lfresh, rhos, ice_ref_salinity, hs_min, cp_ice, Tocnfrz, rhoi
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we fix something like this in Icepack already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The Tf/Tocnfrz was implemented on the E3SM side, and we have not moved any of those changes over to the Consortium. In hindsight, it probably should have been done on the Consortium side and moved to the E3SM side. I think that was early days of the E3SM Icepack version, and we probably hadn't realized that we should do changes, when possible, in the Consortium. That approach was adopted later.

Copy link
Contributor

@dabail10 dabail10 left a comment

Choose a reason for hiding this comment

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

If this is all bfb, then I don't see any issues. I was confused about the Tocnfrz versus Tf thing. I know we removed hard-coded -1.8 when possible, but then we decided not to update to Tf as this would change answers? Can someone remind me what happened here?

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.

Just one comment here which is more appropriate for the documentation PR. I'll add others there.

@@ -464,6 +464,7 @@ either Celsius or Kelvin units). Deprecated parameters are listed at the end.
"time_forc", "time of last forcing update", "s"
"Timelt", "melting temperature of ice top surface", "0. C"
"TLAT", "latitude of cell center", "radians"
"Tliquidus_max", "maximum liquidus temperature of mush", "0. C"
Copy link
Contributor

Choose a reason for hiding this comment

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

for Tocnfrz below, shouldn't we leave the value blank rather than have -1.8 (which has its own option under tfrz_option)?
And I think Tocnfrz should have a \bullet, since it's a namelist parameter now... except that apparently it's not a namelist parameter even though it's in icepack_parameters. Shouldn't it be?

@apcraig
Copy link
Contributor Author

apcraig commented Sep 28, 2023

@dabail10, the Tf/Tocnfrz thing is a little confusing. I think what was done was the following. There were a few places where the freezing T was hardwired, those were modified so the code uses Tf (the computed value of freezing temp) and is internally consistent. Then we added a new option for tfrz_option, "constant", which allows the user to set the value of Tocnfrz via icepack_parameters. The default for Tocnfrz is -1.8. The other options for tfrz_option is "minus1p8", "linear_salt", and "mushy". The difference between "constant" and "minus1p8" is that the former allows the user to set a Tocnfrz value that will be used while the latter always uses -1.8.

We are also supporting "_old" options for all of these settings. That provides backward compatibility for CICE testing to make sure we're not changing answers overall relative the Consortium main. The "_old" version just sets trcrn(It) in icepack_compute_tracers to Tocnfrz (-1.8 by default) instead of Tf (the freezing temperature associated with the tfrz_option). These "_old" versions will be deprecated after the code is moved to Consortium main, that will change answers but be more correct. The "_old" versions exist just for testing.

apcraig and others added 4 commits September 28, 2023 12:12
…o upd230930

Merge Consortium main #390fb5518326217c2501dbf5 to branch, Sept 30, 2023
Update Icepack Consortium main #390fb5518326217c2501dbf5, Sept 30, 2023
@apcraig
Copy link
Contributor Author

apcraig commented Oct 4, 2023

Testing vs Icepack Consortium and CICE Consortium mains performed as expected.
https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#96f2fc707fc743d7ce6eb8f543bd449a35a1a649
https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#7603016e1db8cf6d752e3b35bfbe18391639d4db

  • Icepack modal tests changed answers relative to Consortium main
  • CICE restart files have different surface temp values at non-ice points, but log files are identical
  • CICE boxchanl1e and boxchan1n changed answers, had to set tfrz_option to 'mushy_old' to recover prior results.

This is ready for review and merge.

@apcraig apcraig marked this pull request as ready for review October 4, 2023 20:52
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.

This code has been thoroughly tested in E3SM and is BFB in most cases for the Consortium.

@eclare108213 eclare108213 merged commit 8fad768 into CICE-Consortium:main Oct 4, 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