Optionally produce coeffs_reconstruct stream for MPAS-O, link coeffs file from database for Omega#448
Conversation
|
@xylar Let me know what you think. Pro of using this approach: no need to run a separate forward step besides those being run. ( Con of using this approach: if there are multiple forward steps for a single test case (corresponding to the config file for which |
3da3ae4 to
b548c38
Compare
|
The plan will be to
|
|
This PR addresses #445 |
b548c38 to
36bad9c
Compare
Input database additionsWhen tests have multiple resolutions, the coefficient files are saved as
|
36bad9c to
2e84e77
Compare
|
@xylar Ready for review when you have a chance |
|
Great, I'll make this a priority. |
xylar
left a comment
There was a problem hiding this comment.
This looks good!
Just two suggestions for keeping the code clean.
Other than that, I think it just needs a little bit of documentation. I'll go ahead and approve on the assumption that goes in.
It doesn't look like more testing is needed on my part but let me know if you'd like some.
| self.write_coeffs_reconstruct = False | ||
| if self.write_coeffs_reconstruct: | ||
| model = self.config.get('ocean', 'model') | ||
| if not model == 'mpas-ocean': |
There was a problem hiding this comment.
For whatever reason, I've been lead to believe this is preferred:
| if not model == 'mpas-ocean': | |
| if model != 'mpas-ocean': |
| if reconstruct_variables is None: | ||
| reconstruct_variables = [] | ||
| for variable in reconstruct_variables: | ||
| if variable in ds.keys(): | ||
| if 'normal' in variable: | ||
| out_var_name = variable.replace('normal', '').lower() | ||
| else: | ||
| out_var_name = variable | ||
| if ( | ||
| f'{out_var_name}Zonal' in ds.keys() | ||
| and f'{out_var_name}Meridional' in ds.keys() | ||
| ): | ||
| return ds | ||
| if mesh_filename is None: | ||
| raise ValueError( | ||
| 'mesh_filename must be provided to ' | ||
| f'open_model_dataset for reconstruction of {variable}' | ||
| ) | ||
| ds_mesh = xr.open_dataset(mesh_filename) | ||
| if coeffs_filename is None: | ||
| raise ValueError( | ||
| 'coeffs_filename must be provided to ' | ||
| f'open_model_dataset for reconstruction of {variable}' | ||
| ) | ||
| ds_coeff = xr.open_dataset(coeffs_filename) | ||
| coeffs_reconstruct = ds_coeff.coeffs_reconstruct | ||
| reconstruct_variable( | ||
| out_var_name, | ||
| ds[variable], | ||
| ds_mesh, | ||
| coeffs_reconstruct, | ||
| ds, | ||
| quiet=True, | ||
| ) | ||
| else: | ||
| raise ValueError( | ||
| f'User requested vector reconstruction for {variable} but ' | ||
| f"it isn't present in the dataset." | ||
| ) |
There was a problem hiding this comment.
Could we move this to a helper function, just to keep things tidy?
There was a problem hiding this comment.
Good idea. I'll make these changes and flesh out the docs. Thanks for reviewing!




The following changes are needed because only MPAS-O produces the coefficients needed to reconstruct zonal and meridional components from edge fields:
coeffs_reconstructto the stream for forward steps with MPAS-O (ensure this capability can only be turned on with MPAS-O)coeffs_reconstructstream to the input database (see Optionally produce coeffs_reconstruct stream for MPAS-O, link coeffs file from database for Omega #448 (comment))coeffs_reconstructstream within the forward step when the model is Omega*Zonal, *Meridionalfields should use the output stream if present or reconstruct the fields using coeffsChecklist
api.md) has any new or modified class, method and/or functions listedTestingcomment in the PR documents testing used to verify the changes