Skip to content

Sync from NCAR/main + Thompson params#309

Merged
grantfirl merged 43 commits into
ufs-community:ufs/devfrom
grantfirl:sync_NCAR_main_2025_09_04
Oct 16, 2025
Merged

Sync from NCAR/main + Thompson params#309
grantfirl merged 43 commits into
ufs-community:ufs/devfrom
grantfirl:sync_NCAR_main_2025_09_04

Conversation

@grantfirl
Copy link
Copy Markdown
Collaborator

@grantfirl grantfirl commented Sep 4, 2025

Description of Changes:

Changes from:

  1. Combination PR for #1141, #1152 and ufs/dev #304 NCAR/ccpp-physics#1159

The issue: Some subroutines do not have return statements after an error occurs. Instead two variables errflg and errmsg are reset or re-initialized in subsequent calls, and if an error occurs it is not captured and the program continues without stopping. Please note that some calls already have if(errflg/=0) return in place.

This PR fixes an issue to return right after an error occurs. Subroutines that use errflg and errmsg should be checked for errflg and abort if necessary. Insert if(errflg/=0) return after such calls.

In addition the following is added:

Remove unnecessary return statements at the end of subroutines. In one case, there were actually two return statements right after each other.
Improve error messages and remove duplicate print statements if they are the same as the errmsg.
Change “can not” to “cannot” and other minor additions.
UPDATE: Thanks to reviewers' feedback, this PR also includes cleanup of GFS_phys_time_vary.fv3.F90 to remove copy_error calls from one-thread region (accessing errmsg and errflg directly).

NCAR#1152 description (issue):
The way w3emc is used in GFS_time_vary_pre is not safe, since there is no protection against argument mismatches in case the wrong version of the library (e.g., _4 instead of _d) is used. This came up during the debugging of a w3emc build issue with Intel oneAPI ifx.

A detailed description can be found here: JCSDA/spack-stack#1716

The issue also provides a code snippet that protects against using a wrong kind for the integer and real arguments in the w3difdat routines.

  1. reviewer changes from UFS-dev PR#253 NCAR/ccpp-physics#1154 (parameterized types in canopy_utils_mod)

  2. reviewer changes from UFS-dev PR#195 NCAR/ccpp-physics#1153 (remove dependence on MPI in mp_tempo_post, other minor naming changes)

  3. bugfix for NSSL MP: bugfix: NaNs in module_mp_nssl_2mom.F90 NCAR/ccpp-physics#1157

  4. Make some Thompson MP tuning parameters visible/tunable from host (needed by NRL NEPTUNE)

  5. Remove NOAHMP write statement that was making output too verbose (Remove non-warning log write statement #321 )

Tests Conducted:

UFS RTs on Ursa show no changes to baselines. Some testing was done at NRL with the NEPTUNE model that necessitated reversing some UFS-specific tuning commits. See NCAR#1158

Dependencies:

None

Documentation:

None

Issue (optional):

Fixes NCAR#1155
Closes #320

Contributors (optional):

@matusmartini @climbfuji @dpsarmie

matusmartini and others added 30 commits June 5, 2025 04:48
the end of subroutines, remove duplicate print statements
…k that w3emc is using the expected precision
…k that w3emc is using the expected precision
Co-authored-by: Dom Heinzeller <dom.heinzeller@icloud.com>
Co-authored-by: Dom Heinzeller <dom.heinzeller@icloud.com>
Co-authored-by: Dom Heinzeller <dom.heinzeller@icloud.com>
…ue 1155 for details

Co-authored-by: MicroTed <37668594+MicroTed@users.noreply.github.com>
…997_A-SCM_WoFS_v0-CI

bugfix: NaNs in module_mp_nssl_2mom.F90

if (present(con_nt_c_l)) nt_c_l = con_nt_c_l
if (present(con_nt_c_o)) nt_c_o = con_nt_c_o
if (present(con_av_i)) then
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Where is this con_av_i defined, in GFS_typedefs.F90? Is it initialized as 0 ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ruiyu,
Since it is an optional, It is used only when larger than 0. However, I think it is not used at this moment.

Copy link
Copy Markdown
Collaborator Author

@grantfirl grantfirl Sep 11, 2025

Choose a reason for hiding this comment

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

@RuiyuSun It is passed in as an optional argument. It is defined in GFS_typedefs as -999 and is set by namelist, although no namelists are set up to change it. For all UFS configurations, right now, con_av_i is present, but less than 0, so the functional form is used below. For NEPTUNE, they will pass in a value > 0 and therefore, it can be used as a constant, which is what they need. A couple of commits back, I tried to just have it as an optional argument without defining it in GFS_typedefs.F90, but apparently, the CCPP framework won't allow this, hence the need to define it, but set it as a nonsense/negative value.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@dustinswales @climbfuji Optional argument experts: is it correct to say that for variables that are optional at the scheme level at least need to be declared and have metadata in every host? For example, I originally had the av_i argument in this PR as an optional argument with metadata in Thompson MP, but since I knew that UFS-based hosts wouldn't need it, I didn't even add a declaration or metadata in *_typedefs. The CCPP framework errored out because it expected the host to at least have these for cross-checking. Should the framework be able to skip these variables (that are optional in the scheme but not present in the host)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See d856677 for how I originally coded it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's an interesting question. I agree that the idea of "if it's an optional argument and the host doesn't know, then simply don't provide it - i.e. never present" sounds good. One question would then be if that opens the door to unintended errors. But we need to first remind ourselves what capgen does in this case. Does it create the optional variable? Or does it ignore optional variables when it generates interstitial variables in the caps? @dustinswales do you know?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does anyone knows the answer to @climbfuji's question? @dustinswales Do you know? thanks

Copy link
Copy Markdown
Collaborator Author

@grantfirl grantfirl Sep 17, 2025

Choose a reason for hiding this comment

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

In any case, I don't think that this should hold up this PR since it works as coded, although we're having to define something in the UFS that it will likely never use, which is wasteful. In this case, it's just a scalar, but one could imagine that this could be a larger issue if schemes needed large optional arrays, but in that case, we'd just make them conditionally-allocatable with a condition that always evaluates to false, so no extra memory involved, just kinda unnecessary code.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK. Thanks @grantfirl

Copy link
Copy Markdown
Collaborator

@AnningCheng-NOAA AnningCheng-NOAA left a comment

Choose a reason for hiding this comment

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

looks fine with me

@grantfirl
Copy link
Copy Markdown
Collaborator Author

Combined #321 into this one.

@FernandoAndrade-NOAA
Copy link
Copy Markdown

Testing for #2882 is complete, please continue with the merge process, thank you.

@grantfirl grantfirl merged commit cf752f7 into ufs-community:ufs/dev Oct 16, 2025
3 checks passed
tsga added a commit to tsga/ccpp-physics that referenced this pull request Oct 17, 2025
…_main_2025_09_04"

This reverts commit cf752f7, reversing
changes made to e6949b3.
tsga pushed a commit to tsga/ccpp-physics that referenced this pull request Oct 17, 2025
…25_09_04

Sync from NCAR/main + Thompson params
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.

Non-warning log writes occurring when Land IAU is used MP NSSL NaN Error Fix