Internal field index rotation

[marshallward]

This patch introduces an index rotation feature applied to all internal
fields. When this feature is enabled, model fields are initialized on
the index map as specified by the configuration or input files. The
values of these arrays are then moved to a rotated index map and saved
to a new rotated array, which are then used for model simulation.

The primary purpose of this feature is to detect regressions in
rotationally symmetric calculations, such as fluid dynamic dynamic
processes and other transport phenomena.

Fields created or read by the drivers are unrotated and are handled as
external user-defined field. External fields are rotated or unrotated
as they pass between the driver and the ocean model.

In order to maintain bit reproducibility, all field initialization is
computed or assigned on the input grid.

This patch also includes an extension of the test suite, which applies a
single quarter-turn to each of the existing tests.

The features of this patch are summarized below.

Two new configuration features are added:

• ROTATE_INDEX: Boolean flag which enables this feature.
• INDEX_TURNS: Integer which descibes the number of counterclockwise
  quarter-turns. Default value is 1.

Two new modules and several new functions are added:

• MOM_array_transform: Fundamental operations for rotating arrays,
  vectors, rotationally equivalent pairs

• MOM_transform_FMS: Wrappers to FMS operations which can only operate
  on the unrotated index pax.

• Numerous rotation functions have been introduced in various modules
  associated with initialization (forcing, OBC, etc), which are
  documented in the source code.

The following new features have been added to existing components:

• paired checksums (uvchksum, hchksum_pair) now include a scalar_pair
  argument to distinguish between rotationally equivalent scalars (e.g.
  grid areas on u- and v-points) and vector components.

• register_restart_pair was introduced to register rotationally
  associated pairs.

• turns has been added to the horizontal index (HI) type, as a means
  to track the number of quarter turns in lower level operations.

• allocate_forcing_type and allocate_mech_forcing have been extended
  to permit allocation based on an exisiting (unrotated) reference,
  alongside the older interface using control flags for each field type.

• copy_MOM_domain includes a turns argument, to support rotational
  changes to the FMS domain.

Reproducibility:

• All results should be bitwise reproducible. A nonphysical exception
  exception is the calculation of CFL conditions in systems with zero
  velocity, such as the unit test case. The previous implementation
  could produce a "negative zero" CFL, whose sign would depend on the
  number of rotations.

  The new implementation eliminates these negative zeros, but may also
  change existing CFL calculations, and may report a regression in
  ocean.stats.

• Rotated diagnostic checksum output is not tested, since there is not
  yet a way to associated rotationally equivalent diagnostics. See
  below for more information.

Current limitations:

• Only single PE runs can be rotated.

  Currently FMS assumes a specific ordering for assignment of PE IDs
  (east->west, south->north). An index rotation will typically
  re-orient these domains in a way which breaks this default
  assignment. This ID is used to manage MPI message passing
  targets, so any rearrangement of these IDs will cause errors.

  Since we currently cannot control the PE ID assigned by FMS, we are
  currently unable to support index rotation of parallel runs.

• Certain fields and tracers cannot be rotated.

  There are a small subset of tracers and fields in various
  structures, such as boundary fluxes, which are currently
  unsupported. This is primarily beacuse these tracers are typically
  managed at the driver level.

  There is no reason why these fields cannot be supported, but it
  would require a larger test suite to detect and handle such fields.

• Diagnostic output is not rotated.

  Diagnostics are computed on the rotated grid, but it would require
  additional FMS wrappers to support registering and writing
  diagnostics on the input index map. This is not yet implemented,
  so all diagnostics are on the rotated index map.

  One additional consequence is that there is no way to register pairs
  of diagnostics, and thus no way to automate the chksum.diag output.
  Currently, diagnostic checksums are disabled in the test suite.

  We hope to resolve this issue in a future patch.

• Open boundary condition (OBC) rotation is restriced to INDEX_TURNS=1

  Generalized OBC rotation is currently not supported. This is
  primarily due to the sensitivity of segment configuration, which is
  specified with a large number of flags denoting its orientation.

  It is possible to support generalized rotation, but it will require
  a refactor of the OBC segment code.

  We hope to support generalized rotation in a future patch.

• Incorrect definitions of gridLonT and gridLatT

  Due to certain array length assumptions for 1d axes, we rotate and
  exchange the values of gridLatT and gridLonT during quarter-turn
  operations.

  This is not correct, since these arrays describe latitude and
  longitude, rather than the axes in the first and second dimension.
  However, several other components currently expect the first and
  second reference axis to have matching array lengths, so for now we
  swap these arrays during rotations.

  The 1d arrays are only used for initialization computation (which is
  on the input map) and I/O management, so this error does not affect
  any existing runs. But rotated runs which save these arrays will be
  incorrect.

• Non-ALE sponge rotation is disabled.

  Only ALE sponge rotation is currently supported.

  It should be possible to extend rotation to include non-ALE sponges
  if needed.

Further comments:

• Fields are not rotated in-place, since many calculations require that
  both fields exist at any time. While it is unlikely that rotated and
  unrotated fields would persist for an entire run, there is likely to
  be an increase in memory overhead.

• External fields from the drivers are rotated and unrotated at the
  beginning and end of every timestep, which can create additional
  computational overhead.

  Note that there are control flags, e.g. cycle_start and cycle_end
  which could be used to manage these operations, assuming that the
  driver does not change the fields inbetween coupler steps.

• This patch fixes what appears to have been an error in the global
  h-point indexes of OBC segments. However, it also seems that these
  indices were unused, so this is unlikely to impact existing runs.

This implementation was proposed by Robert Hallberg and follows an
earlier implementation by Nic Hannah, whose efforts introduced several
changes that implemented the rotational invariance of existing numerical
calculations.

[codecov-io]

Codecov Report

Merging #1088 into dev/gfdl will decrease coverage by 0.44%.
The diff coverage is 29.70%.

@@             Coverage Diff              @@
##           dev/gfdl    #1088      +/-   ##
============================================
- Coverage     46.26%   45.81%   -0.45%     
============================================
  Files           213      215       +2     
  Lines         67954    69130    +1176     
============================================
+ Hits          31436    31671     +235     
- Misses        36518    37459     +941     

Impacted Files	Coverage Δ
src/core/MOM_transcribe_grid.F90	36.32% <0.00%> (-9.09%)	⬇️
src/core/MOM_variables.F90	57.77% <0.00%> (-19.45%)	⬇️
src/diagnostics/MOM_debugging.F90	13.29% <ø> (ø)
src/framework/MOM_array_transform.F90	0.00% <0.00%> (ø)
src/framework/MOM_dyn_horgrid.F90	84.37% <ø> (ø)
src/parameterizations/vertical/MOM_ALE_sponge.F90	26.28% <0.00%> (-4.30%)	⬇️
src/framework/MOM_hor_index.F90	25.39% <2.70%> (-30.16%)	⬇️
src/core/MOM_forcing_type.F90	62.79% <10.40%> (-8.11%)	⬇️
src/core/MOM_open_boundary.F90	31.27% <25.52%> (-1.49%)	⬇️
src/core/MOM.F90	66.33% <39.09%> (-3.41%)	⬇️
... and 22 more

---

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 3e9c645...f632ffc. Read the comment docs.

[marshallward]

Gaea regression test: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/pipelines/10178

Regression has passed (excluding parameters).

[Hallberg-NOAA]

