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 units argument mandatory for get_param_real #300

Merged

Conversation

Hallberg-NOAA
Copy link
Member

@Hallberg-NOAA Hallberg-NOAA commented Jan 5, 2023

This commit includes changes to the get_param_real and log_param_real interfaces to make the units arguments mandatory. It also adds an optional unscale argument to the log_param_real interfaces. Without other changes in the previous commits, it will cause the MOM6 code to fail to compile. However, by itself this commit does not change any answers or output.

@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #300 (5a46646) into dev/gfdl (1f0c92f) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 5a46646 differs from pull request most recent head 145fbe8. Consider uploading reports for the commit 145fbe8 to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #300      +/-   ##
============================================
- Coverage     38.22%   38.22%   -0.01%     
============================================
  Files           269      269              
  Lines         76359    76359              
  Branches      14025    14025              
============================================
- Hits          29190    29189       -1     
- Misses        41929    41931       +2     
+ Partials       5240     5239       -1     
Impacted Files Coverage Δ
src/framework/MOM_file_parser.F90 90.57% <ø> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Hallberg-NOAA
Copy link
Member Author

This PR has been lingering for about a month now because there is some controversy about whether it is acceptable to make the units argument non-optional because it follows the desc argument. As a result, the non-controversial parts have been split out into MOM6 PR #320.

Going forward, there seem to be about 5 options for how to deal with the units argument:

  1. Keep the units argument optional, thereby allowing code developers not to document the units of their parameters.
  2. Accept that the non-optional units argument can follow the optional desc argument.
  3. Reorder the units and desc arguments in the get_param() interface and in about 1000 calls to get_param()
  4. Make both the units and desc arguments non-optional in get_param() calls, using the existing do_not_log optional argument to indicate when this is not to be logged, and perhaps trigger the logging in other cases on whether desc is blank rather than on whether desc is present. This would require changes in a number of get_param calls, but far fewer than option 3 above.
  5. Accept option 2 for now, and implement option 4 later.

Because of the separate PR #320, this PR is no longer holding up any other code changes.

  This commit includes changes to the get_param_real and log_param_real
interfaces to make the units arguments mandatory.  It also adds an optional
unscale argument to the log_param_real interfaces.  Without other changes in the
previous commits, it will cause the MOM6 code to fail to compile.  However, by
itself this commit does not change any answers or output.
@Hallberg-NOAA Hallberg-NOAA force-pushed the mandatory_get_param_units branch from 67c4d59 to 1489e59 Compare February 18, 2023 10:43
@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/19644 ✔️

@marshallward marshallward merged commit 201e705 into NOAA-GFDL:dev/gfdl Jun 27, 2023
@Hallberg-NOAA Hallberg-NOAA deleted the mandatory_get_param_units branch May 10, 2024 22:06
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