Fix crash in physics_check_data when registry variable has initial_value but not read from file, yet is modified (intent out) from physics scheme - #422
Merged
jimmielin merged 4 commits intoSep 29, 2025
Conversation
…lue but not read from file, but is modified (intent out) from physics scheme This is a proposed fix for a corner case where a variable is introduced into the CAM registry that has an initial_value but never read from file, as well as having an intent(out) or intent(inout) from a physics scheme. Such a case might happen because a variable might only be set in a scheme's "init" phase, yet used for "run" phases, and thus necessitates its introduction into the registry to persist state across timesteps. However, because this variable needs to be set in the scheme's "init" phase (and thus has intent(out)) it triggers the physics data check routine. The physics_check_data select case uses the find_input_name_idx to retrieve the name_idx, which is then used to get the actual input name, via phys_var_stdnames(name_idx); however, if a variable is intent(out) yet never read from file, find_input_name_idx in physics_data.F90 returns init_mark_idx = -2, which is out of bounds for phys_var_stdnames, and yet not covered by the guard case statements in the select case in physics_check_data; this commit adds the guard case for init_mark_idx with a comment to denote the corner case's triggering conditions.
jimmielin
changed the title
jimmielin
changed the title
nusbaume
approved these changes
Sep 26, 2025
nusbaume
left a comment
nusbaume
left a comment
Collaborator
There was a problem hiding this comment.
I have the world's most nit-picky nitpick, but otherwise this looks great to me. Thanks for fixing this corner case @jimmielin!
| @@ -1415,6 +1415,12 @@ def write_phys_check_subroutine(outfile, host_dict, host_vars, host_imports, | |||
| outfile.comment("If variable is a constituent, then do nothing. We'll handle these later", 6) | |||
Collaborator
There was a problem hiding this comment.
I know this isn't relevant for this PR (or anything really), but any chance we can add a period to the end of the We'll handle these later comment? That way my OCD won't fixate on it every time I look at this code.
Collaborator
Author
There was a problem hiding this comment.
Thanks @nusbaume, I found another one too when searching for later", ) so I added both of them!
Collaborator
Author
|
SIMA regression tests all bit-for-bit passed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phew, that is a long title!
Tag name (required for release branches):
Originator(s): @jimmielin
Description (include the issue title, and the keyword ['closes', 'fixes', 'resolves'] followed by the issue number):
This is a proposed fix for a corner case where a variable is introduced into the CAM registry that has an initial_value but never read from file, as well as having an intent(out) or intent(inout) from a physics scheme.
Such a case might happen because a variable might only be set in a scheme's "init" phase, yet used for "run" phases, and thus necessitates its introduction into the registry to persist state across timesteps. However, because this variable needs to be set in the scheme's "init" phase (and thus has intent(out)) it triggers the physics data check routine.
The physics_check_data select case uses the find_input_name_idx to retrieve the name_idx, which is then used to get the actual input name, via phys_var_stdnames(name_idx); however, if a variable is intent(out) yet never read from file, find_input_name_idx in physics_data.F90 returns init_mark_idx = -2, which is out of bounds for phys_var_stdnames, and yet not covered by the guard case statements in the select case in physics_check_data; this commit adds the guard case for init_mark_idx with a comment to denote the corner case's triggering conditions.
Describe any changes made to build system: updates
write_init_files.pywhich handles code generation forphysics_inputs.F90.To replicate
hplin/gw_drag@2cfe2971b05dcbe81fca4b1157a5ca8c408e7f81.gitmodulesto use jimmielin/atmospheric_physics atb2eabf666d6b06e63e59a429e6a4b5bcc32bfcd7for ncar-physics external--compiler gnu --run-unsupported --compset FPHYStest --res ne3pg3_ne3pg3_mg37; select physics suite./xmlchange CAM_CONFIG_OPTS="--physics-suites gw_cam4 --dyn none"user_nl_camobserve crash at end of first timestep where physics check data normally runs.
Describe any changes made to the namelist: N/A
List any changes to the defaults for the input datasets (e.g. boundary datasets): N/A
List all files eliminated and why: N/A
List all files added and what they do: N/A
List all existing files that have been modified, and describe the changes:
(Helpful git command:
git diff --name-status development...<your_branch_name>)If there are new failures (compared to the
test/existing-test-failures.txtfile),have them OK'd by the gatekeeper, note them here, and add them to the file.
If there are baseline differences, include the test and the reason for the
diff. What is the nature of the change? Roundoff?
All B4B