Skip to content

add .oindex and .vindex to BackendArray#8885

Merged
dcherian merged 13 commits intopydata:backend-indexingfrom
andersy005:split-explicit-indexing-adapter
Apr 17, 2024
Merged

add .oindex and .vindex to BackendArray#8885
dcherian merged 13 commits intopydata:backend-indexingfrom
andersy005:split-explicit-indexing-adapter

Conversation

@andersy005
Copy link
Member

@andersy005 andersy005 commented Mar 28, 2024

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

kmuehlbauer commented Mar 28, 2024

@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

andersy005 commented Apr 1, 2024

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

andersy005 commented Apr 2, 2024

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

dcherian
dcherian reviewed Apr 3, 2024
View reviewed changes
dcherian
dcherian reviewed Apr 3, 2024
View reviewed changes
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

andersy005 commented Apr 5, 2024

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
Closed
4 tasks
@andersy005 andersy005 changed the base branch from main to backend-indexing April 10, 2024 00:06
@andersy005
Copy link
Member Author

andersy005 commented Apr 10, 2024

@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

andersy005 commented Apr 10, 2024

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

andersy005 commented Apr 10, 2024

@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
@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