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 REMAPPING_USE_OM4_SUBCELLS the default #758

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

awallcraft
Copy link

In addition to REMAPPING_USE_OM4_SUBCELLS, for ALE remapping, there are several parameters of the form XXX_REMAPPING_USE_OM4_SUBCELLS, where XXX identifies the target, and they all currently default to True.

To simplify setting them all to False, which is recommended, the defaults for the XXX versions is changed to the value of REMAPPING_USE_OM4_SUBCELLS.

Answers are only changed if REMAPPING_USE_OM4_SUBCELLS is set to False and the default (now False) is used for one or more of the other parameters. In such cases the original behaviour can be recovered by explicitly setting the other parameters to True.

In addition to REMAPPING_USE_OM4_SUBCELLS, for ALE remapping, there are
several parameters of the form XXX_REMAPPING_USE_OM4_SUBCELLS, where
XXX identifies the target, and they all currently default to True.

To simplify setting them all to False, which is recommended, the defaults
for the XXX versions is changed to the value of REMAPPING_USE_OM4_SUBCELLS.

Answers are only changed if REMAPPING_USE_OM4_SUBCELLS is set to False
and the default (now False) is used for one or more of the other parameters.
In such cases the original behaviour can be recovered by explicitly
setting the other parameters to True.
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 16.06%. Comparing base (7a9adbc) to head (2bc0de7).
Report is 4 commits behind head on dev/gfdl.

Files with missing lines Patch % Lines
src/core/MOM_open_boundary.F90 0.00% 2 Missing ⚠️
src/framework/MOM_diag_mediator.F90 0.00% 2 Missing ⚠️
...eterizations/lateral/MOM_lateral_mixing_coeffs.F90 0.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (7a9adbc) and HEAD (2bc0de7). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (7a9adbc) HEAD (2bc0de7)
2 1
Additional details and impacted files
@@              Coverage Diff              @@
##           dev/gfdl     #758       +/-   ##
=============================================
- Coverage     36.67%   16.06%   -20.62%     
=============================================
  Files           278      118      -160     
  Lines         84269    30498    -53771     
  Branches      15869     5602    -10267     
=============================================
- Hits          30908     4898    -26010     
+ Misses        47532    25147    -22385     
+ Partials       5829      453     -5376     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@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 systematic changes to the defaults of subsidiary remapping parameters to follow an overall parameter is a very good idea, and I think that these changes will eventually be accepted exactly as they are written. However, this PR should be included in the set of changes with the previously agreed upon default changes that we will be putting in as a separate targeted PR to main, and it should only be merged into dev/gfdl when we are ready to handle those default changes.

Copy link
Member

@adcroft adcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - defaulting these parameters to a main parameter will be a lot less work for everyone.

@adcroft
Copy link
Member

adcroft commented Dec 4, 2024

@adcroft adcroft merged commit ac6e43d into NOAA-GFDL:dev/gfdl Dec 4, 2024
12 checks passed
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