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
Add CAM diagnostic schemes #105
Add CAM diagnostic schemes #105
Changes from 6 commits
cc42526
ca7794b
78b8bfb
6363a53
61bd9d3
8d44484
9b4a79a
06dee83
e89e912
ff29507
a3ee11b
9f8fc41
17d4465
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.
Shouldn't the contents of these two variables be dictated by whatever is populating the constituent object and loop over what is in that object?
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.
we're looping over the constituents object, yes. But this is a mapping of the standard name to the diagnostic name (which is not contained within the object). Not all of these will necessarily be "used". Does that make any sense?
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.
This still feels "fragile" to me, as the diagnostic name index may not match the constituent index. Right now in ZM, I am assuming that the tendency array maps one-to-one to the constituent array. I would naively think that the diagnostic array would also use the same indexing. Let me know if we should have a discussion on this.
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.
Hi @cacraigucar I'm not quite following. Is the problem that someone could modify one of the lists (
const_std_names
orconst_diag_names
) and then they'd be off? To clarify, when I'm looping over the constituents, I don't expect them to be in this exact order. This is just a mapping between standard name and diagnostic name (which is not contained in the properties object). Let me know if I'm still missing something!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.
Hi @peverwhee, @cacraigucar. If I understand this fragility correctly it might be possible that user error could cause the lists to become out-of-sync. I wonder of ways to mitigate this, e.g., checking the lists are of same length at initialization (to prevent the obvious typo of adding one but not the other), but all bets are off if an entry is added to the wrong place in one of the lists.
My immediate thought was to assign these into a Nx2 array instead of two N-sized arrays but a quick Google didn't reveal if Fortran had syntax like this, which would prevent typing errors as much as possible:
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.
When I do this, I always use a parameter for each dimension size.
The only example of this I found in CAM code is from the very ancient (well before my time) src/physics/camrt/radae.F90
Newer code, such as src/dynamics/se/dycore/fvm_consistent_se_cslam.F90 does have a parameter for the changeable dimension which I think addresses your main safety concern.
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.
Thanks! I like this in
fvm_consistent_se_cslam.F90
, which I guess is what you're referring to: it's as close to safe and 'automatic' as we can get before the clean-up issue #137 is addressed: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.
While this conversation has been good, my concern was not actually about there being two arrays, but rather about the necessity of having them at all. I had a google chat with Courtney which resulted in #137 and will eventually eliminate the need for these arrays.
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.
Thanks @cacraigucar, my apologies for misunderstanding the original issue. Until #137 becomes a more permanent fix the current code looks good!
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.
No apology needed. Nobody understood what my concern was until I met with Courtney and explained it in person!