search

ajdawson / eofs

EOF analysis in Python
http://ajdawson.github.io/eofs/
GNU General Public License v3.0
199 stars 60 forks source link

Fix preserving non-dimension coordinates with latest xarray #124

Closed griverat closed 2 years ago

griverat commented 3 years ago

This fixes #123 by just calling data on the coordinate object.

griverat commented 3 years ago

Seems like the continuous integration needs to be migrated from travis-ci.org to travis-ci.com @ajdawson

ajdawson commented 3 years ago

Thanks for the pull request. It’s been a long time since this repo saw any changes and it needs a bit of updating to infrastructure as you say. I’ll try and sort something out but it probably will take me a little while.

rabernat commented 2 years ago

It would be great if this PR could be merged, since eofs / Xarray interoperability is currently broken.

In this case, since the change is so minor, I feel that merging the PR without CI would be justified. The worst that could happen would be to break Xarray compatibility...which is already broken. 🙃 But I have high confidence that this fix is correct (speaking as an xarray core dev).

rabernat commented 2 years ago

I also found another spot where this (.data) fix is needed. PR to this PR in https://github.com/DangoMelon/eofs/pull/1.

griverat commented 2 years ago

I also found another spot where this (.data) fix is needed. PR to this PR in DangoMelon#1.

Thanks for the heads up! I merged your PR and then applied the same fix to a few more lines, hopefully it is covered enough so this error does not pop up anytime soon.

rabernat commented 2 years ago

hopefully it is covered enough so this error does not pop up anytime soon.

I guess the only way to know for sure is the get the tests running. Have you tried running the eofs test suite?

griverat commented 2 years ago

hopefully it is covered enough so this error does not pop up anytime soon.

I guess the only way to know for sure is the get the tests running. Have you tried running the eofs test suite?

I have run the test suite on master and on this PR branch and both succeed in everything xarray-related (aside from some dask errors). I believe there are no tests that use non-dimension coordinates in the input data, that's why everything passes.

rabernat commented 2 years ago

I believe there are no tests that use non-dimension coordinates in the input data, that's why everything passes.

So let's fix that. Let's add more tests to this PR to cover the bug identified in #123 (plus other potential bugs related to xarray coordinates). Happy to help here.

griverat commented 2 years ago

So let's fix that. Let's add more tests to this PR to cover the bug identified in #123 (plus other potential bugs related to xarray coordinates). Happy to help here.

I have started looking into this file lib/eofs/tests/test_tools.py that seems to test the different interfaces of the package. The test loads the pre-computed solutions that are inside the data folder, so the potential fix would be to add another case that has non-dimension coordinates, this could be just a copy of another test with an added coordinate. Not sure if this is the best route to tackle this issue. Since there is no specific test file to deal with xarray interactions, the error would show up when computing the eofs, which could be somewhat misleading since we are not testing against this specific issue.

rabernat commented 2 years ago

Perhaps you could add additional coordinates to the fixture here?

https://github.com/ajdawson/eofs/blob/603ed8ed86e606fcf8e69a9edc756f81544d4f93/lib/eofs/tests/reference.py#L188-L206