-
Notifications
You must be signed in to change notification settings - Fork 368
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 horizontal tapering function to mesoscale eddy parameterizations (AMOC PR 2/6) #4864
Add horizontal tapering function to mesoscale eddy parameterizations (AMOC PR 2/6) #4864
Conversation
@darincomeau -- I'll run the scripts that automatically update the bld files to reflect Registry changes for this PR and make sure it's all consistent, unless that's what you've already done. |
@jonbob I haven't - the bld file changes were added manually. |
Thanks for getting back to me so quickly @darincomeau. I'll run the scripts and update the files if anything turns up |
@darincomeau - this PR looks like it also includes the changes from PR #4835? That PR has not been merged to the repo yet... |
5 year tests with ECwISC30to60E2r1 SOwISC12to60E2r4 |
Passes MPAS-O nightly regression suite with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes look consistent with what I implemented previously so approving on visual inspection and final approval from @jonbob on the supporting changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, based on my testing and E3SM simulations by others.
c744e64
to
e7f21ab
Compare
Rebased after #4835 was merged to master. |
e7f21ab
to
c744e64
Compare
That works for me. |
@milenaveneziani, could you make that name change, too? (It needs to happen in various places.) |
@xylar: sure. Is that OK if I do it manually though? |
Yeah, do it manually. @jonbob will check our work in any case. |
ok, done. @mark-petersen: you can go ahead and use this branch for the BFB tests. And please do double-check my changes. |
*(dcEdge(iEdge) - config_eddying_resolution_ramp_min) & | ||
/(config_eddying_resolution_ramp_max - config_eddying_resolution_ramp_min) | ||
*(dcEdge(iEdge) - config_Redi_horizontal_ramp_min) & | ||
/(config_Redi_horizontal_ramp_max - config_Redi_horizontal_ramp_min) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixed the BFB mismatch. I had to put this block of code in the same order as before, with the operations in the same order as before. Now it is BFB in E3SM tests (it was BFB in stand-alone tests before, which is partly what took me so long to find it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay, @mark-petersen, that's great!
Now passes compare against branch point 4435bfa for:
@jonbob please proceed with your tests. |
I also tested stand-alone and E3SM with the new flag settings for this PR
in E3SM, passes
in stand-alone nightly suite with all three: |
Any other comments on this one, or is it ready for final testing before the merge? @xylar you still have a red on the approval, can you approve if you agree? |
@mark-petersen, as I mentioned (somewhere) above, my latest test is behaving as expected. The taper looks consistent with the Rossby Radius size, and everything worked without further problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@milenaveneziani did a ton of work to address my concerns so thanks for the coding and testing!
I'm also happy with all the changes. Thanks for the great discussion and thorough testing. I'll give you my second green check! |
The automated scripts found no discrepancies in the bld files |
… next (PR #4864) Add horizontal tapering function to mesoscale eddy parameterizations This PR adds a new 'RossbyRadius' option to the horizontal tapering of GM and Redi, which is based on Hallberg (2013) - https://doi.org/10.1016/j.ocemod.2013.08.007, which turns the parameterization on if the Rossby Radius is resolved by at least two cells. These flags are: * config_Redi_horizontal_taper = 'ramp' (def), 'none', 'RossbyRadius' * config_GM_horizontal_taper = 'ramp' (def), 'none', 'RossbyRadius' This PR replaces the variable `gmResolutionTaper`, which was used for both GM and Redi, to the separate variables `RediHorizontalTaper` and `gmHorizontalTaper` so that Redi and GM tapering can be chosen separately, and it is clear in the code. Logic was added to the GM and Redi schemes so that the there is a base value in the arrays for Redi Kappa and GM Kappa. The base value is then multiplied by the taper array, which has a value between 0 and 1. Arrays that are static in time are computed on init. Under the following conditions, the arrays are recomputed at every time step: * config_Redi_horizontal_taper = 'RossbyRadius' (recompute RediHorizontalTaper and RediKappa) * config_GM_horizontal_taper = 'RossbyRadius' (recompute gmHorizontalTaper) [NML] adds MPAS-Ocean namelist options [BFB]
Passes:
with expected NML DIFFs. Merged to next |
Thanks @jonbob. Assuming all goes well, I'm sure everyone in E3SM land will be happy to have this thread silenced (at only 155 comments). |
@darincomeau - the SMS_PS.northamericax4v1pg2_WC14to60E2r3.WCYCL1850.allactive-wcprodrrm prod test failed on all platforms due to the user_nl_mpaso settings in the testmod definition:
I assume we mostly need to replace "config_eddying_resolution_taper" with "config_Redi_horizontal_taper" and "config_GM_horizontal_taper"? |
@jonbob yes, I agree. The |
…nto next (PR #4864) Re-merge to fix testdefs issue causing failures
re-merged to next with testmods update |
merged to master and expected NML DIFFs blessed |
This PR adds a new 'RossbyRadius' option to the horizontal tapering of GM and Redi, which is based on Hallberg (2013) - https://doi.org/10.1016/j.ocemod.2013.08.007, which turns the parameterization on if the Rossby Radius is resolved by at least two cells. These flags are:
This PR replaces the variable
gmResolutionTaper
, which was used for both GM and Redi, to the separate variablesRediHorizontalTaper
andgmHorizontalTaper
so that Redi and GM tapering can be chosen separately, and it is clear in the code. Logic was added to the GM and Redi schemes so that the there is a base value in the arrays for Redi Kappa and GM Kappa. The base value is then multiplied by the taper array, which has a value between 0 and 1. Arrays that are static in time are computed on init. Under the following conditions, the arrays are recomputed at every time step:[NML] adds MPAS-Ocean namelist options
[BFB]