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.

Sign up for GitHub

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

Jump to bottom

add .oindex and .vindex to BackendArray #8885

Merged
merged 13 commits into from
Apr 17, 2024
Merged

add .oindex and .vindex to BackendArray #8885

merged 13 commits into from
Apr 17, 2024

Conversation

andersy005
Copy link
Member

this PR builds towards

the primary objective is to partially address

  1. Implement fall back .oindex, .vindex properties on BackendArray base class. These will simply rewrap the key tuple with the appropriate *Indexer object, and pass it on to __getitem__ or __setitem__. These methods will also raise DeprecationWarning so that external backends will know to migrate to .oindex, and .vindex over the next year.
  • Closes #xxxx
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@kmuehlbauer
Copy link
Contributor

@andersy005 When will this finally take effect? I've a bunch of backends out there, can I do something prematurely to adapt this? Or should I wait for the DeprecationWarnings to kick in? Thanks and sorry for the noise.

@andersy005
Copy link
Member Author

thank you for chiming in @kmuehlbauer!

When will this finally take effect? I've a bunch of backends out there, can I do something prematurely to adapt this? Or should I wait for the DeprecationWarnings to kick in? Thanks and sorry for the noise.

there's still quite a bit of work that needs to be implemented to make the switch an pleasant experience. So, i would say it might worth waiting until we've implemented the deprecation warnings. i will ping you once i have everything ready in xarray proper so you can take a look

@andersy005 andersy005 marked this pull request as ready for review April 1, 2024 21:01
@andersy005 andersy005 requested a review from dcherian April 1, 2024 21:01
@andersy005
Copy link
Member Author

@dcherian, this is ready for review, when you get a chance

dcherian
dcherian reviewed Apr 3, 2024
View reviewed changes
xarray/core/indexing.py Outdated Show resolved Hide resolved
andersy005 and others added 2 commits April 3, 2024 13:54
@andersy005 @dcherian
Update xarray/core/indexing.py
3270b7a 
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
@andersy005 @dcherian
Update ZarrArrayWrapper class in xarray/backends/zarr.py
889dcf8 
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
@dcherian
Copy link
Contributor

dcherian commented Apr 4, 2024

@andersy005 two thoughts:

  1. It'd be nice to reduce the boilerplate for external backends, but this isn't very actionable at the moment. Just something to keep in mind.
  2. Second, it'd be nice to not release this till we are sure of what approach we want the backends to take.

Given (2), what do you think of keeping this branch open, and then opening PRs against this branch (e.g.#8870)

@andersy005 andersy005 marked this pull request as draft April 5, 2024 18:11
@andersy005
Copy link
Member Author

Given (2), what do you think of keeping this branch open, and then opening PRs against this branch (e.g.#8870)

i'm with you on this. in light of #8909, it is worth consolidating the changes against this branch and merging this into main once we are confident everything is ready.

@andersy005 andersy005 mentioned this pull request Apr 8, 2024
Open
4 tasks
@andersy005 andersy005 changed the base branch from main to backend-indexing April 10, 2024 00:06
@andersy005
Copy link
Member Author

@dcherian, i created a new feature branch backend-indexing, and changed the base branch for this PR to be this new brnach. unfortunately, the CI is exclusively enabled for PR against main. so, i'm not sure how we want to handle this.

this is my first time opening PRs against feature branches in pydata/xarray. is there one or two examples of previous PRs that have used this type of workflow in the past?

@andersy005
Copy link
Member Author

unfortunately, the CI is exclusively enabled for PR against main. so, i'm not sure how we want to handle this.

i temporary enabled the CI triggers on the feature branch....

@andersy005 andersy005 marked this pull request as ready for review April 10, 2024 00:45
@andersy005
Copy link
Member Author

@dcherian, let me know if you deem this ready to be merged into the feature branch

@dcherian dcherian merged commit 10c133b into pydata:backend-indexing Apr 17, 2024
26 checks passed
@andersy005 andersy005 deleted the split-explicit-indexing-adapter branch April 30, 2024 12:12
andersy005 added a commit that referenced this pull request May 10, 2024
@andersy005 @dcherian
add .oindex and .vindex to BackendArray (#8885)
ddd4cdb 
* add .oindex and .vindex to BackendArray

* Add support for .oindex and .vindex in H5NetCDFArrayWrapper

* Add support for .oindex and .vindex in NetCDF4ArrayWrapper, PydapArrayWrapper, NioArrayWrapper, and ZarrArrayWrapper

* add deprecation warning

* Fix deprecation warning message formatting

* add tests

* Update xarray/core/indexing.py

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>

* Update ZarrArrayWrapper class in xarray/backends/zarr.py

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>

---------

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
andersy005 added a commit to hmaarrfk/xarray that referenced this pull request May 10, 2024
@andersy005
Merge branch 'backend-indexing' into tuples_backend_indexing
d1edd15 
* backend-indexing:
  Trigger CI only if code files are modified. (pydata#9006)
  Enable explicit use of key tuples (instead of *Indexer objects) in indexing adapters and explicitly indexed arrays (pydata#8870)
  add `.oindex` and `.vindex` to `BackendArray` (pydata#8885)
  temporary enable CI triggers on feature branch
  Avoid auto creation of indexes in concat (pydata#8872)
  Fix benchmark CI (pydata#9013)
  Avoid extra read from disk when creating Pandas Index. (pydata#8893)
  Add a benchmark to monitor performance for large dataset indexing (pydata#9012)
  Zarr: Optimize `region="auto"` detection (pydata#8997)
  Trigger CI only if code files are modified. (pydata#9006)
  Fix for ruff 0.4.3 (pydata#9007)
  Port negative frequency fix for `pandas.date_range` to `cftime_range` (pydata#8999)
  Bump codecov/codecov-action from 4.3.0 to 4.3.1 in the actions group (pydata#9004)
  Speed up localize (pydata#8536)
  Simplify fast path (pydata#9001)
  Add argument check_dims to assert_allclose to allow transposed inputs (pydata#5733) (pydata#8991)
  Fix syntax error in test related to cupy (pydata#9000)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andersy005 @kmuehlbauer @dcherian