-
Notifications
You must be signed in to change notification settings - Fork 9
Regrid updates and output fixes #447
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
base: master
Are you sure you want to change the base?
Conversation
- utils/common.py:
- Updated require_module to accept an interval of unsupported versions
- utils/dataset_utils.py:
- Added option to get_coord_by_type to suppress warning about a main variable
not being identifiable
- utils/output_utils.py
- Added functions 'fix_netcdf_attrs_encoding' and '_fix_str_encoding' to
fix utf-8 encoding issues for global and variable attributes
- ops/base_operation.py:
- Applying above fix to correct utf-8 encoding issues
- core/regrid.py:
- xarray version warning correctly triggered for versions
outside of compatible version interval
- Grid.detect_extent:
+ determins now also extent in latitude
+ using now average resolution in x and y direction, respectively,
rather than the average resolution, to determine the extent in lon/lat
direction
+ now returning (lon_extent, lat_extent) instead of just lon_extent
- Grid._grid_from_ds_adaptive:
+ Added fix for regional grids that include either meridian or antimeridian
- Grid.to_netcdf:
+ Added fix for ds.encoding['unlimited_dims'] not being updated by xarray
when dropping all time-dependent variables to write out solely the horizontal grid
+ Applying above fix for utf-8 encoding issues
- Grid.extent_lat: added new attribute
- Weights:
+ Will now use "post_mask_source='domain_edge'" setting when remapping a regional grid
via nearest neighbour method, to avoid extrapolation beyond the original source domain
- Tests:
- Added / modified tests for code changes
- Reverted skipping or xfailing tests that used to fail with engine h5netcdf
Pull Request Test Coverage Report for Build 19869800153Details
💛 - Coveralls |
- Consistent distinction between extent, extent_lon, extent_lat - Adaptive grids with global extent in lon/lat will not exceed -180,180 or 0,360 in lon / -90,90 in lat - Modified tests accordingly - Added tests for yet untested parts of the regrid code to boost coverage
|
ToDo: Replace all |
|
@sol1105 Is this ready for review? I can start looking at it next week if so. |
|
@Zeitsperre Thank you, it would be great if you had a look the changes :) The changes require a feature recently merged into the |
Zeitsperre
left a comment
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.
LGTM. Given that @aulemahal implemented some of these changes in xesmf, I'll throw the ball to him to weigh in on this as well.
| s : str, byte | ||
| The string to be fixed. If the input is not of type str or bytes, | ||
| it is returned as is. | ||
| encoding : str, optional |
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.
optional suggests that encoding can be passed as None. Be sure to handle that case if 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.
def _fix_str_encoding(s, encoding="utf-8"):
[...]
encoding : str, optional
The encoding to be used. Default is "utf-8".
I thought optional only means that else the default is used and if None should be allowed it would be set as
encoding: str or None, optional?
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.
From my understanding, optional and something | None are the same thing. It's a shorthand that resembles the optional you would find in other languages (like C++ or Rust) meaning that the function accepts a Null value.
For more info: https://docs.python.org/3/library/typing.html#typing.Optional
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 think the confusion here stems from the caveat in the standard library: Note that this is not the same concept as an optional argument, which is one that has a default.
When I see ,optional in a docstring, I think of the call signature Optional. It's a bit ambiguous, I suppose.
for more information, see https://pre-commit.ci
|
@Zeitsperre Thanks for having such a thorough look at the changes :) |
|
@aulemahal Many thanks for all the effort you put into getting the @Zeitsperre I think I made all necessary changes and all tests pass. Only for Anaconda python3.10 the environment cannot be built because xesmf does no longer support python 3.10. |
|
Sorry about the drop. I hope this is not problematic on your side ? I chose to drop support in the lack of resources to fix bugs related to older python/xarray/sparse versions. Unless you have objections, I will try to remove xESMF 0.9 and 0.9.1 from PyPi because their "support" of Python 3.10 is only partial. |
|
@sol1105 I think we can either drop Python3.10 (loses security support in a year; https://endoflife.date/python) or we can add a version check for the regridding tools, maybe something like:
I would be fine with us dropping Python3.10; There have been many changes to |
@Zeitsperre I do not have any preference here. Should these changes however be done in a different PR? @cehbrecht What do you think? |
I don't mind when we skip Python 3.10. If this regird PR will be "pending" one could already do the "skip" in another PR and merge it back in here again. What ever is easier for you :) |
### What kind of change does this PR introduce?: <!--(Bug fix, feature, docs update, etc.)--> * Drops support for Python3.10 * Raises the minimum supported `numpy` to v1.26 ### Does this PR introduce a breaking change?: <!--(Has there been an API change? New dependencies?)--> Yes, the minimum Python is now 3.11 and the minimum `numpy` is at the absolute last version before `v2.0`. This is roughly following the [scipy SPEC-0 recommendations](https://scientific-python.org/specs/spec-0000/).
|
@Zeitsperre Do you have any objections against a simple merge commit without squash/rebase? |
Pull Request Checklist:
What kind of change does this PR introduce?:
regrid.ipynbpsymaps/psyplotdependencyutils/common.py:require_moduleto accept an interval of unsupported versionsutils/dataset_utils.py:get_coord_by_typeto suppress warning about a main variable not being identifiableutils/output_utils.py:fix_netcdf_attrs_encodingand '_fix_str_encodingto fix utf-8 encoding issues for global and variable attributesops/base_operation.py:core/regrid.py:xarrayversion warning correctly triggered for versions outside of compatible version intervalGrid.detect_extent:(lon_extent, lat_extent)instead of justlon_extentGrid._grid_from_ds_adaptive:Grid.to_netcdf:ds.encoding['unlimited_dims']not being updated byxarraywhen dropping all time-dependent variables to write out solely the horizontal gridGrid.extent_lat, Grid.extent_lon: added new attributesWeights:post_mask_source='domain_edge'setting when remapping a regional grid via nearest neighbour method, to avoid extrapolation beyond the original source domain (xESMF PR yet to be approved and merged)engine="h5netcdf"Does this PR introduce a breaking change?:
clisops.core.regrid.Grid.detect_extentnow returns the tuple(lon_extent, lat_extent)rather than justlon_extentGrid.extentnow carries the combined extent in lon and lat direction and no longer just the extent in lon direction. If the grid is consideredglobalin extent of both, lon and lat coordinates,Grid.extentis set to"global"else, it is set to"regional".Grid.extent_lonandGrid.extent_latallow accessing the extent in only lon and lat direction, respectively.Other information:
I can update
HISTORY.rstonce all changes made by the PR are approved and potential feedback worked in.There are currently two
xesmfPRs open (#444 #445) that should be merged and axesmfrelease triggered before merging this in.