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
generate ADF config file #142
generate ADF config file #142
Changes from 15 commits
fd4db19
97565d0
bd92dc1
49a0f99
c9759dc
13c0620
eb25a31
9fd4ca9
9ed14fd
9ba9d6d
ab8d539
db5a6f9
db47343
35a9ebb
8f79704
0388b54
d8a563f
634c2a2
605a59c
a2308d7
677a668
101e1ac
750e005
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.
Is it okay for this value to be
None
? Do we need something likeThere 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.
Following up because I think the github formatting made it hard to see the context of my comment. This is specifically referring to
a_dict["diag_cam_baseline_climo"]["cam_case_name"] = base_case_name
, sincebase_case_name = c_dict["global_params"].get("base_case_name")
-- what is the expected behavior ifbase_case_name
is not inglobal_params
?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.
In the ADF config file,
compare_obs
determines whether the model run is compared to observations or another model run. If this variable is false in the ADF config file, thendiag_cam_baseline_climo
is used and is a required config file section. If not present, ADF fails.I'm curious if we would want to support comparison to obs in addition to model-model comparison-- @brianpm and @justin-richling do you have thoughts on this?
With all that said, I think that we could just use
base_case_name = c_dict["global_params"]["base_case_name"]
instead of the.get()
since it is an expected value for model-model comparisons. I'll update this now.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 we should allow for the model vs obs comparisons. In
adf_info.py
there is the variabledata_name
that gets set for the baseline case name if model vs model, butdata_name
gets set to "Obs" if its a model vs obs case.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'm a bit hesitant to include this feature within this PR if we want to get this in before Wednesday. I think it will require an additional default configuration file from ADF as well as a number of additional parameters in the cupid configuration file (including
compare_obs
) as well as parameters that are added to the script to generate the ADF config file.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.
See issue #150
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.
if base_case_name:
would be consistent with thets_case_names
check above: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
if
statement is not necessary if we expectbase_case_name
to always be in the CUPiD configThere 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.
Just updated this