fix: handle indexing changes in new xarray versions #159
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
=======================================
Coverage ? 94.44%
=======================================
Files ? 73
Lines ? 6225
Branches ? 0
=======================================
Hits ? 5879
Misses ? 346
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Awesome @slevang, thanks for digging into this! :) Just a minor thing regarding the "mode".
As for the poetry.lock I have no preference tbh. I started using Poetry 2 years ago as it seemed to be the future to me, but not sure anymore about it ;) I've already thought about dropping it all together and using conda together with a requirements file. In case you have a strong preference for any of these two options or even another way, I'm super open :)
xeofs/models/eof.py
Outdated
| # Handle scalar mode | ||
| if "mode" not in scores.dims: | ||
| scores = scores.expand_dims("mode") | ||
|
|
There was a problem hiding this comment.
Perhaps it's better to add the "mode" dimensions in the _BaseModel class, since we also remove the "mode" dim there afterward? Plus other models can inherit this behaviour :)
There was a problem hiding this comment.
Of course, good idea, I had only followed the traceback to where I hit the error and added a quick fix.
xeofs/models/mca.py
Outdated
| # Handle scalar mode | ||
| if "mode" not in scores1.dims: | ||
| scores1 = scores1.expand_dims("mode") | ||
| if "mode" not in scores2.dims: | ||
| scores2 = scores2.expand_dims("mode") | ||
|
|
There was a problem hiding this comment.
same here like in the eof.py module - probably better to move it to inverse_transform within the _base_cross_model.py.
|
Yeah no strong opinion about poetry, I think it works fine. Personally I use conda environments with pip to install packages for the most part. One thing I would say is that running CI on unconstrained latest versions of dependencies is quite beneficial to get ahead of compatibility issues. Basing CI off the fixed lock file makes that challenging, so one option would be to just remove the lock file from git. Without that I believe poetry will solve for latest compatible dependencies. |
Yeah good point that seems indeed smarter. |
Finally figured out a solution to the indexing woes with serialization of multiindexes on newer xarray versions.
In updating to >2023.12.0, we were also getting a failure when doing
xr.dotacrossdim="mode"in the inverse transforms when selecting out a singleton mode.@nicrie not sure if you have a preference for how to update
poetry.lock, I aimed for minimal changes here by just relaxing the xarray version constraint and bumping to latest in the lock file.