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.
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
base: development
Are you sure you want to change the base?
PBL utils transfer #193
Changes from 31 commits
adbad1c
ae2fbb1
06e0328
aba5ec1
9fbafbb
1fcc007
8e106fb
71f5e04
483f46b
d8c00aa
2e6a946
9193552
860abf9
8dc6284
e71de9a
bcc145a
1bf1633
6ad515c
31632c6
933affd
937a087
8ad300a
0d9e0be
7538c86
a348392
a74defe
7a70379
1d0801a
3302625
426f759
e76c482
48189a8
01bfbf4
8376133
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 whereneutral_exchange_coefficient
was being used, except throughcalc_eddy_flux_coefficient
.Additionally
hb_diff.F90
still names the shear squared variable ass2
and notshear_squared
.I was wondering if you intended for there to be any further changes on
trbintd
inhb_diff.F90
in the future?There was a problem hiding this comment.
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 theml2*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 refactoringtrbintd
.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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 computess2
,n2
, andri
and we actually don't usen2
from what I can tell. So we could at least remove then2
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?