Skip to content
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

Merged
merged 16 commits into from
Mar 6, 2025

Conversation

TeaganKing
Copy link
Collaborator

@TeaganKing TeaganKing commented Jan 30, 2025

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.

@mnlevy1981
Copy link
Collaborator

I'll be adding cime_config/config_tool.xml, which can be used by CIME to generate env_postprocessing.xml in a CESM case directory. @ekluzek suggested we have preview_namelists generate the config.yml file used by CUPiD in the CESM workflow in RUN_ANALYSIS=TRUE. I don't know what that looks like, but I wanted to leave a comment here to remember to look into it later (it would mean moving some of the setup from ccs_config's template.cupid to a different script that could be invoked by preview_namelists; I think that script would also be in cime_config/).

TeaganKing and others added 5 commits February 7, 2025 13:37
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
@mnlevy1981
Copy link
Collaborator

This is ready for a final review -- the final summary of changes:

  1. Created cime_config/config_tool.xml, which CIME can parse to create env_postprocessing.xml
  2. Created helper_scripts/cesm_postprocessing.py, which will be launched by case.cupid in the CESM workflow
  3. Updated ADF to recent commit
  4. Added several more arguments to generate_cupid_config_for_cesm_case.py because we want to pass variables from the new env_postprocessing.xml to this script
  5. Updated one of the seaice notebooks to allow base_case_output_dir and CESM_output_dir to differ
  6. cleaned up comments about datatypes in timeseries.py

Happy to address any concerns in the implementation, but I think this has all the features we need for cesm3_0_beta06 :)

Copy link
Collaborator

@mnlevy1981 mnlevy1981 left a 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) 😄

@TeaganKing
Copy link
Collaborator Author

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...

Copy link
Collaborator Author

@TeaganKing TeaganKing left a 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
Copy link
Collaborator Author

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?

Copy link
Collaborator

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
Copy link
Collaborator Author

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?

Copy link
Collaborator

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)
@mnlevy1981
Copy link
Collaborator

@TeaganKing hopefully ed9c322 addresses both of your requests (the cesm_postprocessing.sh comment doesn't show as outdated because I moved the comments you referred to rather than the actual line you commented on)

@TeaganKing
Copy link
Collaborator Author

Great! Looks good to me!


<!-- Variables that define which components are analyzed -->

<entry id="CUPID_RUN_ALL">
Copy link
Collaborator

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?

Copy link
Collaborator Author

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!

Copy link
Collaborator

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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this addition.

@mnlevy1981 mnlevy1981 merged commit fc3c638 into NCAR:main Mar 6, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants