Skip to content

Array: Fix NEP18 dispatching in finalize#7508

Merged
jsignell merged 2 commits intodask:mainfrom
gjoseph92:array/concatenate3-nep18-dispatching
Apr 21, 2021
Merged

Array: Fix NEP18 dispatching in finalize#7508
jsignell merged 2 commits intodask:mainfrom
gjoseph92:array/concatenate3-nep18-dispatching

Conversation

@gjoseph92
Copy link
Copy Markdown
Collaborator

#4669 was meant to do this, but it wasn't working anymore. Turns out arr.__array_function__ is not type(arr).__array_function__, even when arr is a plain ndarray. So I believe we've actually being going down the _concatenate2 path for all array types, even plain ndarrays where we could do the more efficient fill-in-the-blocks method of concatenate3.

  • Tests added / passed
  • Passes black dask / flake8 dask / isort dask

dask#4669 was meant to do this, but it wasn't working anymore. Turns out `arr.__array_function__ is not type(arr).__array_function__`, even when `arr` is a plain ndarray. So I believe we've actually being going down the `_concatenate2` path for all array types, even plain ndarrays where we could do the more efficient fill-in-the-blocks method of `concatenate3`.
@github-actions github-actions bot added the array label Apr 2, 2021

for (idx, arr) in zip(slices_from_chunks(chunks), core.flatten(arrays)):
for (idx, arr) in zip(
slices_from_chunks(chunks), core.flatten(arrays, container=(list, tuple))
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Adding container=(list, tuple) to flatten here is a driveby. Seemed weird to me that we gave it earlier in the function but not here. Not sure if there's any reason for the inconsistency, so I made it consistent.

@gjoseph92
Copy link
Copy Markdown
Collaborator Author

FYI @jsignell I found this in the process of pulling the ndarray-subclass commits out #7491 (comment)

Copy link
Copy Markdown
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks!

@jsignell jsignell merged commit ed983bc into dask:main Apr 21, 2021
@gjoseph92 gjoseph92 deleted the array/concatenate3-nep18-dispatching branch April 21, 2021 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants