-
Notifications
You must be signed in to change notification settings - Fork 435
Remove the old sea ice column package #7132
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
Conversation
…he BGC default to icepack.
|
ERS, PEM, PET and SMS tests pass using the E3SM-Polar-Developer.sh script. @jonbob Please let me know what I can do to help re additional testing, the script failure issue without |
@eclare108213 -- I'll see what I can do with the more local verboseRun, but otherwise that sounds good and I'll just run some E3SM tests as well |
Test merge passes e3sm_ice_developer and e3sm_cryo_developer on chrysalis, with expected NML DIFFs. @eclare108213 -- I think this can be moved from a draft PR |
@eclare108213 -- should we think about getting rid of config_column_physics_type, if there's only one option now? |
It's fine with me to get rid of it. Considerations are (1) backward compatibility and (2) the potential to reuse it for some other column physics combination (not sure what). Also, having it in there and explicitly set to 'icepack' clarifies that the run is using the new column physics option, if that is not obvious from other documentation. Let me know if you'd like me to hack it out. |
Thanks @eclare108213 -- I don't feel strongly, just always happier when we don't have a config flag with one option. @proteanplanet or @njeffery, do either of you have a strong opinion? |
We should eventually get rid of it, but I have no strong opinions for this PR. |
Backward compatibility is not an issue because that's what maintenance branches are for. However, we do need forward compatibility to use a C++ version of Icepack (Icepaxx) within MPAS-SeaIce, and therefore the flag should be kept, even though it currently can only take one value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Verified in a 5-year test that the fully-coupled BGC configuration runs correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is BFB with the current configuration of MPAS-SeaIce in E3SM and deprecates buggy code. If certain F-case configurations break with this change, the contingency is to change the F-cases to align with Icepack, however I cannot find a widely used F-case for which that would be needed.
Also passes: |
Remove the old sea ice column package Removes the old column package from MPAS-seaice, including changing the BGC default to Icepack. Namelist option config_column_physics_type has only one option (‘icepack’) and has been left in the code for now, but most lines using it have been commented out. Several options available in the old column package that were/are being deprecated in Icepack have been removed, including zsalinity and CESM melt ponds. There were also options to turn off individual thermodynamic processes in the column package for testing, which now are no longer available: config_use_congelation_basal_melt, config_use_lateral_melt, config_use_latent_processes. This PR changes the default column physics used for biogeochemistry from column_package to icepack. In ice_comp_mct.F, call the freezing temperature calculation from Icepack instead of the column_package, in routine ice_import_moab. This change might affect runs with a coupled ocean component. MPAS-seaice documentation has been updated accordingly. [NML] [BFB]
merged to next |
Why are we merging to master if there are fails? You answered question on slack. Merge to master, make issue. |
@ndkeen -- BGC doesn't normally run using the gnu compiler, so I think it's OK to merge to master and open an issue to fix it. We're already working up a fix, but may need you to test it on gcp12? |
merged to master and expected NML and baseline DIFFs blessed -- except for pm-cpu which has not yet reported |
also blessed on pm-cpu |
Removes the old column package from MPAS-seaice, including changing the BGC default to Icepack.
config_column_physics_type
has only one option (‘icepack’
) and has been left in the code for now, but most lines using it have been commented out.Several options available in the old column package that were/are being deprecated in Icepack have been removed, including zsalinity and CESM melt ponds. There were also options to turn off individual thermodynamic processes in the column package for testing, which now are no longer available:
config_use_congelation_basal_melt
,config_use_lateral_melt
,config_use_latent_processes
.This PR changes the default column physics used for biogeochemistry from
column_package
toicepack
.In ice_comp_mct.F, call the freezing temperature calculation from Icepack instead of the column_package, in routine ice_import_moab. This change might affect runs with a coupled ocean component.
MPAS-seaice documentation has been updated accordingly.
BFB in 3-month D cases using E3SM-Polar-Developer.sh, with
verboseRun = .true.
in mpas-seaice/src/shared/mpas_seaice_advection_incremental_remap.F. Runs without this setting also fail for the current E3SM master, using the E3SM-Polar-Developer.sh script.[NML]
[BFB]