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

Add parametrized tidal mixing effects to ice shelf basal melt #6306

Merged

Conversation

irenavankova
Copy link
Contributor

@irenavankova irenavankova commented Mar 26, 2024

An implementation of prescribed tidal effect on ice shelf basal melting following (Jourdain et al., 2019):

https://doi.org/10.1016/j.ocemod.2018.11.001

The tidal flow speeds used to produce a forcing data file are calculated from the CATS 2008 (an update of Padman et al., 2002). When the file is available and compset TMIX on, the Jourdain formula is used. The main change is that what used to be a constant config_land_ice_flux_rms_tidal_velocity is now a spatially variable (horizontal) field.

Related Ocean Discussion is here:

E3SM-Ocean-Discussion#76

[NML]
[BFB]

@irenavankova irenavankova force-pushed the ocn/add-tidal-mixing-to-ice-shelf-melt branch from 4d22b59 to d4fb283 Compare March 26, 2024 22:40
@xylar xylar self-requested a review March 27, 2024 14:50
@xylar xylar added mpas-ocean Stealth PR has feature which, if turned on, could change climate. fka FCC labels Mar 27, 2024
@xylar
Copy link
Contributor

xylar commented Mar 28, 2024

@jonbob, I've marked this as "Stealth" but I'm not sure if that is correct. Let me know what you think. I'm going to run an Icos G-case with this configuration for 10 years and see what we get. I'll compare with the same configuration without tidal mixing. Hopefully, that's sufficient as a test of this capability as a stealth feature. I'm also going to figure out where to add an appropriate test.

@xylar
Copy link
Contributor

xylar commented Mar 28, 2024

@jonbob, it's certainly conceivable that we could make this the default behavior for Polar runs. That's the sense in which I was thinking of it as a stealth feature.

@jonbob
Copy link
Contributor

jonbob commented Mar 28, 2024

I think in our current definition, this is a separate configuration and we can remove the stealth label and add a new test to extra coverage. But it's still important to document what impact it has as part of the PR

@xylar xylar added BFB PR leaves answers BFB and removed Stealth PR has feature which, if turned on, could change climate. fka FCC labels Mar 28, 2024
@xylar
Copy link
Contributor

xylar commented Mar 28, 2024

@irenavankova, I hope you don't mind that I pushed a test.

@@ -239,6 +239,7 @@
"tests" : (
"ERS_P480_Ld5.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF.mpaso-jra_1958",
"PEM_P480_Ld5.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF.mpaso-jra_1958",
"SMS_P480_Ld5.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF-TMIX.mpaso-jra_1958",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jonbob, does this look right?

I was able to run this on Chrysalis by excluding mpaso-jra_1958 but didn't know how to test that part.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can do that when I work on this PR -- there is a bit of a trick to specifying tests that use testdefs

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

The easiest way is to add machine_compiler to the test name.
./creat_test SMS_P480_Ld5.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF-TMIX.chrysalis_intel.mpaso-jra_1958

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @rljacob!

Copy link
Contributor

@xylar xylar left a comment

Choose a reason for hiding this comment

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

I ran 2 Icos30 G-cases for 10 years, one with and one without tidal mixing:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xylar/analysis/tmix_vs_no_tmix_20240328.GMPAS-JRA1p5-DIB-PISMF.TL319_IcoswISC30E3r5.chrysalis/yrs-01-10/ocean/index.html

The results are as expected: only the melt rates below ice shelves and the adjacent ocean properties are affected, the rest of the climate is nearly identical.

Interestingly, there is substantially less melting overall and particularly in the Amundsen and Bellingshausen regions:
image
image
image

This likely suggests that other coefficients related to melting will need to be retuned in coordinating with adopting this parameterization in Polar simulations, but that is work for a follow-up PR.

@@ -239,6 +239,7 @@
"tests" : (
"ERS_P480_Ld5.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF.mpaso-jra_1958",
"PEM_P480_Ld5.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF.mpaso-jra_1958",
"SMS_P480_Ld5.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF-TMIX.mpaso-jra_1958",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @rljacob!

@jonbob jonbob added the NML label May 2, 2024
@jonbob
Copy link
Contributor

jonbob commented May 15, 2024

@irenavankova -- I ran the scripts that make the bld files consistent with Registry and they came up with some differences in namelist_definition_mpaso.xml:

--- a/components/mpas-ocean/bld/namelist_files/namelist_definition_mpaso.xml
+++ b/components/mpas-ocean/bld/namelist_files/namelist_definition_mpaso.xml
@@ -1608,25 +1608,25 @@ Default: Defined in namelist_defaults.xml
 
 <entry id="config_land_ice_flux_tidal_Jourdain_alpha" type="real"
        category="land_ice_fluxes" group="land_ice_fluxes">
-Constant in parameterization of tidal velocity used in computing the sub-ice-shelf friction velocity
+alpha from parameterization of tidal velocity by Jourdain et al., 2019 - Equation 9, alpha = 0.777 when TMIX compset on
 
-Valid values: Any positive real number
+Valid values: Any non-negative real number
 Default: Defined in namelist_defaults.xml
 </entry>
 
 <entry id="config_land_ice_flux_tidal_Jourdain_A0" type="real"
        category="land_ice_fluxes" group="land_ice_fluxes">
-Constant in parameterization of tidal velocity used in computing the sub-ice-shelf friction velocity
+A0 from parameterization of tidal velocity by Jourdain et al., 2019 - Equation 9, A0 = 0.656 when TMIX compset on
 
-Valid values: Any positive real number
+Valid values: Any non-negative real number
 Default: Defined in namelist_defaults.xml
 </entry>
 
 <entry id="config_land_ice_flux_tidal_Jourdain_U0" type="real"
        category="land_ice_fluxes" group="land_ice_fluxes">
-Constant in parameterization of tidal velocity used in computing the sub-ice-shelf friction velocity
+U0 from parameterization of tidal velocity by Jourdain et al., 2019 - Equation 9, U0 = 0.003 m/s when TMIX compset on
 
-Valid values: Any positive real number
+Valid values: Any non-negative real number
 Default: Defined in namelist_defaults.xml
 </entry>

Can you please help me resolve the differences?

jonbob added a commit that referenced this pull request May 28, 2024
…to next (PR #6306)

Add parametrized tidal mixing effects to ice shelf basal melt

An implementation of prescribed tidal effect on ice shelf basal melting
following (Jourdain et al., 2019). The tidal flow speeds used to produce
a forcing data file are calculated from the CATS 2008 (an update of
Padman et al., 2002). When the file is available and compset TMIX on,
the Jourdain formula is used. The main change is that what used to be a
constant config_land_ice_flux_rms_tidal_velocity is now a spatially
variable (horizontal) field.

This implmentation is accessed via a compset option to mpaso

[NML]
[BFB]
@jonbob
Copy link
Contributor

jonbob commented May 28, 2024

passes:

  • ERS_Ld5.T62_oQU120.CMPASO-NYF.chrysalis_intel
  • SMS_D_Ld1.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF.chrysalis_intel.mpaso-jra_1958

with expected NML DIFFs. Also passes:

  • SMS_P480_Ld5.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF-TMIX.chrysalis_intel.mpaso-jra_1958

which is the new test introduced by this PR

merged to next

@jonbob
Copy link
Contributor

jonbob commented May 28, 2024

verified that all new files are available on the inputdata repo and have correct permissions

@jonbob
Copy link
Contributor

jonbob commented May 29, 2024

@irenavankova and @xylar -- it looks like this PR was non-BFB on pm-cpu using the gnu compiler. From looking at the changes, it can only be order-of-operations level though of course with the model any little change becomes larger. How do you want to handle this? It is only the one machine and one compiler. I tested it on chrysalis with gnu and did not see any DIFFs and overnight testing with intel passed

@jonbob jonbob merged commit 24bdc2d into E3SM-Project:master May 29, 2024
2 checks passed
@xylar
Copy link
Contributor

xylar commented May 29, 2024

Oh, that's odd. I guess since you merged this, you think it's okay. I guess we'll need to update baselines for pm-cpu? I think that's likely okay since it was BFB on other machines.

Comment on lines -3617 to +3620
+ config_land_ice_flux_rms_tidal_velocity**2))
+ config_land_ice_flux_tidal_Jourdain_alpha**2 * &
(config_land_ice_flux_tidal_Jourdain_A0 * velocityTidalRMS(iCell) + &
config_land_ice_flux_tidal_Jourdain_U0)**2))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only term I could imaging causing the non-BFB results. It's hard for me to understand how a reasonable compiler doesn't produce BFB results when config_land_ice_flux_tidal_Jourdain_alpha = 1.0, config_land_ice_flux_tidal_Jourdain_A0 = 0.0 and config_land_ice_flux_tidal_Jourdain_U0 is identical to the old config_land_ice_flux_rms_tidal_velocity but I guess gnu on pm-cpu might just not be reasonable...

Copy link
Contributor

Choose a reason for hiding this comment

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

That was my thought as well -- but I did want to point it out just in case

@xylar
Copy link
Contributor

xylar commented May 29, 2024

Thanks for the help with this, @jonbob and thanks for working so hard on it, @irenavankova!

@jonbob
Copy link
Contributor

jonbob commented May 29, 2024

I did go ahead and merge it because it passed on our production platforms and compilers -- it seems like a one-off. I can just bless those DIFFs, or if you want we can rework the code section that actually does the work and try to get it BFB on pm-cpu_gnu? I'm guessing it would be possible using the new configs that were added? Or decrease the level of optimization for the affected file?

@xylar
Copy link
Contributor

xylar commented May 29, 2024

I think we should just bless the difference. The only simulations where this is going to have any impact at all (apart from ensemble variability) are those with prognostic ice-shelf melting and we don't have any production simulations going like that right now.

@jonbob
Copy link
Contributor

jonbob commented May 29, 2024

OK, I blessed the DIFFs for the PISMF tests on pm-cpu. There is another new DIFF that doesn't make sense to me:

ERS_Ld5.TL319_oQU240wLI_ais20.MPAS_LISIO_JRA1p5.pm-cpu_gnu.mpaso-ocn_glcshelf

@xylar
Copy link
Contributor

xylar commented May 30, 2024

That's also PISMF, but through the coupler so it makes sense to me. The friction velocity, modified by this PR, will be used there as well.

@jonbob
Copy link
Contributor

jonbob commented May 30, 2024

Thanks @xylar -- I'll bless that test as well

xylar added a commit to xylar/compass that referenced this pull request Jun 4, 2024
xylar added a commit to xylar/compass that referenced this pull request Jun 4, 2024
This merge updates the E3SM-Project submodule from [31e0924](https://github.com/E3SM-Project/E3SM/tree/31e0924) to [8939709](https://github.com/E3SM-Project/E3SM/tree/8939709).

This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list):
- [ ]  (ocn) E3SM-Project/E3SM#6263
- [ ]  (ocn) E3SM-Project/E3SM#6310
- [ ]  (fwk) E3SM-Project/E3SM#6427
- [ ]  (ocn) E3SM-Project/E3SM#6397
- [ ]  (ocn) E3SM-Project/E3SM#6306
- [ ]  (ocn) E3SM-Project/E3SM#6454
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB mpas-ocean NML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants