Skip to content

fixes to colonful reshape#54261

Merged
oscardssmith merged 3 commits intoJuliaLang:masterfrom
nsajko:zerolength_colon_reshape
May 13, 2024
Merged

fixes to colonful reshape#54261
oscardssmith merged 3 commits intoJuliaLang:masterfrom
nsajko:zerolength_colon_reshape

Conversation

@nsajko
Copy link
Copy Markdown
Member

@nsajko nsajko commented Apr 26, 2024

Fixes #54245

@nsajko nsajko added arrays [a, r, r, a, y, s] bugfix This change fixes an existing bug labels Apr 26, 2024
Copy link
Copy Markdown
Member

@nhz2 nhz2 left a comment

Choose a reason for hiding this comment

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

LGTM

nsajko added 2 commits April 29, 2024 17:58
In several different situations `reshape` had thrown `DivideError`
instead of returning a correct result or throwing.

Fixes JuliaLang#54245
@nsajko nsajko force-pushed the zerolength_colon_reshape branch from 8a9ebaf to 30823c9 Compare April 29, 2024 16:33
@nsajko nsajko force-pushed the zerolength_colon_reshape branch from 30823c9 to f1fc62f Compare April 29, 2024 16:57
@nsajko
Copy link
Copy Markdown
Member Author

nsajko commented May 6, 2024

ping

1 similar comment
@nsajko
Copy link
Copy Markdown
Member Author

nsajko commented May 13, 2024

ping

@oscardssmith oscardssmith merged commit b9f68ac into JuliaLang:master May 13, 2024
@nsajko nsajko deleted the zerolength_colon_reshape branch May 13, 2024 14:34
@jishnub
Copy link
Copy Markdown
Member

jishnub commented Jun 17, 2024

The type assertions added here appear to have broken reshape using BigInts. Are these essential for type-inference?
E.g. the following works on Julia v1.10, but is broken on nightly:

julia> using FillArrays

julia> reshape(Fill(1, 2), big(2), :)
2×1 Fill{Int64}, with entries equal to 1

Edit: FillArrays appears to directly call this internal function, which it probably shouldn't. However, is reshape necessarily expected to convert indices to Int?

@nsajko
Copy link
Copy Markdown
Member Author

nsajko commented Jun 17, 2024

_reshape_uncolon is only ever (legally) called from that one reshape method, which itself only accepts Union{Int,Colon} dims.

julia/base/reshapedarray.jl

Lines 129 to 130 in b0b7a85

reshape(parent::AbstractArray, dims::Tuple{Vararg{Union{Int,Colon}}}) = reshape(parent, _reshape_uncolon(parent, dims))
@inline function _reshape_uncolon(A, dims)

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] bugfix This change fixes an existing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

colonful reshape may spuriously throw DivideError

5 participants