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

reshape error on Julia master #373

Open
matthias314 opened this issue Aug 16, 2024 · 7 comments
Open

reshape error on Julia master #373

matthias314 opened this issue Aug 16, 2024 · 7 comments

Comments

@matthias314
Copy link

I discovered the following issue with the OffsetArrays tests, see JuliaArrays/OffsetArrays.jl#353. @nsajko suggested that I report it here and mention JuliaLang/julia#54261. The error happens both with FillArrays v1.11.0 and master:

julia> using FillArrays

julia> reshape(Fill(2,6), big(2), :)
ERROR: TypeError: in typeassert, expected Tuple{Vararg{Int64}}, got a value of type Tuple{BigInt}
Stacktrace:
 [1] _reshape_uncolon
   @ ./reshapedarray.jl:135 [inlined]
 [2] reshape(parent::Fill{Int64, 1, Tuple{Base.OneTo{Int64}}}, dims::Tuple{BigInt, Colon})
   @ FillArrays ~/.julia/packages/FillArrays/Zr8Pf/src/FillArrays.jl:269
 [3] reshape(::Fill{Int64, 1, Tuple{Base.OneTo{Int64}}}, ::BigInt, ::Colon)
   @ FillArrays ~/.julia/packages/FillArrays/Zr8Pf/src/FillArrays.jl:265
 [4] top-level scope
   @ REPL[4]:1
Julia Version 1.12.0-DEV.1053
Commit 6916eb74205 (2024-08-15 23:07 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 24 × Intel(R) Xeon(R) CPU E5-2430 0 @ 2.20GHz
  WORD_SIZE: 64
  LLVM: libLLVM-18.1.7 (ORCJIT, sandybridge)
Threads: 1 default, 0 interactive, 1 GC (on 24 virtual cores)
@nsajko
Copy link

nsajko commented Aug 16, 2024

FTR, it's usually better to move an existing issue to the proper repo, than to create a new one. That's what I was asking for. Someone who's a member of JuliaArrays should be able to do that. Not that it matters much now.

@matthias314
Copy link
Author

Sorry, didn't know that!

@jishnub
Copy link
Member

jishnub commented Aug 17, 2024

I think reshape in Base should handle all Integers, and not just Ints. In that case, we won't need to define these specialized methods here at all.

@nsajko
Copy link

nsajko commented Aug 17, 2024

I agree. While looking into fixing this, I stumbled upon many open tickets for reshape on Julia's issue tracker, which seem like they should somewhat be considered together. E.g.:

JuliaLang/julia#17372

JuliaLang/julia#27474

JuliaLang/julia#39123

JuliaLang/julia#39951

JuliaLang/julia#40076

JuliaLang/julia#45589

It's a bit overwhelming TBH.

@dlfivefifty
Copy link
Member

Is this bug also in Julia v1.11-rc?

I think it’s a mistake to try to support master as it’s always too much of a moving target

@nsajko
Copy link

nsajko commented Aug 17, 2024

The bug is that FillArrays relies on internal Julia functionality, _reshape_uncolon from Base, specifically. This is independent of Julia version, clearly.

It only manifests on nightly, though.

jishnub added a commit to JuliaLang/julia that referenced this issue Dec 5, 2024
This PR generalizes the `reshape` methods to accept `Integer`s instead
of `Int`s, and adds a `_reshape_uncolon` method for `Integer` arguments.
The current `_reshape_uncolon` method that accepts `Int`s 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 `Int`s pass through that method.

The call sequence would now go like this:
```julia
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 `Integer`s to `Int`s in the actual reshaping step. However,
`BigInt` sizes that may be converted to `Int`s will work now:
```julia
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 `Int`s 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`.

---------

Co-authored-by: Neven Sajko <s@purelymail.com>
@jishnub
Copy link
Member

jishnub commented Dec 8, 2024

This issue is now resolved on nightly, but there's a separate error that should be solved by a LineraAlgebra bump including JuliaLang/LinearAlgebra.jl@b6f87af. In any case, most of the reshape specializations in this package should be unnecessary on nightly, so I'll create a PR to remove these. We may merge that once v1.12 moves to a more stable stage, perhaps RC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants