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 MLD_out to diagnose MLD #832

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

theresa-cordero
Copy link

For COBALTv3 we want to save the mixed layer depth from diagnoseMLDbyDensityDifference or diagnoseMLDbyEnergy. This PR adds an optional argument to pass the MLD out of these routines.

This should have no impact on answers or require changes in other parts of the code.

CC: @yichengt900

@MJHarrison-GFDL MJHarrison-GFDL self-assigned this Feb 10, 2025
@MJHarrison-GFDL
Copy link

This look good.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

These changes adding an optional argument to two publicly visible routines look to me like they are reasonable. The only question that I would have is why the new arrays are intent(inout), given that the entire array (including halo regions) are being set in the array-syntax copy?

In diagnoseMLDbyDensityDifference(), MLD is only being set in the range of MLD(is:ie,js:je), so the whole-array copy into MLD_out runs the risk if putting uninitialized values (perphas NaNs) into the halo regions. I would suggest only copying in the valid values to MLD_out and leaving it as intent(inout).

In diagnoseMLDbyEnergy(), MLD is initialized to 0 at about line 335, so the whole array copy should be fine, and MLD_out could be made intent(out). Alternately, MLD_out could be left as intent(inout), but then only the computational domain points (that are actually calculated in this routine) should be copied over.

@theresa-cordero
Copy link
Author

Thanks Bob. I ended up making MLD_out intent(out), initializing MLD_out to zero, and only copying the computational domain. This seems the most consistent with the intention of the argument.

I am not sure why MLD is initialized to zero twice (line 331 and 338) in diagnoseMLDbyEnergy() but not in diagnoseMLDbyDensityDifference(). That predates my changes to these routines but at least one could probably be removed.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

I agree that this revised PR will fully work as intended.

Theresa Morrison and others added 3 commits February 19, 2025 15:40
Add an optional argument to pass the MLD out of the diagnose MLD
routine.
Includes three changes:
- Make MLD_out optional
- initalize MLD_out to 0. everywhere
- copy MLD to MLD_out only in the computational domain.
Remove an extra initalization of MLD in diagnoseMLDbyEnergy
@Hallberg-NOAA
Copy link
Member

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/26447.

@Hallberg-NOAA Hallberg-NOAA merged commit 1344e7c into NOAA-GFDL:dev/gfdl Feb 19, 2025
10 checks passed
@theresa-cordero
Copy link
Author

@yichengt900 The final version here is different from the version on our dev/cefi branch, we can update now or the next time we pull in dev/gfdl.

@yichengt900
Copy link

@theresa-cordero , thanks! I have merged this change back to dev/cefi MOM6!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants