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

+Add and use optional unscale args to reproducing_sum #767

Merged

Conversation

Hallberg-NOAA
Copy link
Member

@Hallberg-NOAA Hallberg-NOAA commented Dec 6, 2024

This PR consists of a series of 4 commits that adds optional unscale arguments to the publicly visible reproducing_sum() interfaces in MOM_coms, and then uses them to refactor the routines in MOM_sum_output and 4 other modules to eliminate about 19 dimensional rescaling factors, and to work more broadly in rescaled variables for dimensional consistency testing.

All answers are bitwise identical, but there are new optional arguments to 2 publicly visible interfaces and a new element in the transparent unit_scale_type. This commits in this PR include:

  • 193c83ec9 Use reproducing_sum with unscale in 5 places
  • 842bbf4da Work in rescaled units in write_energy
  • 4d255baf7 +Add RZL2_to_kg element to unit_scale_type
  • 9e7e16523 +Add optional unscale argument to reproducing_sum

@Hallberg-NOAA Hallberg-NOAA added the enhancement New feature or request label Dec 6, 2024
@marshallward
Copy link
Member

marshallward commented Dec 10, 2024

  1. Since the unscale * array(..) arguments also implicitly create scaled arrays as copies, you might be able to simplify the reproducible_EFP_sum_* functions with a pointer which conditionally points to these rescaled values for the same amount of work + memory usage.

    Something like

    real, intent(in), target :: array(..)
    real, pointer :: summed_array(..)
    
    if (present(unscale)) then
      allocate(summed_array, mold=array)
      summed_array(..) = unscale * array(..)
    else
      summed_array => array
    endif
    

    then eliminate unscale_ and all of the conditional logic in the solver.

  2. field_chksum is being redirected to rotated_field_chksum even though no turns argument is provided. rotated_field_chksum is also redefined as some kind of generic rescaling/rotating wrapper for all debugging operations. I am not quite comfortable with globbing these operations together. It might be more appropriate to create a separate wrapper supporting unscale, or maybe just do it in-place?

    (rotated_field_chksum appears to only be used in one specific place of MOM_restart.F90. Perhaps this function is not even necessary?)

@Hallberg-NOAA Hallberg-NOAA force-pushed the reproducing_sum_unscale branch from d6f594e to 193c83e Compare December 11, 2024 10:43
@Hallberg-NOAA
Copy link
Member Author

I have updated this PR to eliminate the changes to the rotated_field_checksum() interface. I plan to revisit this later in a separate PR, probably with a different form. This revised version also avoids using an internal variable name ending in an underscore, and it no longer contains any array-syntax math on multidimensional arrays.

  Added the new optional unscale argument to reproducing_sum() and
reproducing_sum_EFP().  When this is used, the resulting real sum is restored to
the same scaling as the input arrays, but when there is an EFP_type returned it
is the same as it would have been had the input array been rescaled before it
was passed in.  All answers are bitwise identical, but thre is a new optional
argument to a two publicly visible interfaces.
  Added the new element RZL2_to_kg to the unit_scale_type for convenience when
rescaling masses and other globally integrated variables.  All answers are
bitwise identical, but there is a new element in a transparent type.
  Revised write_energy() and accumulate_net_input()  to work more extensively in
dimensionally rescaled variables by using the new unscale arguments to the
reproducing_sum functions. As a result of these changes, 15 rescaling factors
were eliminated or moved toward the end of write_energy().  All answers are
bitwise identical.
  Use reproducing_sum with unscale to calculate the global mass diagnostic,
globally integrated ocean surface area, offline tracer transport residuals and
in calculating the OBC inflow area in tidal_bay_set_OBC_data().  This change allows
for the elimination or replacement of 6 rescaling factors and one added instance
with a consistent conversion factor and diagnostic units occur on the same line.
All answers are bitwise identical.
@Hallberg-NOAA Hallberg-NOAA force-pushed the reproducing_sum_unscale branch from 193c83e to 03ed726 Compare December 11, 2024 23:13
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.

This looks very good now. The change to reproducing_sum_2d is much better than the one I proposed, and no such change was needed to reproducing_EFP_sum_3d. (My suggestion would have probably made it worse!)

@Hallberg-NOAA
Copy link
Member Author

This PR has been tested at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/25745.

@marshallward marshallward merged commit a4d13e8 into NOAA-GFDL:dev/gfdl Dec 12, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants