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

ALE sponge mask_z halo fixes #1495

Merged
merged 4 commits into from
Sep 13, 2021

Conversation

marshallward
Copy link
Collaborator

In the ALE sponge, the mask_u and mask_v masks are constructed from
mask_z, but also require valid halo data on their E/W or N/S
boundaries.

Although a pass_var function is called on mask_z before computing
these masks, this function will not populate the halos of mask_z if
there is no adjacent data, e.g. a non-reentrant boundary or a
land-masked tile.

And even though mask_z was initialized to zero, this was undone by the
internal dellocation/reallocation of the array inside of
horiz_interp_and_extrap_tracer (although the actual result appears to
be compiler dependent).

There are two major changes in this patch:

  • The FMS-based horiz_interp_and_extrap_tracer function no longer does
    a deallocate/reallocate of its output arrays, and now simply assumes
    they are unallocated.

    The output arrays are also explicitly declared as intent(out).

    This change clarifies that only the compute domains of mask_z and
    associated fields are updated, although it doesn't fully resolve the
    issue described above.

  • The ALE sponge code now explicitly initializes the halo values of
    mask_z before interpolating the mask_u and mask_v masks.

    This ensures that mask_[uv] boundary values are disabled on
    points where no halo data is available (and hence no halo updates from
    pass_var. When the data is available, sensible values will replace
    these zeros.

These changes prevent anomalous values of mask_z from entering the
halos, and ensuring that mask_[uv] contain sensible values.

A similar operation should not be required by the tracer fields, since
the zero-halo values in the mask will correctly disable these values
when no adjacent field is available for halo updates.

In the ALE sponge, the `mask_u` and `mask_v` masks are constructed from
`mask_z`, but also require valid halo data on their E/W or N/S
boundaries.

Although a `pass_var` function is called on `mask_z` before computing
these masks, this function will not populate the halos of `mask_z` if
there is no adjacent data, e.g. a non-reentrant boundary or a
land-masked tile.

And even though `mask_z` was initialized to zero, this was undone by the
internal dellocation/reallocation of the array inside of
`horiz_interp_and_extrap_tracer` (although the actual result appears to
be compiler dependent).

There are two major changes in this patch:

* The FMS-based `horiz_interp_and_extrap_tracer` function no longer does
  a deallocate/reallocate of its output arrays, and now simply assumes
  they are unallocated.

  The output arrays are also explicitly declared as intent(out).

  This change clarifies that only the compute domains of `mask_z` and
  associated fields are updated, although it doesn't fully resolve the
  issue described above.

* The ALE sponge code now explicitly initializes the halo values of
  mask_z before interpolating the mask_u and mask_v masks.

  This ensures that `mask_[uv]` boundary values are disabled on
  points where no halo data is available (and hence no halo updates from
  `pass_var`.  When the data is available, sensible values will replace
  these zeros.

These changes prevent anomalous values of mask_z from entering the
halos, and ensuring that `mask_[uv]` contain sensible values.

A similar operation should not be required by the tracer fields, since
the zero-halo values in the mask will correctly disable these values
when no adjacent field is available for halo updates.
@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #1495 (d87fa88) into dev/gfdl (1623085) will increase coverage by 0.00%.
The diff coverage is 22.22%.

❗ Current head d87fa88 differs from pull request most recent head e05ceb2. Consider uploading reports for the commit e05ceb2 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           dev/gfdl    #1495   +/-   ##
=========================================
  Coverage     29.02%   29.03%           
=========================================
  Files           236      236           
  Lines         71515    71491   -24     
=========================================
- Hits          20759    20756    -3     
+ Misses        50756    50735   -21     
Impacted Files Coverage Δ
src/parameterizations/vertical/MOM_ALE_sponge.F90 15.37% <12.50%> (+0.06%) ⬆️
src/framework/MOM_horizontal_regridding.F90 37.37% <100.00%> (+1.09%) ⬆️
src/initialization/MOM_state_initialization.F90 20.71% <0.00%> (-0.17%) ⬇️

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 1623085...e05ceb2. Read the comment docs.

@marshallward
Copy link
Collaborator Author

I suppose this works for sp_val_[uv] because we are using the mask_[uv] == 1 checks, but if we were to replace this with the probably-faster mask_[uv] * sp_val_[uv] then we'd have to do something about sp_val_[uv] on those edge halos. I'm not sure how that would look.

@marshallward
Copy link
Collaborator Author

It looks like the sp_val_[uv] halo values are an issue if I turn on aggressive initialization.

Even though these values never get used when mask_[uv] is zero, the compiler still forbids their assignment with an uninitialized value.

We may want to first build mask_[uv] and then, depending on the value, conditionally evaluate sp_val_[uv].

marshallward and others added 2 commits September 4, 2021 16:58
Currently, fields from `horiz_interp_and_extrap_tracer` are interpolated
onto [uv] points under the assumption that halos have sensible values.
This is generally true due to the preceding `pass_var` call, but it may
not be valid if the halo is not updated, e.g. along a boundary or
land-masked tile.

The mask is designed to be set to zero when this happens, but it could
still result in an assignment of an uninitialized value to the fields
(`sp_val_[uv])`.  Although the value is not used, it can produce
compiler warnings and errors under strict debugging builds.

This patch moves the calculation of `sp_val_[uv]` so that it is only
conditionally computed when the mask is set to 1.
Copy link
Collaborator

@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.

These changes all make sense to me, and are very well explained in the comments describing this PR.

@Hallberg-NOAA
Copy link
Collaborator

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/13581.

@Hallberg-NOAA Hallberg-NOAA merged commit 87aad26 into mom-ocean:dev/gfdl Sep 13, 2021
@marshallward marshallward deleted the ale_sponge_halo_fix branch October 20, 2021 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants