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

Expand use of .oindex and .vindex #8790

Merged
merged 30 commits into from
Mar 15, 2024
Merged

Expand use of .oindex and .vindex #8790

merged 30 commits into from
Mar 15, 2024

Conversation

andersy005
Copy link
Member

this is a follow-up to previous PRs (#8780 and #8750), continuing the efforts outlined in the plan for decoupling lazy indexing functionality from NamedArray. the primary focus of this PR is the removal of vectorized and orthogonal indexing logic from the __getitem__ method. Now, __getitem__ exclusively handles basic indexers, aligning with points 3 and 4 of the plan:

  • as per point 3, lazy indexing arrays will now implement __getitem__ solely for basic indexing. For orthogonal indexing, .oindex will be used, and for vectorized indexing, .vindex will be utilized.
  • following point 4, IndexingAdapter classes have been updated to consistently implement __getitem__, .oindex, and .vindex
  • 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

@andersy005
Copy link
Member Author

@dcherian, when you get a moment, can you take a look and let me know what you think

@dcherian
Copy link
Contributor

We'll have to figure out the backcompat story for Backends now I guess. Perhaps prototype that approach I laid out, and then we can chat?

dcherian
dcherian reviewed Feb 29, 2024
View reviewed changes
@andersy005 andersy005 marked this pull request as ready for review March 1, 2024 20:07
@andersy005 andersy005 requested a review from dcherian March 1, 2024 20:12
dcherian
dcherian reviewed Mar 3, 2024
View reviewed changes
Copy link
Contributor

@dcherian dcherian left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andersy005 this looks like a good step forward!

It's nice that you haven't needed to touch the backend arrays yet. Will that be the next PR?

The docs failure is real:

File ~/checkouts/readthedocs.org/user_builds/xray/checkouts/8790/xarray/core/indexing.py:489, in ExplicitlyIndexedNDArrayMixin._oindex_get(self, key)
    488 def _oindex_get(self, key):
--> 489     raise NotImplementedError("This method should be overridden")

NotImplementedError: This method should be overridden

...

RuntimeError: Unexpected exception in `/home/docs/checkouts/readthedocs.org/user_builds/xray/checkouts/8790/doc/user-guide/interpolation.rst` line 301

@dcherian dcherian changed the title refactor __getitem__ in Explicitly indexed arrays by removing vectorized and orthogonal indexing logic from it Expand use of .oindex and .vindex Mar 3, 2024
@andersy005
Copy link
Member Author

It's nice that you haven't needed to touch the backend arrays yet. Will that be the next PR?

That's my plan.

The docs failure is real:

I was able to track it down and found that it's a class i had forgotten to update (_ElementwiseFunctionArray)

@andersy005 andersy005 requested a review from dcherian March 7, 2024 17:42
@andersy005
Copy link
Member Author

@dcherian, this is ready for another round of reviews when you get a chance

dcherian
dcherian reviewed Mar 14, 2024
View reviewed changes
dcherian
dcherian reviewed Mar 14, 2024
View reviewed changes
dcherian
dcherian approved these changes Mar 14, 2024
View reviewed changes
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Just some minor suggestions

dcherian
dcherian reviewed Mar 15, 2024
View reviewed changes
dcherian
dcherian reviewed Mar 15, 2024
View reviewed changes
dcherian
dcherian reviewed Mar 15, 2024
View reviewed changes
@dcherian dcherian enabled auto-merge (squash) March 15, 2024 04:06
@dcherian dcherian merged commit c9d3084 into pydata:main Mar 15, 2024
28 of 29 checks passed
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 15, 2024
@dcherian
Merge branch 'main' into state-machine
3c92ddf 
* main: (31 commits)
  correctly encode/decode _FillValues/missing_values/dtypes for packed data (pydata#8713)
  Expand use of `.oindex` and `.vindex` (pydata#8790)
  Return a dataclass from Grouper.factorize (pydata#8777)
  [skip-ci] Fix upstream-dev env (pydata#8839)
  Add dask-expr for windows envs (pydata#8837)
  [skip-ci] Add dask-expr dependency to doc.yml (pydata#8835)
  Add `dask-expr` to environment-3.12.yml (pydata#8827)
  Make list_chunkmanagers more resilient to broken entrypoints (pydata#8736)
  Do not attempt to broadcast when global option ``arithmetic_broadcast=False`` (pydata#8784)
  try to get the `upstream-dev` CI to complete again (pydata#8823)
  Bump the actions group with 1 update (pydata#8818)
  Update documentation for clarity (pydata#8817)
  DOC: link to zarr.convenience.consolidate_metadata (pydata#8816)
  Refactor Grouper objects (pydata#8776)
  Grouper object design doc (pydata#8510)
  Bump the actions group with 2 updates (pydata#8804)
  tokenize() should ignore difference between None and {} attrs (pydata#8797)
  fix: remove Coordinate from __all__ in xarray/__init__.py (pydata#8791)
  Fix non-nanosecond casting behavior for `expand_dims` (pydata#8782)
  Migrate treenode module. (pydata#8757)
  ...
@andersy005 andersy005 deleted the simplify-explicitly-indexed-array-getitem branch March 15, 2024 23:28
@andersy005 andersy005 mentioned this pull request Mar 16, 2024
Merged
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 16, 2024
@dcherian
Merge branch 'main' into flox-quantile
34032ef 
* main: (42 commits)
  correctly encode/decode _FillValues/missing_values/dtypes for packed data (pydata#8713)
  Expand use of `.oindex` and `.vindex` (pydata#8790)
  Return a dataclass from Grouper.factorize (pydata#8777)
  [skip-ci] Fix upstream-dev env (pydata#8839)
  Add dask-expr for windows envs (pydata#8837)
  [skip-ci] Add dask-expr dependency to doc.yml (pydata#8835)
  Add `dask-expr` to environment-3.12.yml (pydata#8827)
  Make list_chunkmanagers more resilient to broken entrypoints (pydata#8736)
  Do not attempt to broadcast when global option ``arithmetic_broadcast=False`` (pydata#8784)
  try to get the `upstream-dev` CI to complete again (pydata#8823)
  Bump the actions group with 1 update (pydata#8818)
  Update documentation for clarity (pydata#8817)
  DOC: link to zarr.convenience.consolidate_metadata (pydata#8816)
  Refactor Grouper objects (pydata#8776)
  Grouper object design doc (pydata#8510)
  Bump the actions group with 2 updates (pydata#8804)
  tokenize() should ignore difference between None and {} attrs (pydata#8797)
  fix: remove Coordinate from __all__ in xarray/__init__.py (pydata#8791)
  Fix non-nanosecond casting behavior for `expand_dims` (pydata#8782)
  Migrate treenode module. (pydata#8757)
  ...
andersy005 added a commit to andersy005/xarray that referenced this pull request Mar 19, 2024
@andersy005
fix bug/unnecessary code introduced in pydata#8790
8d58b6f
dcherian added a commit that referenced this pull request Mar 19, 2024
@andersy005 @dcherian
Implement setitem syntax for .oindex and .vindex properties (#8845)
79272c3 
* Implement setitem syntax for `.oindex` and `.vindex` properties

* Apply suggestions from code review

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

* use getter and setter properties instead of func_get and func_set methods

* delete unnecessary _indexing_array_and_key method

* Add tests for IndexCallable class

* fix bug/unnecessary code introduced in #8790

* add unit tests

---------

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
dcherian added a commit to kmsquire/xarray that referenced this pull request Mar 21, 2024
@dcherian
Merge remote-tracking branch 'upstream/main' into feature/rolling-pad
072e87c 
* upstream/main: (765 commits)
  increase typing annotations coverage in `xarray/core/indexing.py` (pydata#8857)
  pandas 3 MultiIndex fixes (pydata#8847)
  FIX: adapt handling of copy keyword argument in scipy backend for numpy >= 2.0dev (pydata#8851)
  FIX: do not cast _FillValue/missing_value in CFMaskCoder if _Unsigned is provided (pydata#8852)
  Implement setitem syntax for `.oindex` and `.vindex` properties (pydata#8845)
  Support pandas copy-on-write behaviour (pydata#8846)
  correctly encode/decode _FillValues/missing_values/dtypes for packed data (pydata#8713)
  Expand use of `.oindex` and `.vindex` (pydata#8790)
  Return a dataclass from Grouper.factorize (pydata#8777)
  [skip-ci] Fix upstream-dev env (pydata#8839)
  Add dask-expr for windows envs (pydata#8837)
  [skip-ci] Add dask-expr dependency to doc.yml (pydata#8835)
  Add `dask-expr` to environment-3.12.yml (pydata#8827)
  Make list_chunkmanagers more resilient to broken entrypoints (pydata#8736)
  Do not attempt to broadcast when global option ``arithmetic_broadcast=False`` (pydata#8784)
  try to get the `upstream-dev` CI to complete again (pydata#8823)
  Bump the actions group with 1 update (pydata#8818)
  Update documentation for clarity (pydata#8817)
  DOC: link to zarr.convenience.consolidate_metadata (pydata#8816)
  Refactor Grouper objects (pydata#8776)
  ...
@dcamron dcamron mentioned this pull request Apr 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian approved these changes

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

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