Skip to content

Clean up backend indexing some more #10376

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented May 30, 2025

Harmonizes some internal code paths to make the async work less painful (#10327).

  1. All "decoding arrays" - BoolTypeArray, StackedBytesArray, NativeEndiannessArray - now support lazy indexing. Previously indexing these arrays loaded them to memory. I think the type stability is a good win, at least it makes debugging less confusing
  2. Our backends now always return LazilyIndexedArray(BackendArray). This was true for all backends except Scipy. Now the Scipy backend does the same. This ends up fixing ScipyArrayWrapper' object has no attribute 'oindex' #8909

Closes #8909
Closes #8921

I added a new CI with only scipy as the optional dependency to prevent regressing on #8909

I'll add comments for the remaining changes

for k, v in self._variables.items():
v = v.copy(deep=True)
res[k] = v
v._data = indexing.LazilyIndexedArray(v._data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Harmonizes this test backend with all the other backends

@@ -190,7 +190,7 @@ def ds(self):
def open_store_variable(self, name, var):
return Variable(
var.dimensions,
ScipyArrayWrapper(name, self),
indexing.LazilyIndexedArray(ScipyArrayWrapper(name, self)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

harmonized with the other backends

@dcherian dcherian marked this pull request as draft May 30, 2025 05:13
@github-actions github-actions bot added CI Continuous Integration tools dependencies Pull requests that update a dependency file Automation Github bots, testing workflows, release automation labels Jun 17, 2025
@dcherian dcherian marked this pull request as ready for review June 18, 2025 01:10
@dcherian dcherian requested a review from kmuehlbauer June 18, 2025 01:11
@dcherian dcherian requested a review from TomNicholas June 18, 2025 01:15
Copy link
Member

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

Awesome @dcherian !

The docs failure looks like it might be catching something real though

Extension error:
--
2043 | Cell raised uncaught exception:
2044 | �[31mAttributeError�[39m�[31m:�[39m '_ElementwiseFunctionArray' object has no attribute 'transpose'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Automation Github bots, testing workflows, release automation CI Continuous Integration tools dependencies Pull requests that update a dependency file io topic-backends topic-CF conventions topic-documentation topic-indexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScipyArrayWrapper' object has no attribute 'oindex'
3 participants