Clean-up ocean wetting and drying routine

[cbegeman]

Clean-up the wetting and drying routine in MPAS-Ocean and make a few bugfixes in that routine.

This PR utilizes a new variable wettingVelocityFactor, which replaces wettingVelocity, to scale both normalVelocity and normalVelocityTend in instances where the layerThickness is approaching a minimum layerThickness.

We preserve the existing option where config_zero_drying_velocity = True, zeroing out normalVelocity and normalVelocityTend using wettingVelocityFactor in regions where the divergence out of a cell would bring the layerThickness within a tolerance of the minimum thickness.

We remove an approach for setting wettingVelocityFactor between 0 and 1 when config_zero_drying_velocity = False because this code was never operational. Instead when config_zero_drying_velocity = False there is no velocity or tendency modification in drying cells.

Bugfix 1 is changing the time step used in the wetting and drying routine to the RK4 substep. Bugfix 2 is changing range over which wettingVelocityFactor is computed to active connected edges.

[BFB]

[cbegeman]

This PR is migrated from E3SM-Ocean-Discussion#14.

This commit was added since that review 26f525c. With this commit, wetting and drying tests are BFB compared to the version without this commit.

[cbegeman]

This PR was found to be BFB on chrysalis with intel, open-mpi compiler for the nightly test suite. It is non-BFB for the wetdry test suite due to bugfix 1.

[xylar]

@sbrus89, would you be willing to do some tests to make sure things still behave as expected, following up on the testing we did in E3SM-Ocean-Discussion#14?

I'll do some "do no harm" testing in compass to make sure we're good there.

@jonbob, would you be up for running a couple of relevant E3SM tests to make sure we're BFB as expected. Or feel free to recommend some tests for me to run instead.

[sbrus89]

Yes, I'll do that.

[xylar]

I looked over the code and it looks fine to me. My review is mostly from a do-no-harm perspective, as I am not super well versed on the details of the wetting-and-drying code.

The compass pr test suite was BFB on Chrysalis with Intel and OpenMPI.

[rljacob]

@sbrus89 please review.

[cbegeman]

Through conversations with @sbrus89, we realized that the change from rk_substep_weight to dt 4329f78 is best thought of not as a bugfix but rather a change that makes the wetting and drying routine more conservative (more likely to use wettingVelocityFactor as a limiter on the velocity).

In my drying_slope and dam_break tests, I did not find that 4329f78 made a difference to the solution. @sbrus89 is exploring whether dropping this commit makes a difference for resolution convergence in his new parabolic bowl test with wetting and drying.

We also discussed dropping this line in the code,

E3SM/components/mpas-ocean/src/shared/mpas_ocn_wetting_drying.F

Line 366 in 26f525c

layerThickness = min(layerThicknessProvis(k, iCell), layerThicknessCur(k, iCell))

in an attempt to improve the parabolic bowl convergence, which would also make the wetting and drying routine less conservative.

@sbrus89 is planning on reporting back on the outcome of these tests before approving the PR. Please let me know if I represented our conversation accurately!

[rljacob]

@sbrus89 still needs your input.

[sbrus89]

Thanks for the reminder, @rljacob. I'm working on testing now.

[sbrus89]

@cbegeman, After rebasing locally on E3SM/master (0f8188) and testing inside compass (3a4bef) on compy, the drying slope cases run and give good results. However, I'm seeing failures with both dam break cases.

[cbegeman]

@sbrus89 Did the dam break case pass on master? I just ran with E3SM/master at 09498b6 and that fails on chrysalis, intel, openmpi with validate state errors.

[sbrus89]

@cbegeman - I did not check, but yes I would suspect that is the case. I can run compass bisect to track down the commit that introduced the errors.

[cbegeman]

@sbrus89 It must have been relatively recently. I don't recall how long ago I tested dam break for this PR (E3SM-Ocean-Discussion#14 was around for a while), but the dam break case was definitely working when we were testing #5279.

[sbrus89]

I agree, it couldn't have been from too long ago.

[cbegeman]

@sbrus89 If you have time and interest in running git bisect that would be much appreciated. Let me know if you want to pass this off to me.

[sbrus89]

sure, I'll get it set up today and let you know how it goes.

[sbrus89]

@cbegeman - Here's what I found out after running the compass bisect utility:

627683e64a5d01e24c1468d589bbcfa98a0c30e2 is the first bad commit
commit 627683e64a5d01e24c1468d589bbcfa98a0c30e2
Merge: 146732cb27 d0b11efec9
Author: Jon Wolfe <[email protected]>
Date:   Mon Dec 5 14:34:11 2022 -0600

    Merge branch 'cbegeman/ocn/mask-time-series-stats-vars' (PR #5325)
    
    Mask ocean time series stats variables
    
    This PR applies fill value masking to non-restart time series stats
    streams in the ocean component. It also specifies the masking array for
    a subset of variables in the Registry.
    
    Fixes #5253
    
    [BFB]

[xylar]

Man, lesson learned! If you don't do proper CF masking from the beginning, you really will have trouble adding it later. Hopefully, we can build this into Omega from the ground up!

[cbegeman]

@sbrus89 Thanks for the testing. Why am I not surprised it was that PR! I'm figuring out the minimum changes needed for dam break to pass. I should have an answer soon.

[cbegeman]

@sbrus89 You can run the dam_break test case and verify that the solution still agrees just as well with the data after this PR. This is the commit that you need cbegeman@39192f6. I don't like this solution though, we might need some change in how tracers are treated in cases where their tendencies are disabled. I'd like to open a separate issue documenting this problem and address it separately from this PR, since the issue isn't directly associated with this PR. Does that sound good to you?

[sbrus89]

Sure, no problem. I'll report back.

[cbegeman]

@sbrus89 I posted a PR here #5519 that provides the bugfix without needing to revert fill-value masking.

[sbrus89]

@cbegeman, great! Reverting the fill value worked, but I'll test with your new PR instead. Seems like that's a better option.

[sbrus89]

I verified this works with #5519 for the dam break.

[sbrus89]

@cbegeman, this is great. thanks!

[cbegeman]

@jonbob This should be ready for E3SM testing. I think that this doesn't fit the definition of a stealth PR: "BFB code change that includes a new feature, turned off by default" because this PR does not introduce the stealth feature (wetting and drying). However, I'm uncertain about the intention of "includes a new feature" here.

[jonbob]

@cbegeman -- at this point E3SM doesn't even have the wetting and drying namelist, so I think it's safe to bring is as something that only impacts the standalone model

[jonbob]

passes:

• SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel
• SMS_D_Ld1.ne30pg2_EC30to60E2r2.WCYCL1850.chrysalis_intel.allactive-wcprod
• ERS.ne11_oQU240.WCYCL1850NS.chrysalis_intel

merged to next

[cbegeman]

@jonbob Thanks for the testing!

[jonbob]

merged to master