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

Accept more general Integer sizes in reshape #55521

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

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Aug 18, 2024

This PR generalizes the reshape methods to accept Integers instead of Ints, and adds a _reshape_uncolon method for Integer arguments. The current _reshape_uncolon method that accepts Ints is left unchanged to ensure that the inferred types are not impacted. I've also tried to ensure that most Integer subtypes in Base that may be safely converted to Ints pass through that method. I don't particularly like this hard-coding of types, and a better solution is welcome.

The call sequence would now go like this:

reshape(A, ::Tuple{Vararg{Union{Integer, Colon}}}) -> reshape(A, ::Tuple{Vararg{Integer}}) -> reshape(A, ::Tuple{Vararg{Int}}) (fallback)

This lets packages define reshape(A::CustomArray, ::Tuple{Integer, Vararg{Integer}}) without having to implement _reshape_uncolon by themselves (or having to call internal Base functions, as in JuliaArrays/FillArrays.jl#373). reshape calls involving a Colon would convert this to an Integer in Base, and then pass the Integer sizes to the custom method defined in the package.

This PR does not resolve issues like #40076 because this still converts Integers to Ints in the actual reshaping step. However, BigInt sizes that may be converted to Ints will work now:

julia> reshape(1:4, big(2), big(2))
2×2 reshape(::UnitRange{Int64}, 2, 2) with eltype Int64:
 1  3
 2  4

julia> reshape(1:4, big(1), :)
1×4 reshape(::UnitRange{Int64}, 1, 4) with eltype Int64:
 1  2  3  4

Note that the reshape method with Integer sizes explicitly converts these to Ints to avoid self-recursion (as opposed to calling to_shape to carry out the conversion implicitly). In the future, we may want to decide what to do with types or values that can't be converted to an Int.

@jishnub jishnub added the arrays [a, r, r, a, y, s] label Aug 18, 2024
@jishnub jishnub requested a review from nsajko August 18, 2024 10:37
@jishnub jishnub marked this pull request as draft August 18, 2024 11:31
@jishnub jishnub marked this pull request as ready for review August 18, 2024 16:08
@nsajko
Copy link
Contributor

nsajko commented Aug 31, 2024

Fixes #17372

@jishnub
Copy link
Contributor Author

jishnub commented Oct 7, 2024

Gentle bump

2 similar comments
@jishnub
Copy link
Contributor Author

jishnub commented Oct 25, 2024

Gentle bump

@jishnub
Copy link
Contributor Author

jishnub commented Nov 8, 2024

Gentle bump

@jishnub
Copy link
Contributor Author

jishnub commented Nov 20, 2024

@nsajko Could you suggest a way to simplify the methods here without widening the inference results? In particular, what were the type-assertions in _reshape_uncolon achieving, and are there tests for these improvements?

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.

3 participants