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

+Make h_neglect argument mandatory in 28 routines #700

Closed

Conversation

Hallberg-NOAA
Copy link
Member

Made the h_neglect argument non-optional to 28 routines, because there is no way to provide a consistent hard-coded default for a dimensional parameter. The routines where this change was made include remapping_core_h(), remapping_core_w(), build_reconstructions_1d(), P1M_interpolation(), P3M_interpolation(), P3M_boundary_extrapolation(), PLM_reconstruction(), PLM_boundary_extrapolation(), PPM_reconstruction(), PPM_limiter_standard(), PPM_boundary_extrapolation(), PQM_reconstruction(), PQM_limiter(), PQM_boundary_extrapolation_v1(), build_hycom1_column(), build_rho_column(), bound_edge_values(), edge_values_explicit_h4(), edge_values_explicit_h4cw(), edge_values_implicit_h4(), edge_slopes_implicit_h3(), edge_slopes_implicit_h5(), edge_values_implicit_h6(), regridding_set_ppolys(), build_and_interpolate_grid(), remapByProjection(), remapByDeltaZ(), and integrateReconOnInterval().

In those cases that also have an optional h_neglect_edge argument, the default is now set to h_neglect. Improperly hard-coded dimensional default values for h_neglect or h_neglect_edge were eliminated in 15 places as a result of this change, but they were added back in 3 unit testing routines (one of these is archaic and unused) that do not use exercise dimensional rescaling tests.

Because these previously optional arguments were already present in all calls from the dev/gfdl or main branches of the MOM6 code, all answers are bitwise identical in the regression tests, but this change in interfaces could impact other code that is not in the main branch of MOM6.

These changes were discussed and agreed to at the consortium wide MOM6 dev call on July 29, 2024.

  Made the h_neglect argument non-optional to 28 routines, because there is no
way to provide a consistent hard-coded default for a dimensional parameter.  The
routines where this change was made include remapping_core_h, remapping_core_w,
build_reconstructions_1d, P1M_interpolation, P3M_interpolation,
P3M_boundary_extrapolation, PLM_reconstruction, PLM_boundary_extrapolation,
PPM_reconstruction, PPM_limiter_standard, PPM_boundary_extrapolation,
PQM_reconstruction, PQM_limiter, PQM_boundary_extrapolation_v1,
build_hycom1_column, build_rho_column, bound_edge_values,
edge_values_explicit_h4, edge_values_explicit_h4cw, edge_values_implicit_h4,
edge_slopes_implicit_h3, edge_slopes_implicit_h5, edge_values_implicit_h6,
regridding_set_ppolys, build_and_interpolate_grid, remapByProjection,
remapByDeltaZ, and integrateReconOnInterval.

  In those cases that also have an optional h_neglect_edge argument the default
is now set to h_neglect.  Improperly hard-coded dimensional default values for
h_neglect or h_neglect_edge were eliminated in 15 places as a result of this
change, but they were added back in 2 unit testing routines (one of these is
archaic and unused) that do not use exercise dimensional rescaling tests.

  Because these previously optional arguments were already present in all calls
from the dev/gfdl or main branches of the MOM6 code, all answers are bitwise
identical in the regression tests, but this change in interfaces could impact
other code that is not in the main branch of MOM6.
@adcroft
Copy link
Member

adcroft commented Jul 31, 2024

This is at odds with the refactor I am working on. Many of the new schemes do not need an h_neglect so I have made it a property of the individual remapping scheme (class) that is set when it is constructed/initialized. While the existing functions underneath remapping_core_h() could have mandatory h_neglect, those functions will be legacy and hopefully phased out quickly. remapping_core_h() should not have a mandatory h_neglect for when it is calling the new classes. The fact that it is optional currently allows us (me) to gracefully use either approach during development.

@Hallberg-NOAA
Copy link
Member Author

The problem with having an optional h_neglect with a hard-coded dimensional default value (that is actually used) is that it breaks our dimensional consistency testing. As a result, h_neglect is currently being passed in to every routine where it is being made non-optional. If there is another way to eliminate the hard-coded dimensional default values, I am open to considering alternative approaches to do so.

@adcroft
Copy link
Member

adcroft commented Aug 1, 2024

I'll try cherry-picking some commits to update your branch to avoid the h_neglect arguments all together

@adcroft
Copy link
Member

adcroft commented Aug 2, 2024

I made a patch to your branch at Hallberg-NOAA#13 that fixes what I object to. You might have to clean things up - I did this in a rush (which still took too many hours)

@Hallberg-NOAA
Copy link
Member Author

I am withdrawing this PR because we have another substantially revised PR (#705 ) that accomplishes much the same purpose in eliminating a number of unusable hard-coded dimensional defaults, while also addressing the concerns that @adcroft raised with this PR.

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.

2 participants