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.

Sign up for GitHub

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

Jump to bottom

Adds a vector of default values to get_param_real_array() #760

Merged
merged 2 commits into from
Dec 5, 2024
Merged

Adds a vector of default values to get_param_real_array() #760

merged 2 commits into from
Dec 5, 2024

Conversation

adcroft
Copy link
Member

@adcroft adcroft commented Nov 26, 2024 •
edited
Loading

The default= optional argument to get_param() only provides a uniform value to initialize an array of reals. This commit adds the optional defaults= argument that must have the same length as the values argument.

I've also added a few instances of this optional argument:

  • by adding the initialize_thickness_param() procedure, selected by THICKNESS_CONFIG = "param". The procedure was based on the "uniform" method, and uses the parameter THICKNESS_INIT_VALUES which defaults to uniform values derived from MAXIMUM_DEPTH
  • the setting of MLD_EN_VALS in MOM_diabatic_driver.F90 which was previously using a work around to set defaults to 25, 2500, 250000 J/m2.
  • two vectors of 4 values in user/user_change_diffusivity.F90

For super repos there will be some doc file changes, but no answer changes.

This PR was inspired after realizing get_param_real_array() could not be used in PR #737 where 16 run-time non-dimensional parameters are being added.

@codecov Codecov
Copy link

codecov bot commented Nov 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.05128% with 7 lines in your changes missing coverage. Please review.

Project coverage is 36.69%. Comparing base (7a9adbc) to head (9eec351).

Files with missing lines Patch % Lines
src/framework/MOM_file_parser.F90 55.55% 2 Missing and 2 partials ⚠️
src/initialization/MOM_state_initialization.F90 88.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##           dev/gfdl     #760      +/-   ##
============================================
+ Coverage     36.67%   36.69%   +0.01%     
============================================
  Files           278      278              
  Lines         84269    84306      +37     
  Branches      15869    15877       +8     
============================================
+ Hits          30908    30935      +27     
- Misses        47532    47537       +5     
- Partials       5829     5834       +5

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

@adcroft
Copy link
Member Author

adcroft commented Nov 26, 2024

I see that tc2.target failed, but that's because the new parameter value isn't available with the target executable. @marshallward shouldn't we be running the target executable in the target's .testing?

@marshallward
Copy link
Member

marshallward commented Nov 26, 2024 •
edited
Loading

I think regression uses everthing in its own .testing for compilation (that is, .testing/build/target_codebase/.testing) but they may use the top .testing directory when running the tests.

(Edit: We build target inside of its own .testing, but it is symlinked into .testing/build/target and treated as just another executable alongside the others. It would take some additional work to completely run the tests inside of its own .testing and run the regression checks. Running and checksum diffs are not set up for arbitrary directories.)

You are probably right and it should run its own tests. I am a bit worried that we are starting to put too much distance between the two codebases, but I can't see any specific problem here.

@adcroft adcroft added the enhancement New feature or request label Nov 26, 2024
Hallberg-NOAA
Hallberg-NOAA requested changes Nov 26, 2024
View reviewed changes
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.

This change looks like it will be a valuable new addition, but before this is accepted, I am requesting one minor change to keep this new commit in line with the overall MOM6 code style guide.

@adcroft
Copy link
Member Author

adcroft commented Dec 2, 2024

FWIW, dropping the new defaults= optional argument, and making default= a real array does work and only requires a few files to be corrected. These are the files where we use default= to set a vector of constant values, and all were easily changed:

        modified:   ALE/MOM_hybgen_regrid.F90
        modified:   core/MOM_open_boundary.F90
        modified:   initialization/MOM_state_initialization.F90
        modified:   parameterizations/vertical/MOM_diabatic_driver.F90
        modified:   tracer/oil_tracer.F90
        modified:   user/MOM_wave_interface.F90
        modified:   user/user_change_diffusivity.F90

I'll ask a the dev call if we prefer this version...

@adcroft adcroft force-pushed the add-defaults-vector-to-get_param_real_array branch from 9eec351 to 8f7dd3e Compare December 2, 2024 20:50
@adcroft
Removed default for mandatory time scale in OBCs
f8dda20 
Removed two instances of `fail_if_missing=.true., default=0.` which
are contradictory: a default value is meaningless if the parameter must
be specified.

I encountered this when adding the `defaults=` option to `get_param_real_array()`.
@adcroft adcroft force-pushed the add-defaults-vector-to-get_param_real_array branch 3 times, most recently from 9f15344 to f8dda20 Compare December 2, 2024 22:14
@adcroft
Adds a vector of default values to get_param_real_array()
f098514 
The `default=` optional argument to get_param() only provides a uniform
value to initialize an array of reals. This commit adds the optional
`defaults=` argument that must have the same length as the `values`
argument.

I've also added a few instances of this optional argument:
 - by adding the `initialize_thickness_param()` procedure, selected by
   `THICKNESS_CONFIG = "param"`. The procedure was based on the "uniform"
   method, and uses the parameter `THICKNESS_INIT_VALUES` which defaults
   to uniform values derived from `MAXIMUM_DEPTH`
 - the setting of MLD_EN_VALS in MOM_diabatic_driver.F90 which was
   previously using a work around to set defaults to 25, 2500, 250000 J/m2.
 - two vectors of 4 values in user/user_change_diffusivity.F90

There will be some doc file changes, but no answer changes.
@adcroft
Copy link
Member Author

adcroft commented Dec 2, 2024

I tried assumed-rank arrays for default=, following @marshallward 's suggestion, and it looked very promising, but we discovered a feature-support problem with our favourite third compiler. 🙄 So I'm settling on the original proposal, have undone the tc2 change, and addressed the other raised issues.

Hallberg-NOAA
Hallberg-NOAA approved these changes Dec 4, 2024
View reviewed changes
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 changes now all look good to me.

We probably should add the analogous new defaults optional argument to get_param_integer_array(), but that could wait for a subsequent PR.

@adcroft
Copy link
Member Author

adcroft commented Dec 5, 2024

Testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/25665

@adcroft adcroft merged commit 0337147 into NOAA-GFDL:dev/gfdl Dec 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marshallward marshallward marshallward left review comments

@Hallberg-NOAA Hallberg-NOAA Hallberg-NOAA approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adcroft @marshallward @Hallberg-NOAA