-
Notifications
You must be signed in to change notification settings - Fork 26
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
Updates in helper script for updated cupid workflow #176
Conversation
I'll be adding |
…criptions from xml vars
cime_config/config_tool.xml is used by CESM to create env_postprocessing.xml. Variables in this XML file are then used to generate the cupid config file for CESM cases
1. Bring in latest ADF commit as external 2. generate_cupid_config_for_cesm_case.py now has --adf-output-root argument that lets users specify a place other than ${CASEROOT} for ADF_output/ Also cleaned up comments about start_years and end_years in timeseries.py
Keeping with CESM convention, using STARTDATE instead of START_YEAR. This propogates into generate_cupid_config_for_cesm_case.py, which wants to drop the MM-DD portion of the date in favor of saving just the year for timeseries generation (where we assume ENDDATE = YYYY-01-01, so END_YEAR = YYYY - 1)
When you create a new case, CESM will create case.cupid, which in turn will execute the new cesm_postprocessing.sh script -- this means we can update what CUPiD does in the CESM workflow without requiring changes to ccs_config_cesm
This is ready for a final review -- the final summary of changes:
Happy to address any concerns in the implementation, but I think this has all the features we need for |
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 approval seems awfully shady, since half the commits in this PR are from me... but @TeaganKing can't approve it because she opened the PR (and also made the other half of the commits) 😄
Hah! I am reviewing this right now, too. I will be commenting on it shortly... |
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 have just a few minor change requests.
echo "CUPID_RUN_ALL is True, running diagnostics for all components" | ||
fi | ||
|
||
unset PYTHONPATH |
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.
It may make more sense to move this closer to the comment on line 21 (or move the comment) so long as that doesn't mess anything else up?
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.
Good catch! It seems safer to move the comment down than the unset command up :)
cupid_startdate : str | ||
The start date of the case being analyzed ("YYYY-MM-DD"). | ||
|
||
cupid_enddate : int |
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 this one is a str?
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.
Yup, another good catch :)
1. Move comment about unsetting python path to where we unset the path 2. correct comment about cupid_enddate datatype Note: I also removed the ./xmlquery MACH call because we weren't using that anywhere (I wonder if we used to only run some commands based on being on derecho, but now it's all controlled by env_postprocessing.xml)
@TeaganKing hopefully ed9c322 addresses both of your requests (the |
Great! Looks good to me! |
|
||
<!-- Variables that define which components are analyzed --> | ||
|
||
<entry id="CUPID_RUN_ALL"> |
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.
Not a high priority, but do all of these need to be CUPID_RUN_STUFF?
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 idea was that it would be clear that these are cupid-specific variables that turn on running a particular component. So, I wouldn't want to just leave the variable name as only ATM or ICE, for instance. Open to other suggestions, though!
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 leave the variable names as-is for now, we can always change them later :)
"case_name = \"\" # \"b.e30_beta02.BLT1850.ne30_t232.104\"\n", | ||
"base_case_name = \"\" # \"b.e23_alpha17f.BLT1850.ne30_t232.092\"\n", | ||
"base_case_name = None # \"b.e23_alpha17f.BLT1850.ne30_t232.092\"\n", |
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 like this addition.
Description of changes: This PR will include additional parameters in the helper script which generates the cupid configuration file to account for a more flexible cupid workflow.
pre-commit
checks passed (#8 in Adding Notebooks Guide)?