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

Clean-up wetting and drying routines #14

Closed
wants to merge 7 commits into from
Closed

Clean-up wetting and drying routines #14

wants to merge 7 commits into from

Conversation

cbegeman
Copy link
Collaborator

@cbegeman cbegeman commented Mar 1, 2022 •
edited
Loading

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. We intend to add a different approach for setting wettingVelocityFactor between 0 and 1 in a subsequent PR.

We add a ramping function for wettingVelocityFactor between 0 and 1 following O'Dea et al. (2020) that is a function of how close the total column thickness is to the thin film thickness. To turn on this feature, set config_zero_drying_velocity_ramp = .true. and config_use_wetting_drying = .true..

@cbegeman cbegeman marked this pull request as draft March 1, 2022 21:46
@cbegeman
Copy link
Collaborator Author

cbegeman commented Mar 1, 2022

@sbrus89, @dengwirda, @xylar I'm posting this draft of changes made to wetting and drying routines while our discussion today was still fresh. This has not been tested but will be when the wetting-and-drying test cases are available.

@cbegeman cbegeman changed the title Cleanup wetting and drying routines Clean-up wetting and drying routines Mar 1, 2022
@cbegeman cbegeman force-pushed the ocn/cleanup-wetting-drying branch 2 times, most recently from 8e41a23 to 0268bd8 Compare April 4, 2022 17:27
@cbegeman cbegeman marked this pull request as ready for review April 5, 2022 20:46
@cbegeman
Copy link
Collaborator Author

cbegeman commented Apr 5, 2022

This PR passes the compass PR test suite on anvil with intel compilers.

@cbegeman cbegeman requested review from xylar and dengwirda April 5, 2022 21:36
@cbegeman
Copy link
Collaborator Author

cbegeman commented Apr 5, 2022

@sbrus89 I had difficulty adding you as a reviewer but it would be great to get your feedback.

xylar
xylar reviewed Apr 11, 2022
View reviewed changes
Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

@cbegeman, this is great, much cleaner!

Just 2 small fixes to the code.

I ran the pr test suite on Anvil with Intel and Intel-MPI. I used E3SM master as a baseline and compared with a test merge of this branch with master. All tests passed, including validation with the baseline.

Let me know if you'd like me to run other tests.

I think this is ready to move to an E3SM PR as soon as @sbrus89 and @dengwirda have had a look.

components/mpas-ocean/src/Registry.xml Outdated Show resolved Hide resolved
components/mpas-ocean/src/shared/mpas_ocn_tendency.F Outdated Show resolved Hide resolved
@xylar
Copy link
Collaborator

xylar commented Apr 11, 2022 •
edited
Loading

@sbrus89, I have tried to invite you to E3SM-Ocean-Discussion a couple of times but it doesn't look like you've accepted. Maybe you're not getting the invitations?

Joining the repo as a collaborator is a necessary first step to making you a reviewer here.

@sbrus89
Copy link
Collaborator

sbrus89 commented Apr 11, 2022

@xylar, sorry about that. I just joined.

@xylar xylar requested review from sbrus89 and removed request for xylar April 11, 2022 12:46
@cbegeman
Copy link
Collaborator Author

@xylar Thanks for catching those! I fixed up the problem commit.

@cbegeman cbegeman force-pushed the ocn/cleanup-wetting-drying branch from 8e0a7f6 to f6ade09 Compare May 11, 2022 15:13
@cbegeman
Copy link
Collaborator Author

As we discussed, I have added the ramp function for the damping factor on normalVelocity and normalVelocityTend to this PR (f6ade09). This is all still in the wetting and drying module and should not affect standard E3SM runs.

@cbegeman
Copy link
Collaborator Author

@sbrus89 and @xylar Just a reminder to review this PR when you have a chance. Thanks!

xylar
xylar approved these changes Jul 13, 2022
View reviewed changes
Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

Sorry, I lost track of this one. Perhaps @sbrus89 has, too. This looks good.

components/mpas-ocean/src/Registry.xml Outdated Show resolved Hide resolved
components/mpas-ocean/src/shared/mpas_ocn_wetting_drying.F Outdated Show resolved Hide resolved
@cbegeman cbegeman force-pushed the ocn/cleanup-wetting-drying branch 2 times, most recently from 98078ab to ec28546 Compare August 24, 2022 15:53
@cbegeman
Copy link
Collaborator Author

