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

Update standard and function names plus major code clean-up #67

Merged
merged 29 commits into from
Oct 9, 2023

Conversation

nusbaume
Copy link
Collaborator

The main purpose of this PR is to update the CCPP standard names to match the internal AMP agreed-upon names, as well as rename some of the physics routines to make them less ambigious and more accurate. Along with these changes, significant code cleanup was performed along with some minor bug fixes, with the goal being to produce bit-for-bit results using the same repo version in both CAM and CAM-SIMA.

Fixes #60
Fixes #61
Fixes #62
Fixes #64

nusbaume added 24 commits May 5, 2023 14:48
@nusbaume nusbaume added documentation Improvements or additions to documentation enhancement New feature or request bug-fix This PR was created to fix a specific bug. labels Sep 18, 2023
@nusbaume nusbaume self-assigned this Sep 18, 2023
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.

Looks good (and like a lot of work!) to me!

Thank you!

@nusbaume
Copy link
Collaborator Author

@PeterHjortLauritzen I mostly just requested your review here to see if the updated standard names and physics routine names sound reasonable to you. However if you end up not having the time to review then just let me know and I'll take you off the review list. Thanks!

Copy link

@PeterHjortLauritzen PeterHjortLauritzen 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. Thanks Jesse ... just have one question (see comment for specific code section)

@@ -187,20 +124,20 @@ subroutine wet_to_dry_water_vapor_run(ncol, nz, pdel, pdeldry, qv, qv_dry, &

integer, intent(in) :: ncol

Choose a reason for hiding this comment

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

The conversion from wet to dry and dry to wet mixing ratios is general. I am curious why there are separate calls for water vapor, rain, etc. to convert from wet to dry. Could we use a generic subroutine instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We currently have these separate calls for legacy reasons, i.e. we had converted Kessler before we had any general way to deal with constituents in the framework.

However, I agree that we should have generic wet <-> dry conversion routines, and now that we have a constituents object that should be doable. I can make an issue about it so that we remember to tackle it at some point in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just created the issue, which can be found here: #68

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.

review of ChangeLog and standard names. Stiil need to review code

doc/ChangeLog Outdated Show resolved Hide resolved
held_suarez/held_suarez_1994.meta Outdated Show resolved Hide resolved
kessler/kessler.meta Show resolved Hide resolved
kessler/kessler.meta Outdated Show resolved Hide resolved
doc/ChangeLog Outdated Show resolved Hide resolved
doc/ChangeLog Outdated Show resolved Hide resolved
doc/ChangeLog Outdated Show resolved Hide resolved
doc/ChangeLog Outdated Show resolved Hide resolved
kessler/kessler.meta 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.

Minor comment for code. This completes my review of the Fortran

utilities/geopotential_temp.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@mwaxmonsky mwaxmonsky 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 to me! Great work @nusbaume!

@nusbaume nusbaume merged commit 0597379 into ESCOMP:main Oct 9, 2023
cacraigucar added a commit to cacraigucar/atmospheric_physics that referenced this pull request Dec 13, 2023
Merge pull request ESCOMP#67 from nusbaume/updated_standard_names
@nusbaume nusbaume deleted the updated_standard_names branch September 24, 2024 21:46
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. documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: No status
6 participants