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

PBL utils transfer #193

Open
wants to merge 34 commits into
base: development
Choose a base branch
from

Conversation

mwaxmonsky
Copy link
Collaborator

Originator(s): @mwaxmonsky

Summary (include the keyword ['closes', 'fixes', 'resolves'] and issue number): Adds pbl_utils capability from CAM.

Describe any changes made to the namelist: N/A

List all files eliminated and why: N/A

List all files added and what they do:
M phys_utils/atmos_pbl_utils.F90

  • Transferred all functionality (except for compute_radf(...)) from ESCOMP/cam/src/physics/cam/pbl_utils.F90

List all existing files that have been modified, and describe the changes: N/A
(Helpful git command: git diff --name-status development...<your_branch_name>)

List any test failures: N/A

Is this a science-changing update? New physics package, algorithm change, tuning changes, etc? No

@mwaxmonsky mwaxmonsky changed the title Feature/pbl utils refactor PBL utils transfer Jan 23, 2025
@mwaxmonsky mwaxmonsky requested a review from nusbaume February 10, 2025 15:29
@nusbaume nusbaume requested a review from jimmielin February 10, 2025 18:51
Copy link
Member

@jimmielin jimmielin left a comment

Choose a reason for hiding this comment

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

Thanks @mwaxmonsky! I did not review the equations themselves as I assume they match bit-for-bit as used in current CAM.

I had one comment about one of the private functions since you noted hb_diff which I hope to be working on soon.

All the other comments are about unit comments - sorry in advance if I'm being too annoying about this - I noticed that only some of the functions had input/outputs' units noted in the comments, but not all of the unnoted quantities are unitless. Since this file does not have a .meta file attached, I think it would be good for future reference to note down the units while this file is still fresh. I went through the file and worked out the units (hopefully correctly) and added review comments where relevant. I'm happy to discuss if this is too much trouble!

! Description of the NCAR Community Climate Model (CCM1).
! University Corporation for Atmospheric Research. https://doi.org/10.5065/D6TB14WH (Original work published 1987)
! Equation 2.f.15, page 12
! NOTE: shear_squared vriable currently (01/2025) computed in hb_diff.F90 (trbintd) matches referenced equation.
Copy link
Member

Choose a reason for hiding this comment

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

vriable -> variable

I looked in hb_diff.F90 and the other files in the companion PR in ESCOMP/CAM (https://github.com/ESCOMP/CAM/pull/1235/files) and couldn't find where neutral_exchange_coefficient was being used, except through calc_eddy_flux_coefficient.

Additionally hb_diff.F90 still names the shear squared variable as s2 and not shear_squared.

I was wondering if you intended for there to be any further changes on trbintd in hb_diff.F90 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.

Fixed the typo!

In terms of uses of neutral_exchange_coefficient, yup, it's only a helper function (though probably named a little too long) to prevent us from having to copy/paste the ml2*sqrt(s2). It's only used twice so it is right on that edge of probably not needing it but I'm happy to discuss whether it's useful enough to pull out or not.

Updated the comment to reference s2.

In an ideal world, yes, I would like to update and pull some of the functions in trbintd into the physics utils as they are completely standalone and also follow CCM1/other documents that might be useful in other schemes. There was already enough in this PR so we decided it best to add it to review and follow up in the next step with possibly refactoring trbintd.

In theory, there may be some improvements to doing so as well since the computed data is done every time step anyway, moving it into atmos_phys would allow for a cleaner API for the host so that the temporary data is computed by the physics layer and used directly instead of the host doing physics calculations.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for explaining @mwaxmonsky! This sounds great. All the updates will greatly help eventually CCPPizing hb_diff.F90 (when I was first scoping it out months ago I kept thinking how to deal with these pbl_utils...) so I really appreciate this work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nusbaume Actually, after closer inspection, would it be worthwhile to remove trbintd in this PR? It computes s2, n2, and ri and we actually don't use n2 from what I can tell. So we could at least remove the n2 part and I could look at cleaning up the API right now and I don't think there's be that much of a lift to pull that into atmos_phys. Would this be reasonable at the moment?

phys_utils/atmos_pbl_utils.F90 Outdated Show resolved Hide resolved
phys_utils/atmos_pbl_utils.F90 Outdated Show resolved Hide resolved
phys_utils/atmos_pbl_utils.F90 Outdated Show resolved Hide resolved
phys_utils/atmos_pbl_utils.F90 Outdated Show resolved Hide resolved
phys_utils/atmos_pbl_utils.F90 Outdated Show resolved Hide resolved
phys_utils/atmos_pbl_utils.F90 Show resolved Hide resolved
@mwaxmonsky mwaxmonsky requested a review from jimmielin February 12, 2025 22:14
Copy link
Member

@jimmielin jimmielin left a comment

Choose a reason for hiding this comment

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

Thanks @mwaxmonsky!

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.

2 participants