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.

Sign up for GitHub

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

Jump to bottom

fix cf decoding of grid_mapping #9765

Merged
merged 17 commits into from
Nov 14, 2024
Merged

fix cf decoding of grid_mapping #9765

merged 17 commits into from
Nov 14, 2024

Conversation

kmuehlbauer
Copy link
Contributor

@kmuehlbauer kmuehlbauer commented Nov 11, 2024 •
edited
Loading

@kmuehlbauer kmuehlbauer mentioned this pull request Nov 11, 2024
Closed
5 tasks
@kmuehlbauer kmuehlbauer marked this pull request as ready for review November 12, 2024 07:48
@kmuehlbauer
Copy link
Contributor Author

This is ready for review.

cc @observingClouds

@observingClouds
Copy link
Contributor

Thanks @kmuehlbauer for fixing my issue! 🚀I highly appreciate it! I tested your PR and can confirm that the issue I had is now fixed. Well done.

With respect to the tests, I was wondering if they are better included in https://github.com/pydata/xarray/blob/main/xarray/tests/test_conventions.py because conventions.py is where you have made the changes.

@kmuehlbauer
Copy link
Contributor Author

With respect to the tests, I was wondering if they are better included in https://github.com/pydata/xarray/blob/main/xarray/tests/test_conventions.py because conventions.py is where you have made the changes.

Yes, thanks for spotting this. I'll have a look, how to move them..

dcherian
dcherian reviewed Nov 13, 2024
View reviewed changes
xarray/conventions.py Outdated Show resolved Hide resolved
@kmuehlbauer
Copy link
Contributor Author

Nice, didn't manage to outsmart pre-commit hook. 😬

@kmuehlbauer
Copy link
Contributor Author

With respect to the tests, I was wondering if they are better included in https://github.com/pydata/xarray/blob/main/xarray/tests/test_conventions.py because conventions.py is where you have made the changes.

Yes, thanks for spotting this. I'll have a look, how to move them..

I think I need to have the tests there to make them available for all backends. Please correct me, if I'm overseeing anything @dcherian.

@dcherian
Copy link
Contributor

Yeah that is OK. We could just unit test the helper function and assert that coord_names returned is as expected. The backends tests take a while...

@kmuehlbauer
Copy link
Contributor Author

@dcherian Thanks for the review. Shall we keep the backend tests or should I remove them? Otherwise this seems good to go from my end.

@dcherian
Copy link
Contributor

dcherian commented Nov 13, 2024 •
edited
Loading

let's remove it. I think unit tests are enough for this kind of cf-decoding stuff.

@dcherian dcherian added the plan to merge Final call for comments label Nov 13, 2024
@kmuehlbauer
Copy link
Contributor Author

I'm afk now. Will do first thing in the morning, if no one beats me to it.

@kmuehlbauer kmuehlbauer enabled auto-merge (squash) November 13, 2024 22:06
@dcherian dcherian merged commit e674286 into pydata:main Nov 14, 2024
26 of 29 checks passed
dcherian added a commit to dcherian/xarray that referenced this pull request Nov 16, 2024
@dcherian
Merge branch 'main' into custom-groupers
f0f838c 
* main:
  fix cf decoding of grid_mapping (pydata#9765)
  Allow wrapping `np.ndarray` subclasses (pydata#9760)
  Optimize polyfit (pydata#9766)
  Use `map_overlap` for rolling reductions with Dask (pydata#9770)
  fix html repr indexes section (pydata#9768)
dcherian added a commit that referenced this pull request Nov 19, 2024
@dcherian
Merge branch 'main' into backend-indexing
ab20531 
* main: (24 commits)
  Bump minimum versions (#9796)
  Namespace-aware `xarray.ufuncs` (#9776)
  Add prettier and pygrep hooks to pre-commit hooks (#9644)
  `rolling.construct`: Add `sliding_window_kwargs` to pipe arguments down to `sliding_window_view` (#9720)
  Bump codecov/codecov-action from 4.6.0 to 5.0.2 in the actions group (#9793)
  Buffer types (#9787)
  Add download stats badges (#9786)
  Fix open_mfdataset for list of fsspec files (#9785)
  add 'User-Agent'-header to pooch.retrieve (#9782)
  Optimize `ffill`, `bfill` with dask when `limit` is specified (#9771)
  fix cf decoding of grid_mapping (#9765)
  Allow wrapping `np.ndarray` subclasses (#9760)
  Optimize polyfit (#9766)
  Use `map_overlap` for rolling reductions with Dask (#9770)
  fix html repr indexes section (#9768)
  Bump pypa/gh-action-pypi-publish from 1.11.0 to 1.12.2 in the actions group (#9763)
  unpin array-api-strict, as issues are resolved upstream (#9762)
  rewrite the `min_deps_check` script (#9754)
  CI runs ruff instead of pep8speaks (#9759)
  Specify copyright holders in main license file (#9756)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Nov 19, 2024
@dcherian
Merge branch 'main' into groupby-shuffle
d533638 
* main:
  Bump minimum versions (pydata#9796)
  Namespace-aware `xarray.ufuncs` (pydata#9776)
  Add prettier and pygrep hooks to pre-commit hooks (pydata#9644)
  `rolling.construct`: Add `sliding_window_kwargs` to pipe arguments down to `sliding_window_view` (pydata#9720)
  Bump codecov/codecov-action from 4.6.0 to 5.0.2 in the actions group (pydata#9793)
  Buffer types (pydata#9787)
  Add download stats badges (pydata#9786)
  Fix open_mfdataset for list of fsspec files (pydata#9785)
  add 'User-Agent'-header to pooch.retrieve (pydata#9782)
  Optimize `ffill`, `bfill` with dask when `limit` is specified (pydata#9771)
  fix cf decoding of grid_mapping (pydata#9765)
  Allow wrapping `np.ndarray` subclasses (pydata#9760)
  Optimize polyfit (pydata#9766)
  Use `map_overlap` for rolling reductions with Dask (pydata#9770)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian left review comments

Assignees
No one assigned
Labels
plan to merge Final call for comments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

xr.open_dataset(... decode_coords='all') fails for certain CF conform grid_mapping values
3 participants
@kmuehlbauer @observingClouds @dcherian