-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update open_dataset backend to ensure compatibility with new explicit index model #7150
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lukasbindreiter for this PR!
I think it works in most cases but maybe not in all cases. CopyOnWriteArray
is not useful for an (immutable) indexed coordinate variable, but wrapping variable data in a MemoryCachedArray
might still be relevant for custom "lazy" indexes. Unfortunately, we don't have any example yet for the latter. Let's go ahead with your fix and address the general case later.
Would it be possible to add a test with a multi-index? Not sure how best to do that as I'm not familiar with this part of Xarray (backends) and how tests are written there.
I added such a test now (which fails with the old version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small suggestion below.
Could you also update whats_new.rst
with your contribution please?
Done 👍 |
Thank you @lukasbindreiter! Merging. I notice that this is your first contribution to Xarray, welcome! |
Based on feedback on this issue: #7139
whats-new.rst
api.rst