bugfix: static h_new shape remaining_transport_sum

[marshallward]

This patch redefines h_new to match the shape of a center-point rather
than a v-face point.

Dynamic memory builds would have been unaffected by this, and older GCC
(~7.3.0) compilers appear to have not objected to the shape mismatch,
but this raised an error in newer GCCs (~9.3.0).

This should not affect answers, since the loops never strayed into these
redundant undefined regions.

[codecov]

Codecov Report

Merging #82 (efa503b) into dev/gfdl (4640461) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           dev/gfdl      #82   +/-   ##
=========================================
  Coverage     29.05%   29.05%           
=========================================
  Files           244      244           
  Lines         71905    71905           
=========================================
  Hits          20892    20892           
  Misses        51013    51013           

Impacted Files	Coverage Δ
src/tracer/MOM_offline_main.F90	0.00% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4640461...efa503b. Read the comment docs.

[Hallberg-NOAA]

I agree with this correction.

[klindsay28]

I'm not so sure about the assertion

This should not affect answers, since the loops never strayed into these
redundant undefined regions.

h_new is declared in offline_advection_ale with dimensions (SZI_(G),SZJ_(G),SZK_(GV)) and is being passed into remaining_transport_sum where the argument's dimensions are (SZI_(G),SZJB_(G),SZK_(GV)). If SZJB_(G) and SZJ_(G) differ, then it seems like this mismatch will alter the mapping of indices to array locations in memory, whether you are in the halo regions or not.

[marshallward]

@klindsay28 I think you are right, it will change it from a zero-indexed field to a 1-indexed field (in symmetric mode), and may change answers. Thanks for catching that.

Do you have any tests which can check this? It doesn't seem to have been detected in our regression suite.

[klindsay28]

The NCAR specific test suite doesn't have any tests with OFFLINE_TRACER_MODE = True, so I wouldn't expect our tests to catch this.

h_new is used in remaining_transport_sum in the block https://github.com/NCAR/MOM6/blob/8106f48fcd224c70475095e4f9301f0f99e95413/src/tracer/MOM_offline_main.F90#L617-L623

For a test to catch this change, you would need to have h_new values such that sampling it at the wrong location would lead to a change in the conditional on line 621. I'm not familiar with this code at all, so I'm not sure how you could arrange for that to occur in a test.

[Hallberg-NOAA]

I am very confident that this PR will not impact any existing solutions. Until we accepted #37 near the end of 2021, this code would not have worked at all due to a segmentation fault, and had not worked since at least July 2019. No one appears to have noticed, which means that no one was using this part of the code.

The offline tracer mode is a useful idea, but it was never fully implemented, and never had been incorporated into any regression tests, primarily because the postdoc who was working on it accepted a well-deserved new position abroad (working with a different ocean modeling system) before it was fully complete. As of this year, it is now compiling, running, reproducing across layouts, and passing dimensional consistency tests, but this bug suggests that it is not being fully tested in all modes (e.g., symmetric or non-symmetric memory).

[marshallward]

I've updated the commit log to indicate that answer changes are likely (assuming anyone used this, at least).

[Hallberg-NOAA]

This has passed TC testing and pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/14928.