Skip to content

Loosen restriction on optional arguments.#248

Merged
dustinswales merged 25 commits into
ufs-community:ufs/devfrom
dustinswales:feature/optional_args_again
Apr 25, 2025
Merged

Loosen restriction on optional arguments.#248
dustinswales merged 25 commits into
ufs-community:ufs/devfrom
dustinswales:feature/optional_args_again

Conversation

@dustinswales
Copy link
Copy Markdown
Collaborator

@dustinswales dustinswales commented Jan 27, 2025

When optional arguments were added back into the CCPP as part of #189, all variables that were conditionally allocated on the host side were required to be defined as optional within the schemes. This restriction required host-model logic to be implicitly built into the schemes and metadata. For example, scheme variables that were optional in the context of the host model, but always required within the context scheme, still needed to be defined as optional within the scheme. This resulted in schemes having an arbitrary/confusing mis-match of required/optional arguments.

Alongside this PR is a PR into the CCPP Framework that removes this restriction.
The changes contained within here revert nearly all of the optional argument changes introduced by #189 to the primary schemes.
The UFS interstitial schemes are untouched in this PR, as these schemes are designed to flexible (optional arg heavy) in order to couple to many primary schemes.

@climbfuji

Copy link
Copy Markdown

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

This is too much to review line by line. We will need to rely on the UFS regression testing. As long as all tests pass with b4b identical results, this is good. Thanks for all your efforts!

@dustinswales
Copy link
Copy Markdown
Collaborator Author

This is too much to review line by line. We will need to rely on the UFS regression testing. As long as all tests pass with b4b identical results, this is good. Thanks for all your efforts!

@climbfuji Agreed.
I found a small metadata bug when running the full tests overnight. Re running now.

@grantfirl
Copy link
Copy Markdown
Collaborator

@dustinswales I've finished my review. It looks like perhaps there is a mistake in the changes to cu_gf_driver. Otherwise, looks good to me. I'll approve once changes to cu_gf_driver are made.

@dustinswales
Copy link
Copy Markdown
Collaborator Author

@grantfirl Let me look into what's going on.

@grantfirl
Copy link
Copy Markdown
Collaborator

@dustinswales Let me know when you get a chance to look into the cu_gf_driver thing. I'm hesitant to put this in the UFS merge queue until it's sorted out.

@dustinswales
Copy link
Copy Markdown
Collaborator Author

@grantfirl Sorry, I forgot to ping you, but I figured the issue out last week and this is all ready to go. All RTs pass on Hera:
/scratch1/BMC/gmtb/Dustin.Swales/UFS/framework/optional_args/ufs-weather-model-2/tests/logs/RegressionTests_hera.log

@grantfirl
Copy link
Copy Markdown
Collaborator

grantfirl commented Mar 7, 2025

@grantfirl Sorry, I forgot to ping you, but I figured the issue out last week and this is all ready to go. All RTs pass on Hera: /scratch1/BMC/gmtb/Dustin.Swales/UFS/framework/optional_args/ufs-weather-model-2/tests/logs/RegressionTests_hera.log

@dustinswales But, the inconsistency between the Fortran and metadata still exists. aod_gf is optional in the Fortran but not in the metadata. maxmf is not optional in the Fortran but is optional in the metadata. Maybe RTs pass, but isn't this still wrong?

Comment thread physics/CONV/Grell_Freitas/cu_gf_driver.meta
Comment thread physics/Radiation/RRTMG/radsw_main.F90
@grantfirl
Copy link
Copy Markdown
Collaborator

@dustinswales Added small metadata-only bugfix coming from another PR after it was already merged into ufs/dev and caught upon merge into NCAR/main: NCAR@0d4b4b9

@dustinswales dustinswales force-pushed the feature/optional_args_again branch from de7d232 to b3f86f8 Compare April 16, 2025 22:30
@jkbk2004
Copy link
Copy Markdown

all tests are done at ufs-community/ufs-weather-model#2573. @rhaesung @grantfirl can you merge 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.