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

Unify reshape methods accepting Colon and OneTo #56850

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Dec 17, 2024

Currently, there are a bunch of reshape methods that do identical things. The methods that accept a dims::Vararg simply forward the Tuple to rehape, and there are specialized methods for Colons. We may unify all these methods and have only one method to handle both OneTos and Colons, such that any combination of these works. Firstly, this addresses some missing combinations of dims, as seen in #41069, and secondly, this would reduce the number of methods that packages need to define to avoid ambiguities when they're specializing on the array type. E.g., the method in https://github.com/JuliaArrays/FillArrays.jl/blob/295266cef8fa38813b0f31bed9c3ddb27a4b4de3/src/FillArrays.jl#L275-L279 and https://github.com/JuliaArrays/OffsetArrays.jl/blob/8decfce50edeeab1694232f80427ea66c6afc023/src/OffsetArrays.jl#L381-L383 seem identical to those in Base, and such ambiguities are better avoided by broadening the specific Base methods.

The current implementation would firstly convert OneTos in dims to Integers, and secondly, convert Colons to integers through the usual route. Finally, we would call reshape(parent::AbstractArray, dims::Tuple{Vararg{Integer}}), or, in common cases, reshape(parent::AbstractArray, dims::Tuple{Vararg{Int}}).

One comment in #41069 was that the method

reshape(parent::AbstractArray, dims::Tuple{Vararg{Union{Integer,Colon,OneTo}}})

should be defined instead as

reshape(parent::AbstractArray, dims::Tuple)

but this would require making _reshape_uncolon public. I've not made this change in this PR, and we may think about this separately.

Supersedes and closes #41069.

This PR also generalizes the method reshape(parent, dims::IntOrInd...) to accept any combinations with Colons. This means that packages would typically not need to define this method, and may only specialize the method that accepts a Tuple of dims. Unfortunately, this would currently break OffsetArrays, as this commits type-piracy and defines the generic method (see line 531 of test/testhelpers/OffsetArrays.jl), but it shouldn't be doing this anyway. A patch to remove this method from OffsetArrays would fix the method overwritten error if this is merged.

@jishnub jishnub force-pushed the jishnub/reshape_unify branch from c7344b4 to ec278ed Compare December 24, 2024 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant