Skip to content
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

Data override ext weights #1556

Merged
merged 27 commits into from
Aug 2, 2024

Conversation

uramirez8707
Copy link
Contributor

@uramirez8707 uramirez8707 commented Jul 22, 2024

Description
This PR takes in #1534 and makes the changes needed to data_override and horiz_interp to read in the weight files and fill in the horiz_interp type

Currently, this has only been implemented for bilinear.

Fixes #1399

How Has This Been Tested?
CI, including new test

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes

rem1776
rem1776 previously approved these changes Jul 23, 2024
Copy link
Contributor

@bensonr bensonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this provides a workaround for Issue #1537, I would prefer we get that fix into the same release to ensure this workaround doesn't become the standard method.

- **next_file_name:** The name of the third file in the set
- **external_weights:** The external weights parent key. **Required** only if it is desired to use external weights from file
- **file_name:** Name of the file where the external weights are located, including the directory
- **source:** Name of the source that generated the external weights. The acceptable values are "esmf" and "fregrid"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we supporting ESMF weight files? I had put in PR#1399 that we were only supporting fregrid-generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the documentation

- **multi_file:** The multifile parent key. **Required** only if it is desired to use multiple(3) input netcdf files instead of 1. Note that **file_name** must be the second file in the set when using multiple input netcdf files
- **prev_file_name:** The name of the first file in the set
- **next_file_name:** The name of the third file in the set
- **external_weights:** The external weights parent key. **Required** only if it is desired to use external weights from file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the only supported method for external weights is bilinear. Is the interp_method used in conjunction with the external_weights to determine the method?

It seems we may be opening things up to undetermined behavior using the interp_method in conjunction with the external_weights. Is there a method or some other global attribute in the file we could use to compare against the chosen interp_method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a method or some other global attribute in the file we could use to compare against the chosen interp_method

The only thing that we currently do is check that the weight file has the expected dimensions for the interp method. It can be done more rigorous though.

- **interpol_method:** Method used to interpolate the field. The acceptable values are "bilinear", "bicubic", and "none". "none" implies that the field in the file is already in the model grid. The LIMA format is no longer supported. **Required** only if overriding from a file
- **grid_name:** Name of the grid to interpolate the data to. The acceptable values are "ICE", "OCN", "ATM", and "LND"
- **fieldname_in_model:** Name of the field as it is in the code to interpolate.
- **override_file:** The parent key containing the nested keys related to overriding data from file. **Required** only if overriding from a file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerned the use of parent and nested keys here could be confused with the idea of parent and nested grids. Is the standardized YAML terminology to use parent and nest or can we define it as we want?

Copy link
Contributor

@bensonr bensonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uramirez8707 - appreciate you changing the language according to my suggestions.

@rem1776 rem1776 merged commit 33a87c6 into NOAA-GFDL:main Aug 2, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reading remapping weights generated by fregrid/ESMF into data_override
4 participants