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.

Sign up for GitHub

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

Jump to bottom

Add wetting and drying capability to local time-stepping scheme #6074

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Add wetting and drying capability to local time-stepping scheme #6074

merged 1 commit into from
Feb 7, 2024

Conversation

gcapodag
Copy link
Contributor

@gcapodag gcapodag commented Nov 17, 2023 •
edited by mark-petersen
Loading

This PR adds a wetting and drying capability to the local time-stepping (LTS) scheme for the barotropic ocean already available in master. The PR also adds several improvements for the LTS scheme not related to wetting and drying like for instance the upwind option for the layerThickEdgeFlux and inline tidal boundary conditions needed to run the drying slope test case from Compass. A separate PR on Compass will include drying slope, dam break, and parabolic bowl test cases for LTS.

This PR is bit-for-bit for E3SM mainline simulations, which do not include LTS or wetting and drying.

[BFB]

@mark-petersen mark-petersen added BFB PR leaves answers BFB Stealth PR has feature which, if turned on, could change climate. fka FCC labels Nov 17, 2023
cbegeman
cbegeman requested changes Nov 25, 2023
View reviewed changes
Copy link
Contributor

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

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

@gcapodag Exciting work!

I posted a few questions here because I found it hard to keep track of where you're scaling normalVelocityTend terms by wettingVelocityFactor and where you're scaling normalVelocity terms. In previous implementations of wetting and drying, we scale each of these only once (here, I guess it would be once per stage). Can you clarify for me how this is working here or point me to a design doc?

I haven't done any testing yet. Let me know when the compass LTS PR is ready.

@mark-petersen
Copy link
Contributor

