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

+(*)Revise remapping of diagnostics #564

Merged
merged 4 commits into from
Mar 12, 2024

Conversation

Hallberg-NOAA
Copy link
Member

The series of 3 commits in this pull request revise how the remapping of diagnostics are handled, including correcting bugs that lead to segmentation faults when global indexing is used and working in depth (z-) space with for some remapping of non-Boussinesq diagnostics. This required the reordering of the halo updates for temperatures, salinities and thicknesses during the initialization in MOM.F90 and the subsequent calculation of derived thermodynamic quantities so that they are available for remapping diagnostics.

For REMAPPING_ANSWER_DATES later than 20240201, diag_remap_updated() does an explicit sum to determine the total water column thickness, rather than using sum function, which is indeterminate of the order of the sums. For some compilers, this could change the vertical grids used for remapping diagnostics at roundoff, but no such change was detected for any of the compilers used with the MOM6 regression test suite.

The interface to diag_mediator_init() has been revised and simplified, with a thermo_var_ptrs argument replacing three arguments with the same information in the previous interface.

All answers and diagnostics are identical in Boussinesq mode without global indexing, but some remapped diagnostics are changed (by not using expressions that depend on the Boussinesq reference density) in the non-Boussinesq mode. There are altered or augmented public arguments to four publicly visible routines.

The commits in this PR include:

  • b83f4b82a +(*)Use z-space remapping for some diagnostics
  • 589e91435 +(*)Refactor MOM_diag_remap for global indices
  • 7b239aa7c Earlier initialization thickness halo updates

@Hallberg-NOAA Hallberg-NOAA added bug Something isn't working enhancement New feature or request labels Feb 8, 2024
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.14%. Comparing base (0bd4c16) to head (dbf9014).

Additional details and impacted files
@@             Coverage Diff              @@
##           dev/gfdl     #564      +/-   ##
============================================
- Coverage     37.16%   37.14%   -0.03%     
============================================
  Files           271      271              
  Lines         80660    80734      +74     
  Branches      15048    15061      +13     
============================================
+ Hits          29981    29986       +5     
- Misses        45105    45151      +46     
- Partials       5574     5597      +23     

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

Copy link
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

A major improvement over the complicated index arithmetic that I put in ages ago, never mind also fixing global indexing! Other fixes also look good as well.

I added one suggestion but it's very minor and does not need to hold up the PR.

src/framework/MOM_diag_mediator.F90 Show resolved Hide resolved
@marshallward
Copy link
Member

Oh, there is apparently one conflict that needs addressing...

  Moved the post-initialization halo updates for thicknesses and temperatures
and salinities in initialize_MOM to occur immediately after the last point where
they are modified and before the remapped diagnostic grids are set up.  This
does not change any existing answers but it will enable the future use of the
thermo_var_ptr type in calls to thickness_to_dz when setting up remapped
diagnostics, and perhaps elsewhere in the initialization of other auxiliary
variables.  All answers are bitwise identical.
  Refactored MOM_diag_remap to work with global indices and to move logical
branches outside of do loops.  This was done by adding internal routines that
set the loop indices consistently with the NE c-grid convention used throughout
the MOM6 code and converting the optionally associated mask pointer into an
optional argument.  It also simplifies the logic of many of the expressions
within the remapping code.  There is also a new element, Z_based_coord, in the
diag_remap_ctrl type, to indicate whether the remapping is working in thickness
or height units, but for now it is always set to false.

  The function set_h_neglect or set_dz_neglect is used to set the negligible
thicknesses used for remapping in diag_remap_update and diag_remap_do_remap,
depending on whether the remapping is being done in thickness or vertical height
coordinates.  Diag_remap_init has a new vertical_grid_type argument, and
diag_remap_do_remap has a new unit_scale_type argument.

  For REMAPPING_ANSWER_DATES later than 20240201, diag_remap_updated does an
explicit sum to determine the total water column thickness, rather than using
sum function, which is indeterminate of the order of the sums.  For some
compilers, this could change the vertical grids used for remapping diagnostics
at roundoff, but no such change was detected for any of the compilers used with
the MOM6 regression test suite.

  All answers and diagnostics in cases that worked before are bitwise identical,
but there are new arguments to two publicly visible interfaces.
  Do remapping in Z-space for some remapped diagnostics, depending on which
coordinate is used.  The subroutine thickness_to_dz is used with the
thermo_vars_type to do the rescaling properly in non-Boussinesq mode.

  A new thermo_var_ptrs argument was added to diag_mediator_init, replacing
three other arguments that had the same information, and its call from MOM.F90
was modified accordingly.

  The various calls to the diag_remap routines from post_data_3d and
diag_update_remap_grids were modified depending on whether a z-unit or h-unit
vertical grid is being remapped to.

  All answers and diagnostics are identical in Boussinesq mode, but some
remapped diagnostics are changed (by not using expressions that depend on the
Boussinesq reference density) in the non-Boussinesq mode.  There are altered
or augmented public arguments to two publicly visible routines.
@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/22576 ✔️

@marshallward marshallward merged commit d311b1d into NOAA-GFDL:dev/gfdl Mar 12, 2024
12 checks passed
@Hallberg-NOAA Hallberg-NOAA deleted the diag_remap_in_Z branch May 10, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants