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

Move Regridding and Remapping Out of step_MOM_thermo #761

Open
wants to merge 3 commits into
base: dev/gfdl
Choose a base branch
from

Conversation

theresa-cordero
Copy link

@theresa-cordero theresa-cordero commented Nov 29, 2024

This PR will move the regridding and remapping routines out of the step_MOM_thermo. This is for two reasons: (1) these routines are not related to stepping the thermodynamics and (2) this change will allow a separate timestep for regridding and remapping.

These changes were mostly straightforward but some routines in step_MOM_thermo after the ALE block were rearranged and moved into another new routine.

These code changes did not change answers in the Baltic 0.25 degree example case. They should be tested in layer mode and with particles before these changes are accepted. It also did not change the diagnostics saved in my Baltic 0.25 degree example, but diagnostics should also be checked carefully.

Commit:
Move regridding, remapping, and subsequent processes into a new routine ALE_regridding_and_remapping which is called immediately after step_MOM_thermo.

Clean up inputs and local variables in step_MOM_thermo by removing those only used for regridding and remapping.

Routines that need to be done after step_thermo, regridding, and remapping, regardless of use_ALE or not into another routine since they do not make sense in ALE_regridding_and_remapping.

Answers and diagnostics have not changed in Baltic case in ALE mode. Testing still needs to be done layer mode and with particles.

@awallcraft
Copy link

The ALE code isn't completely consistent in its terminology, but the process of calculating a new vertical grid, what you are calling gridgen, is always referred to as regrid. This is followed by a remap from the old to the new grid. So I suggest renaming ALE_gridgen_and_remapping as step_ALE_regrid_and_remap or perhaps step_MOM_ALE to be consistent with the rest of the routine names.

I don't think update_tracers_after_ALE should mention ALE, I suggest step_MOM_tracer_cleanup.

The wiki contains a Code Style Guide and it says:

All subroutines, functions, arguments, and elements of public types should be described in with dOxygen comments.

If you look at other subroutines you can see that the comments can be long or short, but they must exist.

@theresa-cordero theresa-cordero changed the title Move Grid Generation and Remapping Out of step_MOM_thermo Move Regridding and Remapping Out of step_MOM_thermo Dec 3, 2024
@theresa-cordero
Copy link
Author

Thanks for you comments Alan! I am working on some additional changes to address these comments and others raised in person.

We all agree: (1) we are continuing to use "regridding" not "grid generation", (2) "update_tracers_after_ALE" is not a clear name that reflects what is in that routine, but it makes sense for now to have that as a separate routine rather than duplicated.

@theresa-cordero theresa-cordero marked this pull request as ready for review February 3, 2025 16:53
@Hallberg-NOAA Hallberg-NOAA force-pushed the split_remapping_from_stepthermo branch from f2d5dab to 1b76e35 Compare February 16, 2025 12:42
@Hallberg-NOAA
Copy link
Member

I have used these changes in the MOM6-examples regression suite, and the answers are identical. I will be happy to approve this PR once the trailing spaces have been removed, a missing callTree_leave() call has been added, and some doxygen comment syntax has been corrected.

@Hallberg-NOAA Hallberg-NOAA added the refactor Code cleanup with no changes in functionality or results label Feb 16, 2025
Copy link

codecov bot commented Feb 26, 2025

Codecov Report

Attention: Patch coverage is 61.06195% with 44 lines in your changes missing coverage. Please review.

Project coverage is 38.10%. Comparing base (93067d0) to head (a627c78).
Report is 14 commits behind head on dev/gfdl.

Files with missing lines Patch % Lines
src/core/MOM.F90 61.06% 21 Missing and 23 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           dev/gfdl     #761      +/-   ##
============================================
- Coverage     38.13%   38.10%   -0.03%     
============================================
  Files           297      298       +1     
  Lines         87579    87744     +165     
  Branches      16407    16457      +50     
============================================
+ Hits          33398    33438      +40     
- Misses        48167    48269     +102     
- Partials       6014     6037      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Thank you for this refactoring of MOM.F90, which I am very happy to approve now that the very minor issues with the previous version have been addressed.

Theresa Morrison and others added 3 commits February 26, 2025 13:52
Move grid generation, remapping, and subsequent processes into a new
routine ALE_gridgen_and_remapping which is called immediately after
step_MOM_thermo.

Clean up inputs and local variables in step_MOM_thermo by removing
those only used for gid generation and remapping.

Routines that need to be done after step_thermo, grid generation
and remapping, regardless of use_ALE or not into another routine
since they do not make sense in ALE_gridgen_and_remapping.

Answers and diagnostics have not changed in Baltic case in ALE mode.
Testing still needs to be done layer mode and with particles.
@Hallberg-NOAA Hallberg-NOAA force-pushed the split_remapping_from_stepthermo branch from a627c78 to 4cce2e3 Compare February 26, 2025 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code cleanup with no changes in functionality or results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants