Skip to content

Conversation

eclare108213
Copy link
Contributor

@eclare108213 eclare108213 commented Mar 14, 2025

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

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]

Copy link

github-actions bot commented Mar 14, 2025

PR Preview Action v1.6.0

🚀 View preview at
https://E3SM-Project.github.io/E3SM/pr-preview/pr-7132/

Built to branch gh-pages at 2025-04-07 19:19 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@eclare108213
Copy link
Contributor Author

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 verboseRun = .true., and anything else.

@jonbob
Copy link
Contributor

jonbob commented Mar 24, 2025

@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

@jonbob jonbob added BFB PR leaves answers BFB NML labels Apr 7, 2025
@jonbob
Copy link
Contributor

jonbob commented Apr 7, 2025

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 eclare108213 marked this pull request as ready for review April 7, 2025 22:34
@jonbob
Copy link
Contributor

jonbob commented Apr 8, 2025

@eclare108213 -- should we think about getting rid of config_column_physics_type, if there's only one option now?

@eclare108213
Copy link
Contributor Author

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.

@jonbob
Copy link
Contributor

jonbob commented Apr 8, 2025

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?

@njeffery
Copy link
Contributor

njeffery commented Apr 8, 2025

We should eventually get rid of it, but I have no strong opinions for this PR.

@proteanplanet
Copy link
Contributor

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.

Copy link
Contributor

@njeffery njeffery left a 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.

Copy link
Contributor

@proteanplanet proteanplanet left a 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.

@jonbob
Copy link
Contributor

jonbob commented Apr 15, 2025

Also passes:
*SMS_D_Ln5.conusx4v1pg2_r05_IcoswISC30E3r5.F2010.chrysalis_intel

jonbob added a commit that referenced this pull request Apr 21, 2025
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]
@jonbob
Copy link
Contributor

jonbob commented Apr 21, 2025

merged to next

@jonbob jonbob merged commit d4287eb into master Apr 22, 2025
5 checks passed
@jonbob jonbob deleted the eclare/seaice/deprecate_colpkg branch April 22, 2025 19:19
@ndkeen
Copy link
Contributor

ndkeen commented Apr 22, 2025

Why are we merging to master if there are fails? You answered question on slack. Merge to master, make issue.

@jonbob
Copy link
Contributor

jonbob commented Apr 22, 2025

@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?

@jonbob
Copy link
Contributor

jonbob commented Apr 22, 2025

merged to master and expected NML and baseline DIFFs blessed -- except for pm-cpu which has not yet reported

@jonbob
Copy link
Contributor

jonbob commented Apr 23, 2025

also blessed on pm-cpu

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.

5 participants