The use of array syntax is generally discouraged in MOM6, as described in the MOM6 style guide (see https://github.com/NOAA-GFDL/MOM6/wiki/Code-style-guide#array-syntax). In this case, there is a certain compactness to this copy as it avoids the need for an explicit temporary array, but I think that it would be preferable to write this line as A(:,:) = A(n:1:-1, :) to make it more explicit that array syntax is being used.

[adcroft]

Agreed. Adding A(:,:) = A(n:1:-1,:) works better than adding HI to these interfaces and since these operations are over the entire array the syntax works well. It's the partial loop ranges that are indecipherable using array syntax.

[Hallberg-NOAA]

This should be rewritten as A(:,:) = A_in(:,:) to make it more explicit what is going on. Please see the comment a few lines below for an expanded discussion of this point.

[marshallward]

Agreed, I will overhaul this entire file.

[Hallberg-NOAA]

Perhaps this would be even more elegant and robust to user silliness if this line were to read qturns = modulo(turns, 4).

[marshallward]

I can add it, but I will say that values outside 0-3 are handled by rotate_array and allocate_rotated_array, so it's probably not necessary in this case.

In fact, I think I have too many modulo() calls in the higher level functions, and would ideally only like to handle the modulo at the lowest level.

But happy to add it if you think it will help.

[Hallberg-NOAA]

It probably doesn't matter too much either way since this is not performance critical code, but since there is an extra allocate and a set of copies, the modulo is probably a cheaper alternative and it would avoid unnecessarily bloating the memory of the model (which might actually matter). However, if there is already a modulo on the number of turns in the calling code above this, then it does not help. I would defer to your judgement, @marshallward, on the right approach here.

[marshallward]

Yes, you are right.. I missed that qturns = 4 will do the allocate. Yeah, in that case I will apply the change.

[Hallberg-NOAA]

If there were an award for the cleanest major pull request with the best documentation, this PR would win it hands-down. This set of changes was masterfully executed and wonderfully explained, and I am happy to approve it.

I have a couple of comments at specific points in the MOM_array_transform.F90 code about the use of array syntax (generally don't use it, but be very explicit that it is being used when there is a compelling reason to use it), and some suggested improvements by replacing turns with modulo(turns, 4). I am happy to approve and merge these changes as is, or after these few minor points have been addressed.

This PR also raises some other topics for discussion after it is merged, including:

• The use of pointer, intent(inout) which seems like a good idea that should be used more widely if it is legal, especially if pointer, intent(in) is legal.
• Reconsidering the use of the vardesc type. We had been phasing it out, but with the new paired restart calls it makes a lot of sense to retain it.
• What is the right balance between code compactness (e.g., the use of ; to group logically connected assigments) versus stylistic alignment (e.g., closing a subroutine argument list with a single line ) aligned with the opening ( ), or whether this is a point where a bit of individual style should be encouraged.

[marshallward]

This should have said "post_data expect data to be on the rotated index map...

[marshallward]

Since these dummy arrays are 1-indexed by default, use of lbound and ubound is not adding any value and potentially causing confusion, so I will rewrite this and the other loops as 1-to-n.

[marshallward]

I've pushed an updated with your suggested changes, as well as my own described above, thanks @Hallberg-NOAA.

I agree that vardesc still seems to have some value. I personally prefer passing structs of metadata over functions with lots of arguments, but can defer this discussion.

Thanks also for indulging my whitespace indentations. Let's schedule a whitespace battle royale at some point.

[marshallward]

Updated Gaea regression test: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/pipelines/10188

Updated regression has passed.

[adcroft]

Changes order of parameters in MOM_parameter_doc files so will handle manually.

[adcroft]

I'm holding this up briefly since moving initialization calls around moved some parameter documentation into the wrong module blocks. A small PR (#1090) is incoming that should precede both this and #1089.

No changes needed on this PR.

[Hallberg-NOAA]

Well, I managed to proactively mess this up! I already updated dev/gfdl for both MOM6 and MOM6-examples after I saw your approval, and I pushed it before I saw the new small PR to disentangle this. Sorry about that. - Bob Hallberg Dr. Robert Hallberg Oceanographer NOAA Geophysical Fluid Dynamics Laboratory NOAA GFDL || Phone: (609) 452-6508 Princeton University Forrestal Campus || Cell: (732) 599-0459 201 Forrestal Road || Fax: (609) 987-5063 Princeton, New Jersey 08540-6649 || Email: Robert.Hallberg@noaa.gov

…

On Wed, Apr 15, 2020 at 6:56 PM Alistair Adcroft ***@***.***> wrote: ***@***.**** commented on this pull request. I'm holding this up briefly since moving initialization calls around moved some parameter documentation into the wrong module blocks. A small PR ( #1090 <NOAA-GFDL#1090>) is incoming that should precede both this and #1089 <NOAA-GFDL#1089>. No changes needed on this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <NOAA-GFDL#1088 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABR2BFK22KRHEB4KTYREFPLRMY3SLANCNFSM4MH722GQ> .

[adcroft]

Indeed, there are now conflicts that I had set things up to handle more smoothly. Misbehave again and I'll remove your push privileges. 😁

FYI, you did not record the MOM6 commits in the MOM6-examples commit message. I'll add them when I next update MOM6-examples.

[nichannah]

Well done Marshall!

[MJHarrison-GFDL]

Indeed!

…

On Thu, Apr 16, 2020 at 8:37 PM Nic Hannah ***@***.***> wrote: Well done Marshall! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <NOAA-GFDL#1088 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSPZ4HHR6RV3UASL5I6SI3RM6QCPANCNFSM4MH722GQ> .