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.

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

Compatibility with xarray>=2022.9.0 #276

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

johnomotani
Copy link
Collaborator

@johnomotani johnomotani commented Jan 20, 2023

Following this comment #213 (comment), converting a few members of Region from scalar DataArray to float reduces the number of errors.

WIP because there are still a few errors in the unit tests, and performance is worse - for some unit tests between 1.5x and 2x slower.

Fix #275.

@johnomotani johnomotani added the bugfix Fix for a bug label Jan 20, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #276 (a39b729) into master (280a570) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #276   +/-   ##
=======================================
  Coverage   69.01%   69.01%           
=======================================
  Files          15       15           
  Lines        3208     3208           
  Branches      790      790           
=======================================
  Hits         2214     2214           
  Misses        732      732           
  Partials      262      262           
Impacted Files Coverage Δ
xbout/region.py 83.55% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

xarray behaviour changed, see pydata/xarray#6999
and links from there.
Updates to xarray, meant that 'index' values were not updated when
'coordinate' values were updated. Fix by explicitly updating indexes.
@johnomotani
Copy link
Collaborator Author

I think with this PR the errors are gone with the latest xarray versions, but a significant slow-down is still there. I suspect the culprit is in the region operations - the tests in test_region.py seem to take ~2x as long (in some cases? I haven't checked the whole list carefully), and those operations are used several times in other operations that have slowed down (like interpolate_parallel() and various 2d plotting methods).

xarray made a lot of changes to how they handle 'indexes' recently. I wonder if the newest versions end up creating 'indexes' where they were not created before, adding overhead that way. If so, maybe we can try and avoid that happening in cases where it is not necessary? [E.g. in region.py it might not be necessary to create indexes for the guard-cell DataArrays in _concat_*_guards(), if the indexes can just be used from the main array?]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xarray - compatibility with latest versions
2 participants