cbegeman commented Oct 6, 2022

@sbrus89 Do you want to look this over so we can open up the PR in E3SM soon?

@sbrus89
Copy link
Collaborator

sbrus89 commented Oct 6, 2022

@cbegeman - yes, I can do that. Would you mind rebasing to clear up the diffs?

@cbegeman
Copy link
Collaborator Author

cbegeman commented Oct 6, 2022

@xylar Could you bring E3SM-Ocean-Discussion/master up to E3SM/master when you get a chance? Thanks!

@xylar xylar changed the base branch from master to alternate October 7, 2022 05:28
@xylar xylar changed the base branch from alternate to master October 7, 2022 05:28
@xylar
Copy link
Collaborator

xylar commented Oct 7, 2022

@cbegeman, done!

@cbegeman
Copy link
Collaborator Author

cbegeman commented Oct 7, 2022

@sbrus89 Ready for your review

@sbrus89
Copy link
Collaborator

sbrus89 commented Oct 8, 2022

Thank you!

@cbegeman
Copy link
Collaborator Author

cbegeman commented Nov 14, 2022 •
edited
Loading

Hi all, I've retested this since the dam break fixes and it is still BFB with master (plus the dam break fix). @sbrus89 and @dengwirda, this is ready for review. Thanks!

MPAS-Dev/compass#451
E3SM-Project#5279

sbrus89
sbrus89 approved these changes Dec 6, 2022
View reviewed changes
Copy link
Collaborator

@sbrus89 sbrus89 left a comment

Choose a reason for hiding this comment

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

@cbegeman - I've finished testing this. While I didn't see it was BFB with master, the differences are very small. I assume this is due to the change in 8ff26b8. I apologize for this taking me so long.

@cbegeman
Copy link
Collaborator Author

@dengwirda How did your testing go? I don't think we need extensive testing from you at this point as it will undergo further testing when it is migrated to the main E3SM repo.

@cbegeman cbegeman removed the request for review from dengwirda January 25, 2023 21:21
@cbegeman
Copy link
Collaborator Author

@dengwirda I'm migrating this PR to E3SM. You are welcome to give feedback on it there.

@xylar
Copy link
Collaborator

xylar commented Jan 25, 2023

Yes, I agree. This has been here long enough and needs to move over to E3SM.

@cbegeman cbegeman mentioned this pull request Jan 26, 2023
Merged
@cbegeman
Copy link
Collaborator Author

Close in favor of E3SM-Project#5418

@cbegeman cbegeman closed this Jan 30, 2023
mark-petersen pushed a commit that referenced this pull request May 31, 2023
@ambrad
Crusher: Work around tcmalloc issue.
3791fab 
cee/15.0.0 with GPU MPI buffers can crash in a system lib like this:

#4  0x00007fffe159e35b in (anonymous namespace)::do_free_with_callback(void*, void (*)(void*)) [clone .constprop.0] () from /opt/cray/pe/cce/15.0.0/cce/x86_64/lib/libtcmalloc_minimal.so.1
#5  0x00007fffe15a8f16 in tc_free () from /opt/cray/pe/cce/15.0.0/cce/x86_64/lib/libtcmalloc_minimal.so.1
#6  0x00007fffe99c2bcd in _dlerror_run () from /lib64/libdl.so.2
#7  0x00007fffe99c2481 in dlopen@@GLIBC_2.2.5 () from /lib64/libdl.so.2
#8  0x00007fffea7bce42 in _ad_cray_lock_init () from /opt/cray/pe/lib64/libmpi_cray.so.12
#9  0x00007fffed7eb37a in call_init.part () from /lib64/ld-linux-x86-64.so.2
#10 0x00007fffed7eb496 in _dl_init () from /lib64/ld-linux-x86-64.so.2
#11 0x00007fffed7dc58a in _dl_start_user () from /lib64/ld-linux-x86-64.so.2
#12 0x0000000000000001 in ?? ()
#13 0x00007fffffff42e7 in ?? ()
#14 0x0000000000000000 in ?? ()

Work around this by using cee/14.0.3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xylar xylar xylar approved these changes

@sbrus89 sbrus89 sbrus89 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cbegeman @xylar @sbrus89