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

Adds the Bodner et al. 2023 version of MLE #352

Merged
merged 3 commits into from
May 30, 2023

Conversation

abodner
Copy link

@abodner abodner commented Apr 20, 2023

This PR involves the implementation of the Bodner 2023 submesoscale scheme to include new frontal width scaling with dependence on surface atmospheric forcing parameters via the ePBL scheme. The new scheme reveals non-zero changes in the mixed layer depth, in particular shoaling of the mixed layer depth in the lower latitudes and deepening in higher latitudes.
MLD changes

adcroft and others added 3 commits April 14, 2023 10:13
- Renamed function from psi(z) to mu(sigma)
- Added comments and units in function mu(sigma)
- Added [numerical] unit tests for mu(z), including special limits,
  special values, and one test value (checked against a python script).
Changes:

- Allow MLE parameterization to see surface buoyancy flux return from
  PBL scheme (affects MOM.F90, MOM_variables.F90:vertvisc_type,
  MOM_diabatic_driver.F90, MOM_set_viscosity.F90)

- Adds the Bodner et al., 2023, parameterization of restratification by
  mixed-layer eddies to MOM_mixed_layer_restrat.F90

  - This is a new subroutine rather than embedded inside the previous
    "OM4" version. It uses different inputs, different parameters,
    filters the BLD differently,

- Renamed mixedlayer_restrat_general to mxiedlayer_restrat_OM4 to better
  distinguish the two versions.

- Added function rmean2ts to extend the resetting running-mean time
  filter used in OM4 to use different time scales when growing or
  decaying. While mathematically the same in the limit of a zero
  "growing" time-scale, the implementation differs in the use of a
  reciprocal instead of division so was not added to the OM4 version.

- Updated module documentation

Co-authored-by: Abigail Bodner <abigail.bodner@gmail.com>
This patch adds the Bodner MLE testing parameters to the tc2.a test.
@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #352 (a271e3c) into dev/gfdl (fc823f5) will increase coverage by 0.16%.
The diff coverage is 76.35%.

❗ Current head a271e3c differs from pull request most recent head 8c46575. Consider uploading reports for the commit 8c46575 to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #352      +/-   ##
============================================
+ Coverage     37.07%   37.24%   +0.16%     
============================================
  Files           264      264              
  Lines         74347    74609     +262     
  Branches      13784    13838      +54     
============================================
+ Hits          27567    27790     +223     
- Misses        41684    41699      +15     
- Partials       5096     5120      +24     
Impacted Files Coverage Δ
src/core/MOM_unit_tests.F90 16.66% <0.00%> (-2.09%) ⬇️
src/core/MOM_variables.F90 46.96% <ø> (ø)
...parameterizations/vertical/MOM_diabatic_driver.F90 46.12% <20.00%> (-0.20%) ⬇️
...ameterizations/lateral/MOM_mixed_layer_restrat.F90 76.67% <79.38%> (+4.99%) ⬆️
src/core/MOM.F90 51.54% <100.00%> (+0.12%) ⬆️
...c/parameterizations/vertical/MOM_set_viscosity.F90 60.12% <100.00%> (+0.56%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@marshallward
Copy link
Member

The performance test is failing because its internal MOM_input parser doesn't support nested parameters (MLE%... %MLE), and this patch introduces a new one.

Similarly, the other regressions are likely due to the modification of the tc2.a test.

I will fix the perf test script, but for now we can just override this test (which is not actually much of a test anyway).

@marshallward
Copy link
Member

I believe that #353 will fix the issue in the Performance Monitor test.

@abodner
Copy link
Author

abodner commented Apr 21, 2023 via email

@Hallberg-NOAA
Copy link
Member

Thank you, @abodner, for this valuable and very well documented commit. We will be approving this commit and merging into dev/gfdl shortly, just as soon as we figure out the cleanest way to resolve the very minor code conflicts with the latest version of dev/gfdl and run it through our newly refurbished automated testing.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution.

Everything looks correct upon visual inspection of the code changes, apart from some minor problems with the documentation of a few the rescaled units of the new variables in the new code, but these can be corrected with a follow-up commit, perhaps while these routines are being adapted to work in fully non-Boussinesq mode.

The version of this PR with resolved conflicts has passed the pipeline testing (with the expected additions to the runtime parameters in the MOM_parameter_doc files) at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/19331

@marshallward
Copy link
Member

For somewhat boring technical reasons, the CI was re-run for this PR:

https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/19335 ✔️ 🟡

This will be handled as a merge rather than a rebase, in order to better preserve the valuable content in the PR discussion.

@marshallward marshallward merged commit 57b7a91 into NOAA-GFDL:dev/gfdl May 30, 2023
@marshallward
Copy link
Member

marshallward commented May 30, 2023

Since I don't think it was ever mentioned:

This PR is expected to fail the GitHub regression test, since the testing includes the new parameterization, which causes answer changes.

@abodner
Copy link
Author

abodner commented May 30, 2023

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants