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

Enables and modifies the submesoscale eddy parameterization #5172

Merged

Conversation

vanroekel
Copy link
Contributor

@vanroekel vanroekel commented Sep 2, 2022

This enables the Fox-Kemper et al 2011 submesoscale eddy parameterization as the default for all configurations.

It also fixes a unit error in the submesoscale eddy code that did not influence the long control run, but was introduced in the first PR (#5099). The variable gradBuoyEddy was not the buoyancy but the density gradient. Here the name is changed to gradDensityEddy for clarity.

[NML]
[CC]

@vanroekel vanroekel added mpas-ocean CC PR is climate changing labels Sep 2, 2022
@vanroekel
Copy link
Contributor Author

@mark-petersen
Copy link
Contributor

This is the metric that shows the major change due to turning on the mesoscale eddy parameterization:
image

@mark-petersen
Copy link
Contributor

Adding reviewers from the AMOC working group, who were involved in these changes. The code is already in. Here we simply turn it on. Your approval here really says that you agree with setting config_submesoscale_enable = .true. on all simulations, based on the 700+ years test above (as well as all the other testing we did along the way).

@mark-petersen
Copy link
Contributor

This PR requires that #5167 and #5171 be merged first. Updating the MOC calculation #5170 is needed to produce the same AMOC statistic as above.

Comment on lines 1113 to 1114
lines.append(' <var name="normalMLEvelocity"/>')
lines.append(' <var name="vertMLEVelocityTop"/>')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
lines.append(' <var name="normalMLEvelocity"/>')
lines.append(' <var name="vertMLEVelocityTop"/>')
lines.append(' <var name="normalMLEvelocity"/>')
lines.append(' <var name="vertMLEVelocityTop"/>')

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding theses! They are needed for the offline MOC and are likely to be useful for debugging.

Copy link
Contributor

@xylar xylar Sep 10, 2022

Choose a reason for hiding this comment

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

We need to add them to standalone as well. We want to maintain consistency:
https://github.com/E3SM-Project/E3SM/blob/master/components/mpas-ocean/src/analysis_members/Registry_time_series_stats_monthly_mean.xml#L135-L136

It seems like we probably want to enable config_submesoscale_enable = .true. in many compass tests, too, following this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@jonbob
Copy link
Contributor

jonbob commented Sep 21, 2022

@vanroekel - testing of this PR fails due to the addition of vertMLEVelocityTop to the streams file. Apparently that array is not defined anywhere in the mpas-o source?

@vanroekel
Copy link
Contributor Author

@jonbob -- sorry about that -- this variable should be vertMLEBolusVelocityTop -- do you want me to push a fix?

@jonbob
Copy link
Contributor

jonbob commented Sep 21, 2022

@vanroekel - I can take care of that, if you're jammed up. I just removed the bad arrays from the streams for testing, so it's not holding anything up

@vanroekel vanroekel changed the title Enables submesoscale eddy parameterization Enables and modifies the submesoscale eddy parameterization Sep 25, 2022
@vanroekel
Copy link
Contributor Author

@jonbob I pushed the fixes I gave you in testing #5171 here, updated the comment, and PR title. Let me know if there are any issues.

@jonbob
Copy link
Contributor

jonbob commented Sep 28, 2022

@vanroekel - do you want to change config_submesoscale_Ce to 0.08 in this PR, to match the value of the long run you did?

@vanroekel
Copy link
Contributor Author

yes definitely. thanks for catching that @jonbob it should be changed now

@mark-petersen
Copy link
Contributor

Coordinated with Luke: rebased on master after #5171 merge. Compiled with gnu and intel, tested with nightly stand-alone suite with gnu debug and config_submesoscale_enable set to both true and false.

@mark-petersen mark-petersen force-pushed the vanroekel/ocean/enable-submesoscale-eddy-parameterization branch from b1f85d7 to ddd41fd Compare September 29, 2022 21:59
@jonbob
Copy link
Contributor

jonbob commented Sep 29, 2022

@mark-petersen - I had already started merging this to next for testing

@mark-petersen mark-petersen self-requested a review September 29, 2022 22:21
@jonbob
Copy link
Contributor

jonbob commented Sep 29, 2022

@xylar, @lconlon, @katsmith133 - can you please review (or finish your review)?

Copy link

@katsmith133 katsmith133 left a comment

Choose a reason for hiding this comment

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

Looks good to me based upon visual inspection

@jonbob
Copy link
Contributor

jonbob commented Sep 29, 2022

@vanroekel - can you check and make sure I changed the streams output field to the one you intended?

@mark-petersen
Copy link
Contributor

Tested this PR on Chrysalis with

./create_test SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.chrysalis_gnu -q acme-small --walltime 30:00

and it died on run start-up with the problem in #5204. I'll see if I can figure that out tomorrow.

@jonbob
Copy link
Contributor

jonbob commented Sep 29, 2022

@mark-petersen -- acme-small is a queue on anvil, not chrysalis. You also need to add " --project e3sm" on chrysalis. For me, a test merge passes SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel. I just started SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.chrysalis_gnu and it just successfully completed as well.

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.

Approved based on code inspection and @vanroekel's and @jonbob's testing.

@vanroekel
Copy link
Contributor Author

@jonbob streams changes look good to me. Thanks!

Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

approved based on testing above.

@mark-petersen
Copy link
Contributor

Thanks all. This is a big accomplishment. There were sufficient reviewers, so I'm removing LeAnn and Alice, who are out.

jonbob added a commit that referenced this pull request Sep 30, 2022
…on' into next (PR #5172)

Enables and modifies the submesoscale eddy parameterization

This enables the Fox-Kemper et al 2011 submesoscale eddy
parameterization as the default for all configurations.

It also fixes a unit error in the submesoscale eddy code that did not
influence the long control run, but was introduced in the first PR
(#5099). The variable gradBuoyEddy was not the buoyancy but the density
gradient. Here the name is changed to gradDensityEddy for clarity.

[NML]
[CC]
@jonbob
Copy link
Contributor

jonbob commented Sep 30, 2022

test merge passes:

  • PEM_Ln9.ne30pg2_EC30to60E2r2.WCYCL1850.chrysalis_intel
  • ERS_Ld5.T62_oQU120.CMPASO-NYF.chrysalis_intel
  • SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel

with expected NML and regular DIFFs

merged to next

@jonbob jonbob merged commit bcf0289 into master Oct 3, 2022
@jonbob jonbob deleted the vanroekel/ocean/enable-submesoscale-eddy-parameterization branch October 3, 2022 15:17
@jonbob
Copy link
Contributor

jonbob commented Oct 3, 2022

merged to master and expected DIFFs and NML DIFFs blessed, except:

SMS_PS.northamericax4v1pg2_WC14to60E2r3.WCYCL1850.chrysalis_intel.allactive-wcprodrrm

which did not complete but was running fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CC PR is climate changing mpas-ocean NML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants