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

refactor indexing.py: introduce .oindex for Explicitly Indexed Arrays #8750

Merged
merged 19 commits into from
Feb 23, 2024

Conversation

andersy005
Copy link
Member

@andersy005 andersy005 commented Feb 14, 2024

this pull request represents an initial effort to refactor the indexing logic within indexing.py by separating it from variable.py. to enhance the flexibility of explicitly indexed array classes, this PR introduces orthogonal and vectorized indexing methods—.oindex[key] and .vindex[key[. these changes are a part of a broader initiative aimed at enabling indexing support for namedarray

  • Towards NamedArray tracking issue #8238
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

edit 1:

  • to minimize the blast radius of this PR, i decided to just focus on oindex[key] method in this PR. i intend to introduce vindex[key] in a separate PR

TODO:

@andersy005 andersy005 changed the title refactor indexing.py: introduce .oindex and .vindex for Explicitly Indexed Arrays refactor indexing.py: introduce .oindex and .vindex for Explicitly Indexed Arrays Feb 14, 2024
xarray/core/indexing.py Outdated Show resolved Hide resolved
xarray/core/variable.py Outdated Show resolved Hide resolved
xarray/core/indexing.py Outdated Show resolved Hide resolved
@andersy005 andersy005 changed the title refactor indexing.py: introduce .oindex and .vindex for Explicitly Indexed Arrays refactor indexing.py: introduce .oindex for Explicitly Indexed Arrays Feb 15, 2024
xarray/core/variable.py Outdated Show resolved Hide resolved
@andersy005 andersy005 marked this pull request as ready for review February 16, 2024 01:27
def __getitem__(self, key):
return self.func(key)


class BasicIndexer(ExplicitIndexer):
Copy link
Contributor

Choose a reason for hiding this comment

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

drive-by comment: Our TODO list should include migrating these indexer objects to namedarray/indexer.py or something (eventually)

@dcherian
Copy link
Contributor

Do you know why there are no changes to the backend arrays yet? Those too use *Indexer objects, and we want to break that out. However now I really there's a backwards compatibility problem there. Can you take a look at cfgrib/rioxarray/tiledb-python to see how they use these lazy arrays and indexing please?

@andersy005
Copy link
Member Author

andersy005 commented Feb 16, 2024

Can you take a look at cfgrib/rioxarray/tiledb-python to see how they use these lazy arrays and indexing please?

after taking a look at the respective repos, it's my understanding both the rioxarray and cfrib projects use the explicit_indexing_adapter function. this function acts a bridge, connecting high-level *Indexer objects with backend-specific indexing methods. thanks to each backend wrapping its BackendArray within a LazilyIndexedArray (which supports the oindex[]), everything functions smoothly since we are still passing *Indexer objects within the xr.Variable.__getitem__. however, i anticipate that modifications will be necessary when we begin to directly pass the underlying tuple to methods like __getitem__, oindex, and vindex within xr.Variable.__getitem__

on the other hand, TileDB-CF-Py seems to bypass this pattern, handling the getitem method differently by using its unique logic for processing the underlying tuple. It still wraps its BackendArray within a LazilyIndexedArray though.

cc @dcherian

xarray/core/indexing.py Outdated Show resolved Hide resolved
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.

Thanks, this generally seems like a step in the right direction

@andersy005 andersy005 merged commit d9760f3 into pydata:main Feb 23, 2024
29 checks passed
@andersy005 andersy005 deleted the refactor-indexing branch February 23, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants