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

run_tower fixes and improvements #2963

Merged
merged 17 commits into from
Feb 18, 2025

Conversation

samsrabin
Copy link
Collaborator

@samsrabin samsrabin commented Feb 13, 2025

Description of changes

Resolves some bugs and improves some messaging. Also, two new options:

  • --no-inputdata-check option implies --setup-only but skips the check/download of input data. This is used instead of --setup-only in run_tower system testing for a speedup of ~20%.
  • --xmlchange option allows user to specify xmlchange settings to apply. E.g., --xmlchange CLM_CO2_TYPE=constant,CCSM_CO2_PPMV=850.

Specific notes

Contributors other than yourself, if any: None

CTSM Issues Fixed:

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Does this create a need to change or add documentation? Did you do so? No

Testing performed, if any: Site/regional Python unit and system tests.

Remaining work

  • Add system test to check that --setup-only is obeyed
  • Add system test to check that --overwrite is obeyed
  • Add system test to check that --xmlchange is obeyed

@samsrabin samsrabin self-assigned this Feb 13, 2025
@samsrabin samsrabin added enhancement new capability or improved behavior of existing capability bug something is working incorrectly bfb bit-for-bit test: python Pass clm_pymods test suite plus Python sys/unit tests before merging labels Feb 13, 2025
@samsrabin
Copy link
Collaborator Author

samsrabin commented Feb 13, 2025

In my testing of ctsm5.3.024, test_sys_run_tower.py takes about 871 seconds (n=2). This PR, as of now, speeds that up by >90% (~82 seconds, n=3). I will mark this PR as closing #2946.

@samsrabin samsrabin marked this pull request as ready for review February 14, 2025 04:47
@samsrabin
Copy link
Collaborator Author

With the new tests, this now takes about 5 minutes (300 seconds). It would be shorter if I didn't feel like I needed to duplicate tests between both NEON and PLUMBER sites. @ekluzek and @TeaganKing, would it be crazy for me to try and get rid of the separate NeonSite and Plumber2Site classes, and instead just implement those differences within TowerSite? It seems simple to do, but I feel like maybe I'm missing something.

@TeaganKing
Copy link
Contributor

Hi @samsrabin , We were hoping to implement the NeonSite and Plumber2Site classes separately in order to have clearer parts of the code that were shared versus specific to each type of tower. We also wanted to set this up so that we could eventually create more generic tower site classes parallel to those types of classes. Thus, while it's feasible to merge them back together, I think it's preferable to keep these classes as child classes that are separate from the TowerSite class.

@samsrabin
Copy link
Collaborator Author

@TeaganKing Yup, makes sense—that's the context I was missing. Thanks!

@TeaganKing
Copy link
Contributor

Thanks Sam! I'll review this hopefully later this afternoon or maybe Tuesday!

@samsrabin
Copy link
Collaborator Author

samsrabin commented Feb 15, 2025

(Couldn't stop thinking about simplifying, but I recognize that's a bigger task, so I started a new branch here. It keeps the tower child classes but removes some almost-identical member functions. Will submit a PR for that only after this one is approved, because it branches from this one.)

Copy link
Contributor

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

Requesting a few minor changes that don't actually change the functionality of this PR, but this looks great! Thanks so much for putting this together!

@samsrabin samsrabin merged commit 0d44388 into ESCOMP:b4b-dev Feb 18, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bfb bit-for-bit bug something is working incorrectly enhancement new capability or improved behavior of existing capability test: python Pass clm_pymods test suite plus Python sys/unit tests before merging
Projects
None yet
2 participants