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 ocean option for Redi kappa to be set equal to GM (AMOC PR 3/6) #4904

Merged
merged 3 commits into from
Jun 2, 2022

Conversation

mark-petersen
Copy link
Contributor

@mark-petersen mark-petersen commented Apr 20, 2022

add new option: config_Redi_closure = 'equalGM', sets RediKappa equal to gmBolusKappa everywhere. Default remains config_Redi_closure = 'constant'.

[BFB]

@mark-petersen
Copy link
Contributor Author

This PR is a recreation of #4867. I was not able to open that one back up.

Note that this PR base is currently the #4864 branch, because it builds on that, but so we just see the one commit relevant to this PR. I will change the base to master after #4864 is merged.

@mark-petersen mark-petersen force-pushed the mark-petersen/ocn/redi-closure-equal-gm branch from 826ff40 to c1ea7b9 Compare April 21, 2022 19:42
@mark-petersen mark-petersen force-pushed the darincomeau/ocn/gm-horizontal-resolution-function branch 2 times, most recently from 888ea12 to 0996cfa Compare April 21, 2022 20:17
@mark-petersen mark-petersen force-pushed the mark-petersen/ocn/redi-closure-equal-gm branch from c1ea7b9 to 7f609b2 Compare April 21, 2022 20:17
@vanroekel
Copy link
Contributor

code looks fine here, just needs the transplant of the little bit of equalGM code from the 2/6 PR.

@darincomeau
Copy link
Member

5 year CRYO1850.ne30pg2_ECwISC30to60E2r1 tests:

Baseline, Visbeck GM

This PR:
GM Visbeck, Redi kappa = GM kappa

config_GM_closure = 'Visbeck'
config_Redi_closure = 'equalGM'

@mark-petersen mark-petersen force-pushed the mark-petersen/ocn/redi-closure-equal-gm branch from 7f609b2 to d2b96ca Compare April 25, 2022 18:03
@mark-petersen
Copy link
Contributor Author

Rebased after #4864 changes so this merges smoothly, plus addition of extra line mentioned by @vanroekel above. Passes nightly regression suite with gnu on badger with

+config_GM_closure = 'Visbeck'
+config_Redi_closure = 'equalGM'
+config_GM_horizontal_taper = 'RossbyRadius'
+config_Redi_horizontal_taper = 'RossbyRadius'

and passes

SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.anvil_gnu

Copy link
Member

@darincomeau darincomeau 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 my 5 year tests.

@mark-petersen mark-petersen force-pushed the mark-petersen/ocn/redi-closure-equal-gm branch from d2b96ca to 8c2fafc Compare April 27, 2022 21:17
@xylar xylar force-pushed the darincomeau/ocn/gm-horizontal-resolution-function branch from 7bc8a33 to 24859cf Compare May 11, 2022 18:58
Base automatically changed from darincomeau/ocn/gm-horizontal-resolution-function to master May 25, 2022 18:12
@mark-petersen mark-petersen force-pushed the mark-petersen/ocn/redi-closure-equal-gm branch from 8c2fafc to 142ee46 Compare May 25, 2022 19:56
@milenaveneziani
Copy link
Contributor

This should be ready to go, after @vanroekel's approval, right?

@mark-petersen
Copy link
Contributor Author

Yes. @vanroekel please review.

I tested with gnu debug on badger with nightly test suite, passes all with default flags; then again with config_Redi_closure = 'equalGM' as the only change, then tested again with these flags

config_GM_closure = 'Visbeck'
config_Redi_closure = 'equalGM'
config_GM_horizontal_taper = 'RossbyRadius'
config_Redi_horizontal_taper = 'RossbyRadius'

and it passes all tests.

Copy link
Contributor

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

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

so one thing @milenaveneziani noted that should be made clearer I think is this capability as implemented doesn't work for all schemes. I think only Visbeck and constant will work. E&G won't as it uses a 3-D varying array so RediKappa can't copy that. Putting comments in the registry would be sufficient for now

description="Control what type of function is used for Redi $\kappa$."
possible_values="'constant', 'data'"
description="Control what type of function is used for Redi $\kappa$. For 'equalGM', RediKappa is set to gmBolusKappa, so picks up the closure used by GM."
possible_values="'constant', 'equalGM', 'data'"
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be good to say this only works for 2-d schemes (e.g. constant or visbeck)? This won't work for edenGreatbatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I added that comment above.

@milenaveneziani
Copy link
Contributor

Good point @vanroekel. I had forgotten about that. I agree that a comment in the Registry would be great at this point.

Copy link
Contributor

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for the extra comment @mark-petersen

@mark-petersen
Copy link
Contributor Author

Passes:

SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.chrysalis_intel
SMS_Ln9.T62_EC30to60E2r2.GMPAS-IAF.chrysalis_gnu

@jonbob jonbob added Stealth PR has feature which, if turned on, could change climate. fka FCC and removed Do not merge NML labels May 31, 2022
@jonbob
Copy link
Contributor

jonbob commented May 31, 2022

test merge passes:

  • SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel
  • SMS_D_Ld1.ne30pg2_r05_EC30to60E2r2.WCYCL1850.chrysalis_intel
  • PEM_Ln9.ne30pg2_EC30to60E2r2.WCYCL1850.chrysalis_intel
  • ERS.ne11_oQU240.WCYCL1850NS.chrysalis_intel

This PR should be good to go once the repo is open

jonbob added a commit that referenced this pull request Jun 1, 2022
…4904)

Add ocean option for Redi kappa to be set equal to GM

Brings in a new option to config_Redi_closure, 'equalGM', that sets
RediKappa equal to gmBolusKappa everywhere.
Default remains config_Redi_closure = 'constant'.

[BFB]
@jonbob
Copy link
Contributor

jonbob commented Jun 1, 2022

merged to next

@jonbob jonbob merged commit 5f6b107 into master Jun 2, 2022
@jonbob
Copy link
Contributor

jonbob commented Jun 2, 2022

merged to master

@jonbob jonbob deleted the mark-petersen/ocn/redi-closure-equal-gm branch June 2, 2022 16:31
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 Stealth PR has feature which, if turned on, could change climate. fka FCC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants