-
Notifications
You must be signed in to change notification settings - Fork 168
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
Add run and finalize methods to marine LETKF task #2944
base: develop
Are you sure you want to change the base?
Add run and finalize methods to marine LETKF task #2944
Conversation
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.
We'll probably have to rearrange things between your PR and mine at some point, but this looks good to me. Thanks for doing this @AndrewEichmann-NOAA .
exec_cmd_gridgen.add_default_arg(self.task_config.GRIDGEN_EXEC) | ||
exec_cmd_gridgen.add_default_arg(self.task_config.GRIDGEN_YAML) | ||
|
||
try: |
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 know this will be refactored soon, but maybe use this in the meantime marine_da_utils.py#L18 ?
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.
Why not
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.
Add an issue and maybe also a TODO if you don't want to address this in this PR. This will be refactored when we start making use of the Jedi class.
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.
changed - I forgot about this comment
@@ -82,12 +82,14 @@ declare -rx COM_OCEAN_HISTORY_TMPL=${COM_BASE}'/model/ocean/history' | |||
declare -rx COM_OCEAN_RESTART_TMPL=${COM_BASE}'/model/ocean/restart' | |||
declare -rx COM_OCEAN_INPUT_TMPL=${COM_BASE}'/model/ocean/input' | |||
declare -rx COM_OCEAN_ANALYSIS_TMPL=${COM_BASE}'/analysis/ocean' | |||
declare -rx COM_OCEAN_LETKF_TMPL=${COM_BASE}'/analysis/ocean/letkf' |
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.
What files get written to the letkf directories vs the regular component analysis directories?
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.
These are the analysis files from the LETKF, which are separate from the variational analysis files
env/WCOSS2.env
Outdated
export NTHREADS_MARINEANALLETKF=${NTHREADSmax} | ||
export APRUN_MARINEANALLETKF="${APRUN_default} --cpus-per-task=${NTHREADS_MARINEANALLETKF}" | ||
export NTHREADS_MARINEANLLETKF=${NTHREADSmax} | ||
export APRUN_MARINEANLLETKF="${APRUN_default} --cpus-per-task=${NTHREADS_MARINEANLLETKF}" |
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 is incorrect specification for WCOSS2.
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.
@aerorahul Is there an example you could suggest to work off of? There seem to be a number of choices.
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.
Since JEDI does not yet employ threads, this could simply be:
export APRUN_MARINEANLLETKF="${APRUN_default}"
When threading do come into play,
export APRUN_MARINEANLLETKF="${APRUN_default} --ppn ${tasks_per_node}--cpu-bind depth --depth=${NTHREADS_MARINEANLLETKF}"
You could see the JEDI atm sections of this code for reference.
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.
some minor comments.
approved pending tests passing.
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.
A couple very minor things.
Also need to resolve pynorms failures. |
Co-authored-by: Walter Kolczynski - NOAA <[email protected]>
@AndrewEichmann-NOAA pynorms is still failing:
https://github.com/NOAA-EMC/global-workflow/actions/runs/11376852041/job/31650003088?pr=2944 |
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.
Seem inline comments
exec_cmd_gridgen.add_default_arg(self.task_config.GRIDGEN_EXEC) | ||
exec_cmd_gridgen.add_default_arg(self.task_config.GRIDGEN_YAML) | ||
|
||
try: |
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.
Add an issue and maybe also a TODO if you don't want to address this in this PR. This will be refactored when we start making use of the Jedi class.
def marineanlletkf(self): | ||
|
||
deps = [] | ||
dep_dict = {'type': 'metatask', 'name': f'{self.run}_fcst', 'offset': f"-{timedelta_to_HMS(self._base['cycle_interval'])}"} |
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 should be enkfgdas
, not self.run
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.
changed and tested
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.
Doesn't look like this change got pushed, it is still {self.run}
.
Co-authored-by: Walter Kolczynski - NOAA <[email protected]>
def marineanlletkf(self): | ||
|
||
deps = [] | ||
dep_dict = {'type': 'metatask', 'name': f'{self.run}_fcst', 'offset': f"-{timedelta_to_HMS(self._base['cycle_interval'])}"} |
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.
Doesn't look like this change got pushed, it is still {self.run}
.
Description
Adds run and finalize methods to marine LETKF task, experiment yaml for gw-ci in GDASApp, workflow additions, removes bugs found on the way, and completes the bulk of the work on LETKF task. Conversion of fields to increments pending.
Partially resolves NOAA-EMC/GDASApp#1091 and NOAA-EMC/GDASApp#1251
Mutual dependency with GDASApp PR NOAA-EMC/GDASApp#1287 and IC fix file issue #2944 (comment)
Type of change
Change characteristics
How has this been tested?
Example:
Checklist