Unify reshape
methods accepting Colon
and OneTo
#56850
Open
+216
−44
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, there are a bunch of
reshape
methods that do identical things. The methods that accept adims::Vararg
simply forward theTuple
torehape
, and there are specialized methods forColon
s. We may unify all these methods and have only one method to handle bothOneTo
s andColon
s, such that any combination of these works. Firstly, this addresses some missing combinations ofdims
, 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 inBase
, and such ambiguities are better avoided by broadening the specificBase
methods.The current implementation would firstly convert
OneTo
s indims
toInteger
s, and secondly, convertColon
s to integers through the usual route. Finally, we would callreshape(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
should be defined instead as
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 withColon
s. This means that packages would typically not need to define this method, and may only specialize the method that accepts aTuple
ofdims
. Unfortunately, this would currently breakOffsetArrays
, as this commits type-piracy and defines the generic method (see line 531 oftest/testhelpers/OffsetArrays.jl
), but it shouldn't be doing this anyway. A patch to remove this method fromOffsetArrays
would fix the method overwritten error if this is merged.