@gcapodag now that the AB2 PR is merged (#5989), it is a good time to rebase these commits to the head of master. Since you merged master into this branch a few times along the way, I recommend starting with master and cherry-picking your commits. Thanks!

@xylar
Copy link
Contributor

xylar commented Nov 28, 2023

I recommend starting with master and cherry-picking your commits.

@mark-petersen, an interactive rebase in which you pick the commits you want and remove the rest from the list is an easier way to cherry-pick several commits.

@gcapodag
Copy link
Contributor Author

Thank you for your input @cbegeman , I replied to your comments, hopefully my replies make sense. As soon as MPAS-Dev/compass#716 goes in, I will submit the compass PR as well.

@gcapodag
Copy link
Contributor Author

@mark-petersen @xylar please let me know if the "rebase" has gone through. thanks!

@gcapodag
Copy link
Contributor Author

@cbegeman I might get the compass PR going so it will be possible to test this, and when the init mode changes to the drying slope will be merged, I will add those as well. How does this sound?

@cbegeman
Copy link
Contributor

@cbegeman I might get the compass PR going so it will be possible to test this, and when the init mode changes to the drying slope will be merged, I will add those as well. How does this sound?

Yes, that sounds great. Once you post the PR then I will review it along with this PR. Merging the compass PR after the E3SM PR is merged is the way to go. Thanks!

@gcapodag gcapodag mentioned this pull request Nov 29, 2023
Merged
8 tasks
@gcapodag
Copy link
Contributor Author

Testing for this PR can be done from this Compass PR: MPAS-Dev/compass#738

@mark-petersen
Copy link
Contributor

@gcapodag this code rebase looks correct, thanks. I will review this week.

@mark-petersen
Copy link
Contributor

Compiled with gnu on chicoma and intel in chrysalis. With the later, results are bfb against master branch point using nightly compass suite with default flags. That is a sanity test, as I'm not running LTS. But so far so good...

@rljacob
Copy link
Member

rljacob commented Dec 7, 2023

Needs an approval.

@mark-petersen
Copy link
Contributor

Since I last looked at this Dec 5, I started with this branch and merged in 09491ec, which is master from Dec 21 just before the stand-alone bug in #6133. I then recompiled stand-alone with intel on chrysalis, and also ran the E3SM tests:

cd cime/scripts
./create_test SMS_Ld62.T62_oQU240.GMPAS-IAF.chrysalis_intel -q acme-small --walltime 1:00:00
./create_test SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.chrysalis_gnu -q acme-small --walltime 30:00

These both pass the compile and smoke test.

@mark-petersen
Copy link
Contributor

@gcapodag thank you for the substantial testing shown in MPAS-Dev/compass#738. I will try to reproduce those tests and approve this PR soon.

@gcapodag
Copy link
Contributor Author

gcapodag commented Jan 4, 2024

@mark-petersen sounds good, thanks!

@gcapodag
Copy link
Contributor Author

@cbegeman the compass PR should be ready for you try out with this one as well

@gcapodag
Copy link
Contributor Author

@cbegeman does this PR have a green light on your end?

@cbegeman
Copy link
Contributor

@gcapodag Thanks for reminding me to approve this. I'll do it now, given that my compass tests were successful. Great work!

@gcapodag
Copy link
Contributor Author

@cbegeman thank you!

@mark-petersen
Copy link
Contributor

Tested current head and rebased locally onto master. Compiled with intel on chrysalis and gnu on chicoma, with both debug and optimized. Ran nightly compass test suite with all (that does not test this feature, of course). Both gnu and intel are bfb on all tests against master branch point. Also tested and passed E3SM

SMS_Ln9.T62_oQU240.GMPAS-IAF.chrysalis_gnu
SMS_Ln9.T62_oQU240.GMPAS-IAF.chrysalis_intel

mark-petersen
mark-petersen approved these changes Jan 29, 2024
View reviewed changes
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.

I tested with the associated compass test cases at MPAS-Dev/compass#738 (comment). Approving based on the beautiful results in these test cases, and that there is no alterations of the mainline model results for this stealth feature.

@gcapodag
Copy link
Contributor Author

Thank you very much @mark-petersen!

cbegeman added a commit to MPAS-Dev/compass that referenced this pull request Jan 30, 2024
@cbegeman
Merge pull request #738 from gcapodag/LTS_WD_PR
633a562 
Add LTS option to dam_break and parabolic_bowl test cases

This PR enhances the dam_break and parabolic_bowl test cases with the option to run using the local time-stepping (LTS) time integrator. Note that LTS only works in a single layer setting. Please also see the companion PR E3SM-Project/E3SM#6074
@mark-petersen
Copy link
Contributor

FYI for the E3SM review, the Local Time Stepping feature that is altered here is only used in MPAS-Ocean standalone for single layer simulations right now.

@rljacob
Copy link
Member

rljacob commented Feb 1, 2024

Notes: should be done next week.

@jonbob jonbob added MPAS-Ocean standalone Issues and features for standalone MPAS-Ocean code that dont impact E3SM. and removed mpas-ocean Stealth PR has feature which, if turned on, could change climate. fka FCC labels Feb 6, 2024
jonbob added a commit that referenced this pull request Feb 6, 2024
@jonbob
Merge branch 'gcapodag/mpaso/wd' into next (PR #6074)
b55a814 
Add wetting and drying capability to local time-stepping scheme

This PR adds a wetting and drying capability to the local time-stepping
(LTS) scheme for the barotropic ocean already available in master. The
PR also adds several improvements for the LTS scheme not related to
wetting and drying like for instance the upwind option for the
layerThickEdgeFlux and inline tidal boundary conditions needed to run
the drying slope test case from Compass. A separate PR on Compass will
include drying slope, dam break, and parabolic bowl test cases for LTS.

[BFB] - MPAS-Ocean standalone only
@jonbob
Copy link
Contributor

jonbob commented Feb 6, 2024

Passes:

  • ERS.ne30pg2_r05_EC30to60E2r2.GPMPAS-JRA.chrysalis_intel.mosart-rof_ocn_2way
  • ERS_Ld5.T62_oQU120.CMPASO-NYF.chrysalis_intel
  • SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel

merged to next

@jonbob jonbob merged commit 50485d0 into E3SM-Project:master Feb 7, 2024
3 checks passed
@jonbob
Copy link
Contributor

jonbob commented Feb 7, 2024

merged to master

@xylar xylar mentioned this pull request Mar 15, 2024
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbegeman cbegeman cbegeman approved these changes

@mark-petersen mark-petersen mark-petersen approved these changes

Assignees

@jonbob jonbob

Labels
BFB PR leaves answers BFB MPAS-Ocean standalone Issues and features for standalone MPAS-Ocean code that dont impact E3SM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gcapodag @mark-petersen @xylar @cbegeman @rljacob @jonbob