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
Constituents #206
Constituents #206
Changes from 28 commits
2d48f66
b2ee696
147169b
c023d6f
fbc54f6
502a7e5
f703d54
582589c
076bc79
a8a5394
cdccfef
aa68644
bf9b08e
0126363
1d11cf8
0d519b8
4a21611
48d8e35
cae253f
818617b
dc004f2
7b9fb3c
cf3e747
0571130
3106e8b
6862f3b
9433fcc
92639bf
00f4423
98d917f
4e5e1f0
ef5223d
4f19dc6
af08f73
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.
I've gone back and forth on this one, but thought I'd put it out there for discussion. I am uncomfortable about requiring host models to add a call for a subroutine which does not exist, but is auto-generated. Is there a way that this call could be hidden away in auto-generated CCPP code?
Up until this PR, we had the 5 phases as the only special code needed to interface with the CCPP. With this PR, we are adding "register", "initialize" and "number" for constituents. Each have their own API and there is no code to look at until the CCPP is run. Obviously this will need to be extensively documented, but I am wondering if the calls be hidden away somehow?
This code to interface with the CCPP is not adhering to the "just add the 5 phase calls and supply metadata for your scheme and it should 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.
Would making another routine (cam_initialize_constituents?) at the end of this module to "hide" this away be sufficient?
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 has been in the system for years. It was discussed and approved way back when the NRL folks met with the CCPP Framework committee and pointed out the lack of support for communicating with the dycore. It is not a new interface. It is not one of the 'required 5' phase calls (so it is technically optional) but models need it to function.
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.
@gold2718 - I don't remember this, but that doesn't mean anything. So you are saying the Dom and Grant approved this, correct, that there are three new optional interfaces to the CCPP?
Thinking about this further, I'd like to propose another possible solution. That would be instead to add an optional "6th" phase that would be analogous to our "register" phase in CAM. I do remember early on you and I discussed this phase and you said we'd implement that phase if needed. @peverwhee, @gold2718 and @nusbaume, would this be a way to hide these three object interfaces?
I'm concerned that if we end up adding additional CCPP-blessed objects in the future that the number of interfaces will proliferate. To me that makes the threshold of host models adopting CCPP higher (with every additional interface that needs to be potentially added).
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.
I think the problem with this approach to initializing advected consitituents is that any interface that hides what is going on has to be compatible with how all the models (e.g., CAM, UFS, NEPTUNE) bring up their dycore structures. Lumping everything together might look 'simpler' from a 'number of CCPP interfaces' perspective but it will lead to more complex implementation in the host models (including CAMDEN) which will then have to parse out the data returned by the new pre-packaged interface and add the data to their internal structures. We can consider this if you get buy-in from NOAA and NRL (more NRL since the UFS still has their advected constituents listed in a separate text file that has to magically work with the CCPP fields at runtime).
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.
opened #215 for continued discussion
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.
Intentionally not resolving this, so the discussion is readily viewable when #215 is worked upon.