Remapping refactor and fix for mismatched ocean depths #662

adcroft · 2024-05-30T20:03:16Z

This a significant refactor of the core remapping algorithm, with a patch at the end that fixes a bug for some situations mostly encountered when initializing from observations.

Commits include:

Cleaning up compiler warnings in MOM_remapping.F90
Added stand alone drivers for timing and unit tests of MOM_remapping.F90
Re-factor of unit_tests() in MOM_remapping.F90 to use helper class making cleaner/shorter
New tests of parts of remap_via_sub_cells() algorithm, added as remap_via_sub_cells() was deconstructed into three functions: intersect_src_tgt_grids(), remap_src_to_sub_grid(), and remap_sub_to_tgt_grid()
The fix for reconstructions in top/bottom layers implemented as an alternative version of the remap_src_to_sub_grid() function (not an flag or optional argument)
- Nine runtime parameters were added. This is necessary to confirm/assess which bits of the model are affected by the bug fix. While initialization is hypothesized as the main area of affect, the remapping of EBT structure, and diagnostics are places we should expect to see non-trivial changes also.

The refactor is in preparation to adding a suite of new reconstructions and new remapping options including conservative remapping of transport.

config_src/drivers/timing_tests/time_MOM_remapping.F90

src/ALE/MOM_remapping.F90

Hallberg-NOAA

I have read through this code in its entirety. This extensive revision to the MOM_remapping code is exceptionally well documented and tested, and I understand and agree with what it is doing.

I have added a handful of specific minor comments that I think should be considered before this is accepted, but this is very close to being ready to go.

- Addressed compiler warnings about unused variables and argument intent. - Note there is one dummy argument that was unused and the best way to address the warnings was to remove the argument, i.e. and API change. This was only in a debugging/utility function so does not impact the rest of the model. - Address warnings about unused/uninitialized variables in MOM_remapping.F90 - Sets values that appear to the compiler to be potentially unset, but always are in practice

- Added new driver, time_MOM_remapping, to config_src/timing_tests/ that exercises the remapping_core_h() function with PCM and PLM. - We should add other reconstruction schemes too, but the main reason for adding this now is to monitor the impact of refactoring the function remapping_core_h() which is mostly independent of the reconstruction schemes. - Fixed .testing/Makefile to not fail with `make build.timing -j`

Added a simple driver for the remapping unit tests.

- Added a simple class within MOM_remapping.F90 to greatly abbreviate the lines comparing values of arrays. - Translated the existing remapping unit tests to use the testing class.

- The first block of remap_via_sub_cells() computes the intersection between two columns (grids). I've split out this block into a stand alone function so that we can re-use it in a to-be-written analog of remap_via_sub_cells() that will remap integrated quantities. - Added some in-line documentation of algorithm used by remap_via_sub_cells() - Added new column-wise function intersect_src_tgt_grids() - Added tests of intersect_src_tgt_grids() that check against known results - Had to add a new helper function to MOM_remapping to report state of integer arrays.

- This second phase of remap_via_sub_cells() maps the values from the source grid to the sub-grid using the pre-calculated arrays/indices from intersect_src_tgt_grids(). This phase as-is is only used for remapping of intensive variables but it does implicitly calculate intermediate extensive quantities which are analogous to what we need when remapping momentum. Splitting this phase out primarily allows us to test this bit of code and it has indeed revealed an edge case that fails. - Added new column-wise function remap_src_to_sub_grid() - Added tests of remap_src_to_sub_grid() that check against known results - One test is commented out corresponding to an edge case that reveals the current code is wrong. Will enable this test after checking how many experiments are impacted but the associated bug fix.

- We had debugging left over from development a decade ago that was retained "just in case" but has been obsoleted by the unit testing. - Also cleaned up unused variables.

- Added remap_sub_to_tgt_grid() to handle remapping of intermediate state on sub-layers to the target grid. This function will be reused by alternate remap_via_subcells() analogs for extensive quantities. - Added unit tests for remap_sub_to_tgt_grid().

@claireyung

- The original remap_via_subcells() assumed that the first and last sub-layers would be vanished and thus needed to only assign the edge values of the source reconstructions to the sub-layers. However, this is only a valid assumption (and correct) if the total column thickness of source/target are the same. That is generally true doing the main loop. However, when initializing from WOA, the ocean model depth and source-data depth often differ, and this assumption that we can ignore the top/bottom reconstructions breaks. - The original fix was developed with @claireyung and took the form claireyung@924b7ac In this patch, I have unrolled the loop inside remap_src_to_sub_grid() to avoid adding an additional `if` test on the loop index. - The fix is implemented in remap_src_to_sub_grid() and the original method is reached by setting a logical inside the remapping_CS. Default is to use the OM4 alorithm (original method with bug). - New runtime parameters are added in to recover original algorithm selectively: - OBC_REMAPPING_USE_OM4_SUBCELLS, - Z_INIT_REMAPPING_USE_OM4_SUBCELLS, - EBT_REMAPPING_USE_OM4_SUBCELLS, - SPONGE_REMAPPING_USE_OM4_SUBCELLS, - SPONGE_REMAPPING_USE_OM4_SUBCELLS, - DIAG_REMAPPING_USE_OM4_SUBCELLS, - NDIFF_REMAPPING_USE_OM4_SUBCELLS, - HBD_REMAPPING_USE_OM4_SUBCELLS, - INTWAVE_REMAPPING_USE_OM4_SUBCELLS, and - REMAPPING_USE_OM4_SUBCELLS all of which default to True. - No answer changes.

adcroft · 2024-06-21T16:26:40Z

I've re-written the commits to address above previous review comments. The latest revision also added one more run-time flag to control remapping within internal tides, which I missed in the original PR. This was caught by the MacOS regressions - we don't understand why the non-MacOS tests did not catch this.

marshallward · 2024-06-21T20:03:51Z

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

…_h() This commit addresses a `\todo` added in NOAA-GFDL#662, namely that the functions remap_via_sub_cells() and remap_via_sub_cells_om4() could, and should, be moved up into remapping_core_h(), as well as into the obsolete remapping_core_w(). In that PR, the function remap_via_sub_cells() had been modularized into three other functions, and the near-duplicatem remap_via_sub_cells_om4(), called the two of the same functions as well as an out-of-date version of the third function. Since the reampping_core_h() function calls is already brief, calling one function in addition to remap_via_sub_cells(), and since remap_via_sub_cells() is only called from remapping_core_*(), we can remove a level of call-tree depth depth by copying remap_via_sub_grid() into reampping_core_h() and removing the unused function(s).

…_h() This commit addresses a `\todo` added in #662, namely that the functions remap_via_sub_cells() and remap_via_sub_cells_om4() could, and should, be moved up into remapping_core_h(), as well as into the obsolete remapping_core_w(). In that PR, the function remap_via_sub_cells() had been modularized into three other functions, and the near-duplicatem remap_via_sub_cells_om4(), called the two of the same functions as well as an out-of-date version of the third function. Since the reampping_core_h() function calls is already brief, calling one function in addition to remap_via_sub_cells(), and since remap_via_sub_cells() is only called from remapping_core_*(), we can remove a level of call-tree depth depth by copying remap_via_sub_grid() into reampping_core_h() and removing the unused function(s).

Hallberg-NOAA reviewed May 31, 2024

View reviewed changes

config_src/drivers/timing_tests/time_MOM_remapping.F90 Outdated Show resolved Hide resolved