Implementation of Zanna-Bolton-2020 equation discovery model of mesoscale momentum fluxes

[Pperezhogin]

Momentum part of equation discovery parameterization of ZB2020.

Compared to paper, new filters are introduced to improve physical and numerical properties.

Some combination of parameters:

1. EXP205. Model with backscattering.

USE_ZB2020=True
Stress_iter=1
Stress_order=2
HPF_iter=2
HPF_order=2
LPF_iter=1
LPF_order=4
amplitude=7.0

2. flux-filtered. Model mostly affecting the mean flow.

USE_ZB2020=True
Stress_iter=4
Stress_order=1
amplitude=0.75

3. ZB-ssd. Model with hyperviscosity. Affects only mean flow.

USE_ZB2020=True
ssd_iter=10
ssd_bound_coef=0.2
amplitude=0.3

[codecov]

Codecov Report

Merging #344 (3df4d90) into dev/gfdl (fc823f5) will decrease coverage by 0.12%.
The diff coverage is 9.52%.

❗ Current head 3df4d90 differs from pull request most recent head 2db0a0d. Consider uploading reports for the commit 2db0a0d to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #344      +/-   ##
============================================
- Coverage     37.07%   36.96%   -0.12%     
============================================
  Files           264      265       +1     
  Lines         74347    74641     +294     
  Branches      13784    13817      +33     
============================================
+ Hits          27567    27592      +25     
- Misses        41684    41953     +269     
  Partials       5096     5096              

Impacted Files	Coverage Δ
src/parameterizations/lateral/MOM_Zanna_Bolton.F90	8.77% <8.77%> (ø)
src/parameterizations/lateral/MOM_hor_visc.F90	50.17% <33.33%> (-0.14%)	⬇️

... and 1 file with indirect coverage changes

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

[Hallberg-NOAA]

Thank you very much for this contribution, @Pperezhogin! This looks like a very valuable contribution, and I am looking forward to trying it out in MOM6 configurations.

There are, however, several (mostly minor) things that will need to be addressed with this PR before we could merge this into MOM6.

1. This PR is failing the automated testing, as indicated by the red X at Implementation of Zanna-Bolton-2020 equation discovery model of mesoscale momentum fluxes #344. This is just due to trailing white space, but this is not something that is allowed in MOM6, as detailed at https://github.com/NOAA-GFDL/MOM6/wiki/Code-style-guide. It is, however, very easy to find this trailing white spaces using a command like grep -n ' $' *.F90. Please correct this in an updated PR.
2. As detailed in the MOM6 style guide, all arguments to routines must be described with doxygen comments, and all routines must have doxygen comments describing their purpose. These are missing for several of the new internal routines, like min_max() and compute_masks().
3. This PR contains a huge number (106) of commits. Please see the MOM6 discussion on this at Commit and Pull Request Etiquette mom-ocean/MOM6#1335. We would recommend that you consider combining all of these various commits into a single or a few commits that we want to retain in the history for debugging purposes. Each commit within a PR should be tested and be known to give reasonable answers in regression tests (for example). With a big, complicated PR, I will often do my development on one branch, and then redo it with many fewer deliberately selected commits to document the sensible (and well-tested) way points in the PR that will become a part of the official model history that we have to work through when debugging. If you want to retain the PR same name and description, you could use git push --force to update your PR in place. The other option is that we could do a squash-merge of this PR so that it appears as a single commit in the history, but that would deprive us of the possibility of putting in useful waypoints to help in any future debugging of the code and the resulting commit message that would result from the concatenation of the 106 messages in the individual commits would probably not be very readable.
4. Please see the MOM6 style guide about when array-syntax math is acceptable. In general, array syntax math should be avoided for arrays with halos. It can be a very bad idea when working on arrays with halos that might not get filled in on non-reentrant domains, and hence could contain NaNs that could get in the way when debugging, even if they do not propagate through multiplicative masks to bring down a simulation.
5. We strongly prefer the use of opaque types to prevent mis-use of code from outside of a module, and to retain greater freedom to revise a module later on. Your ZB2020_CS type is transparent (i.e., it does not have a private attribute inside of it), and its use_ZB2020 element is being used in horizontal viscosity. Please consider making this type opaque, and return the value of use_ZB2020 either as an argument to ZB_2020_init or make that initialization routine into a function that uses this value, and then store that logical in the horizontal viscosity control structure.
6. A number of the comments describing the new parameters that are being read in via get_param calls in ZB_2020_init will be very cryptic when viewed outside of the context of this module. Bear in mind that these will all be logged in parameter files, and should be somewhat understandable on their own.
7. Please do not log all of the new parameters with the new scheme when USE_ZB2020 is false. The easiest way to do this to add the argument ..., do_not_log=.not.CS%use_ZB2020 to all of the get_param_calls in ZB2020_init apart from the first one for "USE_ZB2020" itself.
8. The parameters read with get_param should have names that better reflect their usage. For example "amplitude" does not give any hint of what it is the amplitude of or what scheme it is used for. "ZB_AMPLITUDE" at least hints that this variable is used the ZB2020 scheme. (Note also that by convention, parameter names are case-sensitive and usually given in all capital letters.)
9. Please do not register any of the diagnostics from this module unless it will be used. To avoid doing so, please add a logical test for the value of CS%use_CP2020 around the register_diag_field calls on lines 122-145 or add a return statement to ZB_2020_init before these calls when it is false.
10. Please describe the units of each of the real variables in this routine in the comments where they are introduced. Many of these declarations do have descriptions of their units, but they are missing in a number of cases. If the units are arbitrary (e.g., in a linear solver) we describe them with [arbitrary].
11. Please use the MOM6 error handler via calls like call MOM_error(WARNING, ...) or call MOM_mesg(...) instead of simple Fortran write() statements. The former allows us to use verbosity controls and helps us manage potentially huge volumes of output from failing parallel runs, while the latter could tie up an entire parallel computer system if there is a message coming from every point on every PE.

Again I would like to repeat my enthusiasm for this new capability, and I hope that you will find these comments to be helpful and not too onerous to address.

[Pperezhogin]

Hi @Hallberg-NOAA!

Thanks for the detailed review! I responded to every bullet point in a separate commit, as you can see in branch PR_ZB_2020.

Also, in response to point 3, I created a separate branch with 5 commits representing main development stages for the last year: PR_ZB_2020_combined. The last commit in both branches is the same.

The functionality of the resulting code is the same as submitted via PR: Regression did not change, all tests were passed.

[marshallward]

@Pperezhogin I think it would be simpler if we closed this and reopened with your rebased branch, PR_ZB_2020_combined. Otherwise, we would have to squash-merge this one due to the large number of commits.

[marshallward]

Thanks @Pperezhogin ! I'll close this one and move discussion to the other PR.