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

cam6_3_134: Update atmospheric_physics external #891

Merged
merged 18 commits into from
Oct 31, 2023

Conversation

nusbaume
Copy link
Collaborator

@nusbaume nusbaume commented Oct 3, 2023

This PR updates the atmospheric_physics external in CAM, which requires some source modifications on the CAM-side. There has also been some replacement of CAM code with CCPP physics routines in order to reduce duplication.

Finally, this PR also fixes a bug found in Kessler where it was using the wrong pressure when converting to/from potential temperature via the exner function.

Fixes #752
Fixes #802

Closes #904

@nusbaume nusbaume added bug-fix This PR was created to fix a specific bug. externals externals updating issue or PR SIMA ccpp-conversion labels Oct 3, 2023
@nusbaume nusbaume self-assigned this Oct 3, 2023
Externals_CAM.cfg Outdated Show resolved Hide resolved
src/utils/ccpp_constituent_prop_mod.F90 Outdated Show resolved Hide resolved
src/utils/ccpp_constituent_prop_mod.F90 Outdated Show resolved Hide resolved
src/utils/ccpp_constituent_prop_mod.F90 Outdated Show resolved Hide resolved
src/physics/cam/geopotential.F90 Outdated Show resolved Hide resolved
src/physics/cam/geopotential.F90 Show resolved Hide resolved
src/physics/cam/geopotential.F90 Outdated Show resolved Hide resolved
src/dynamics/se/dp_coupling.F90 Outdated Show resolved Hide resolved
src/physics/simple/held_suarez_cam.F90 Outdated Show resolved Hide resolved
src/physics/cam/geopotential.F90 Outdated Show resolved Hide resolved
src/physics/cam/physpkg.F90 Outdated Show resolved Hide resolved
src/physics/cam_dev/physpkg.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

We had some committed code which had the 1:ncol error as well as more places you added code. This should be all of them in this PR.

Basically the rule of thumb is that any run methods which have arrays dimensioned by pcols in CAM, need to have that subsetted by 1:ncol in the call. (BTW, my ZM conversion doesn't have pcols removed from the lower layer yet - that will come with the metadata creation step - I went and checked!)

src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
src/physics/simple/kessler_cam.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@peverwhee peverwhee left a comment

Choose a reason for hiding this comment

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

A couple of non-critical questions/comments about the constituent_prop_mod stub.

Comment on lines 69 to 71
if(present(errmsg)) then
errmsg = 'Still Not Used!'
end if
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get why this error message exists for ccp_get_standard_name, but why these other subroutines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! I've changed the error messages to just be empty strings in order to avoid confusion (and to hint that CAM doesn't actually use these specific error messages). If however you think I should actually be generating and using an error message variable here and in CAM then please let me know.

Comment on lines +31 to +50
subroutine ccp_get_standard_name(this, std_name, errcode, errmsg)
! Return this constituent's standard name

! Dummy arguments
class(ccpp_constituent_prop_ptr_t), intent(in) :: this
character(len=*), intent(out) :: std_name
integer, optional, intent(out) :: errcode
character(len=*), optional, intent(out) :: errmsg

std_name = 'Not Used!'

! Provide err values if requested:
if(present(errcode)) then
errcode = 0
end if
if(present(errmsg)) then
errmsg = 'Still Not Used!'
end if

end subroutine ccp_get_standard_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because in CAM we are building (but not running) the CCPP-ized version of qneg, which uses the standard_name constituent property, which in-turn is pointing to this subroutine (in the same way that the real CCPP constituents object points to a subroutine).

So without a subroutine like this the CAM build will fail. I could simplify it further, but I also imagine in the future we'll have physics schemes which will really need to use this subroutine, or something like it, which is why I kept it in there.

Hopefully that makes sense? If you still think it would be a good idea to simplify it even further then just let me know.

@peverwhee peverwhee changed the title Update atmospheric_physics external cam6_3_134: Update atmospheric_physics external Oct 26, 2023
@nusbaume
Copy link
Collaborator Author

@jtruesdal just pinging you about reviewing this PR when you get the chance!

Copy link
Collaborator

@jtruesdal jtruesdal left a comment

Choose a reason for hiding this comment

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

Thanks for the ping. Looks good.

@nusbaume nusbaume merged commit 1e09295 into ESCOMP:cam_development Oct 31, 2023
@nusbaume nusbaume deleted the atmos_phys_update branch November 13, 2023 19:47
gold2718 pushed a commit to gold2718/CAM that referenced this pull request May 2, 2024
Merge pull request ESCOMP#891 from nusbaume/atmos_phys_update

cam6_3_134: Update atmospheric_physics external

ESCOMP commit: 1e09295
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix This PR was created to fix a specific bug. ccpp-conversion externals externals updating issue or PR SIMA
Projects
Status: Tag
Development

Successfully merging this pull request may close these issues.

5 participants