-
Notifications
You must be signed in to change notification settings - Fork 446
EAMxx: introducing Reproducible Climate Statistics test #7789
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
Conversation
|
aa5d29b to
221dbc2
Compare
|
Minor quibble - the "EST" terminology is redundant if we refer to an "EST test" - so how about we call it something like "ENS" for "ENsemble Statistics" test? |
jgfouca
left a comment
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.
Looks great! Just some style comments
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.
Pull Request Overview
This pull request replaces the MVK (Multivariate Kolmogorov-Smirnov) testing system with a new RCS (Reproducible Climate Statistics) system for NBFB (Not-Bit-For-Bit) testing in EAMxx. The RCS system provides a more comprehensive statistical testing framework with multiple test types and enhanced configurability.
Key changes include:
- Introduction of RCS system test with comprehensive statistical testing capabilities
- New perturbation test mod for creating ensemble perturbations
- Automated documentation generation for the RCS system
Reviewed Changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| components/eamxx/mkdocs.yml | Updated documentation link from MVK to RCS |
| components/eamxx/docs/user/multi-instance-rcs.md | New comprehensive documentation for RCS system |
| components/eamxx/docs/user/multi-instance-mvk.md | Removed old MVK documentation |
| components/eamxx/cime_config/testdefs/testmods_dirs/eamxx/perturb/* | New perturbation testmod for RCS ensemble setup |
| components/eamxx/cime_config/testdefs/testmods_dirs/eamxx/mvkpert/* | Removed old MVK perturbation files |
| components/eamxx/cime_config/config_tests.xml | Updated test configuration from MVKxx to RCS |
| components/eamxx/cime_config/SystemTests/rcs_stats.py | New comprehensive statistical testing framework |
| components/eamxx/cime_config/SystemTests/rcs_perts.py | New perturbation utilities for RCS |
| components/eamxx/cime_config/SystemTests/rcs.py | New RCS system test implementation |
| components/eamxx/cime_config/SystemTests/mvkxx.py | Removed old MVK system test |
| components/eamxx/cime_config/SystemTests/ksxx* | Removed old KS test files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@mahf708 , just wanted to check in on the status of this PR |
|
I'd like to address comments above by Jim, Luca, and Noel. I will do that soon, hopefully before Monday. |
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.
Pull Request Overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0281d6d to
cd06159
Compare
|
@AaronDonahue, this is ready for integration as-is. It would be nice to add some tests to test.py; let's see what @ndkeen recommends. That can be done either here or in a follow-up PR. I'd like @jgfouca, @bartgol, and @ndkeen to take one last look before integration. Thanks! |
| <STOP_N>12</STOP_N> | ||
| <REST_OPTION>never</REST_OPTION> | ||
| <REST_N>$STOP_N</REST_N> | ||
| <HIST_OPTION>$STOP_OPTION</HIST_OPTION> |
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.
@ndkeen note that changing HIST_OPTION will make the test fail by default. There's a workaround to override the hist archive, etc., but I thought I'd rather avoid that potential rabbit hole for now. We are disallowing useless restarts though, so that's a plus!
|
For Jim, only this commit should be reviewed: cd06159 |
jgfouca
left a comment
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.
Much better! Just one minor thing
cd06159 to
7292f39
Compare
bf8fcad to
8102319
Compare
Introducing RCS (and removing MVK) to ATM. Highlights: perturb test mod for creating necessary perturbation RCS system test with rcs_perts (manipulate perturbation) and est_stats for statistical testing rcs_stats can be triggered from the command line too (some details in rcs_stats were generated by bot) note a limitation in cice (in eam and cice buildnml, ntasks are wrong in multi-deriver multi-instance setup #7788) automatically generated docs cocreated with bots [BFB]
Introducing RCS (and removing MVK) to ATM.
Highlights:
docs: https://docs.e3sm.org/E3SM/pr-preview/pr-7789/EAMxx/user/multi-instance-rcs
cocreated with bots
[BFB]