-
Notifications
You must be signed in to change notification settings - Fork 6
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
Refactor prep config #336
Refactor prep config #336
Conversation
* develop: Resolve pandas warnings (#322) Add atmospheric 3DVar case (#302) Update to Skylab7 Modules - Part 1 (SLES12) (#319) Towards ensemble hofx support (#298) Change Jedi Log plot and turn off Jb evaluation (#310) passed benchmark change (#303) Use `EXPLICIT_DIFFUSION` as Static Background for `geos_marine` (#301) Fix to check for missing obs in SaveObsDiag (#299) Cloning GEOS_mksi instead for GSI channel records (#297) Update modules and keep static files in shared locations (#296) Addressing missing observations (#286)
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
src/swell/deployment/prepare_config_and_suite/prepare_config_and_suite.py
Outdated
Show resolved
Hide resolved
@Dooruk I gave this a pretty thorough review. Functionally, I think it looks good and I didn't find any bugs. My suggestions above are primarily related to style and, in a few cases, adding additional robustness or clarity. That said, don't let my suggestions hold up this PR. Take a quick look through them and feel free to either directly commit them (I tried to make it a one-click thing using "Suggestions") or just "resolve" the conversation without implementing the changes. Especially for the type hinting stuff (#349), I can always just open a separate PR on that later. |
Thanks for your thorough review @ashiklom, I made most of the modifications you suggested and deferred some to a future PR. This is almost set to merge, except while I started fixing some issues with the CLI I encountered an error with the So |
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 good to me!
Same thing as #324 that @danholdaway introduced with additional fixes.
This shouldn't change (and didn't change as far as I could test) any of the configuration YAML outputs. I tested creating experiments with the defaults/overrides and Callum and I tested the CLI interface.
A few highlights:
task_questions
logic with thesuite_questions
so we don't have to havesuite_questions
in each suite folder.I made a few minor fixes and merged with SLURM changes by @ashiklom (I had to add dashes to avoid whitespace between SLURM directives and added
items
to jinja2 undefined class).So please take a look and test if you can and see if anything breaks. I'm planning on merging this next week.