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

New const_get_index logic without cam_ccpp_cap dependency (in atmos_phys) #314

Merged

Conversation

jimmielin
Copy link
Member

Companion atmospheric_physics PR: ESCOMP/atmospheric_physics#135

This avoids a circular dependency if schemes require const_get_index, which presently depends on cam_ccpp_cap, which depends on individual schemes.

@jimmielin jimmielin added the enhancement New feature or request label Oct 16, 2024
@jimmielin jimmielin self-assigned this Oct 16, 2024
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Looks good, but did have two small change requests.

if not os.path.isdir(atm_phys_to_be_ccppized_dir):
# CAM-SIMA will likely not run without this, so raise an error
emsg = "ERROR: Unable to find CCPP physics to_be_ccppized directory:\n"
emsg += f" {atm_phys_util_dir}\n Have you run 'git-fleximod'?"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be atm_phys_to_be_ccppized_dir here:

Suggested change
emsg += f" {atm_phys_util_dir}\n Have you run 'git-fleximod'?"
emsg += f" {atm_phys_to_be_ccppized_dir}\n Have you run 'git-fleximod'?"

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, my bad! Fixed


if (errcode /= 0) then
call endrun(subname//"Error "//to_str(errcode)//": "// &
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might use stringify here instead of to_str (my hope is to eventually use stringify for all string conversions):

Suggested change
call endrun(subname//"Error "//to_str(errcode)//": "// &
use string_utils, only: stringify
...
call endrun(subname//"Error "//stringify((/errcode/))//": "// &

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated!

Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

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

Looks great now! Just had one last bug fix (but it doesn't need a re-review). Thanks for getting this in!

@@ -319,7 +319,7 @@ subroutine const_get_index(name, cindex, abort, warning, caller)
call ccpp_const_get_idx(const_props, name, cindex, errmsg, errcode)

if (errcode /= 0) then
call endrun(subname//"Error "//to_str(errcode)//": "// &
call endrun(subname//"Error "//stringify(errcode)//": "// &
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think stringify expects an array here, so:

Suggested change
call endrun(subname//"Error "//stringify(errcode)//": "// &
call endrun(subname//"Error "//stringify((/errcode/))//": "// &

@jimmielin
Copy link
Member Author

Thanks @nusbaume! Apologies for the oversight. Updated and waiting for tests to run then I'll merge to development.

@jimmielin
Copy link
Member Author

All build tests passed - had to wait in the queue for longer than I expected. Merging now! Thanks everyone for the reviews and feedback.

@jimmielin jimmielin merged commit 1281928 into ESCOMP:development Oct 18, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Tag
Development

Successfully merging this pull request may close these issues.

3 participants