Convert marine LETKF task to use JCB #3381
Conversation
|
@DavidNew-NOAA |
There was a problem hiding this comment.
Mostly comments on how the JCB template is rendered.
These changes are going to be required for when we get around to refactoring marine_analysis.py and marine_letkf.py to use the Jedi class. When using the Jedi class, the JCB template is rendered by a single line. That means that j2-yamls need to do most of the work and not python.
See for example:
See also:
and:
global-workflow/ush/python/pygfs/jedi/jedi.py
Line 138 in d38841f
| letkf_yaml.save(self.task_config.letkf_yaml_file) | ||
| #################################################################################################### | ||
|
|
||
| envconfig_jcb = copy.deepcopy(self.task_config) |
There was a problem hiding this comment.
I don't understand the reason to make a copy of task_config. I know this is what marine_analysis.py does too, but with the atmosphere, aerosols, and snow, the JCB base and algo yamls are all parsed using task_config. I think at least for consistency we should do it this way for the marine jobs too.
| #################################################################################################### | ||
|
|
||
| envconfig_jcb = copy.deepcopy(self.task_config) | ||
| envconfig_jcb['cyc'] = int(os.getenv('cyc')) |
There was a problem hiding this comment.
cyc and PDY should already be in task_config
| envconfig_jcb = copy.deepcopy(self.task_config) | ||
| envconfig_jcb['cyc'] = int(os.getenv('cyc')) | ||
| envconfig_jcb['PDY'] = self.task_config.current_cycle.strftime('%Y%m%d') | ||
| envconfig_jcb['window_length'] = f"PT{self.task_config['assim_freq']}H" |
There was a problem hiding this comment.
In general, if you use task_config to render the JCB template, environment variables required for the rendering should be added to task_config in the constructor for the MarineLETKF class, like MARINE_WINDOW_BEGIN etc are, for example.
| jcb_config = {**jcb_base_config, **jcb_algo_config} | ||
|
|
||
| # convert datetime to string | ||
| jcb_config['window_begin'] = self.task_config.MARINE_WINDOW_BEGIN.strftime('%Y-%m-%dT%H:%M:%SZ') |
There was a problem hiding this comment.
Any JCB variables like this should be set in the JCB base yaml. In fact, window_begin and window_length are are already set in https://github.com/NOAA-EMC/GDASApp/blob/583e630a14d799495ea41147004916a9b6b81379/parm/soca/marine-jcb-base.yaml#L20
| jcb_base_config = Template.substitute_structure(jcb_base_config, TemplateConstants.DOUBLE_CURLY_BRACES, envconfig_jcb.get) | ||
| jcb_base_config = Template.substitute_structure(jcb_base_config, TemplateConstants.DOLLAR_PARENTHESES, envconfig_jcb.get) | ||
| jcb_algo_config = parse_j2yaml(path=jcb_algo_yaml, data=envconfig_jcb) | ||
| jcb_algo_config = Template.substitute_structure(jcb_algo_config, TemplateConstants.DOUBLE_CURLY_BRACES, envconfig_jcb.get) |
There was a problem hiding this comment.
If I understand this correctly, the need for the Template class is to render this line https://github.com/NOAA-EMC/GDASApp/blob/583e630a14d799495ea41147004916a9b6b81379/parm/soca/marine-jcb-3dfgat.yaml.j2#L5 ? If so, why can't obs be set directly in the algo yaml (marine-jcb-3dfgat.yaml)? In the atmosphere, aero, and snow, we set them directly, such as here: https://github.com/NOAA-EMC/GDASApp/blob/583e630a14d799495ea41147004916a9b6b81379/parm/atm/jcb-prototype_3dvar.yaml.j2#L7
|
|
||
| # Render the full JEDI configuration file using JCB | ||
| jedi_config = render(jcb_config) | ||
| jedi_config['observations'] = observers |
There was a problem hiding this comment.
I think by the time the JCB render method is called, this jedi_config dict should be complete and ready to render into a yaml. You should find some way to set the observations key using JCB templating directly.
Description
Converts marine LETKF to use JCB instead of GDASApp yamls for configuration, to bring in line with other tasks.
Resolves NOAA-EMC/GDASApp#1485
Needs NOAA-EMC/jcb-algorithms#11 merged
Type of change
Change characteristics
How has this been tested?
Clone, build, and run C48mx500_hybAOWCDA GDASApp ctests on Hera
Checklist