pydata / xarray

N-D labeled arrays and datasets in Python
https://xarray.dev
Apache License 2.0
3.63k stars 1.09k forks source link

fix cf decoding of grid_mapping #9765

Closed kmuehlbauer closed 1 week ago

kmuehlbauer commented 1 week ago
kmuehlbauer commented 1 week ago

This is ready for review.

cc @observingClouds

observingClouds commented 1 week ago

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 commented 1 week ago

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..

kmuehlbauer commented 1 week ago

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

kmuehlbauer commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

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

kmuehlbauer commented 1 week ago

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