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

Mixed Type promotion in a Vector regression on nightly #44821

Closed
evetion opened this issue Apr 1, 2022 · 6 comments · Fixed by #47893
Closed

Mixed Type promotion in a Vector regression on nightly #44821

evetion opened this issue Apr 1, 2022 · 6 comments · Fixed by #47893
Labels
invalid Indicates that an issue or pull request is no longer relevant

Comments

@evetion
Copy link
Member

evetion commented Apr 1, 2022

On all recent nightlies ArchGDAL (see yeesian/ArchGDAL.jl#285) started failing.

On Julia 1.8 and below the following [a::ArchGDAL.RasterBand{UInt8}, b::ArchGDAL.IRasterBand{UInt8}] will promote to a Vector{ArchGDAL.AbstractRasterBand{UInt8}} (note the I, the types differ).

The type hierarchy is (I)RasterBand <: ArchGDAL.AbstractRasterBand <: AbstractDiskArray{T,2} <: AbstractArray{T,N}.

On Julia 1.9 however the same is promoted all the way up the hierarchy to a
Vector{Matrix{UInt8}}.

@evetion evetion changed the title Dispatch regression on nightly Mixed Type promotion in a Vector regression on nightly Apr 1, 2022
@oscardssmith oscardssmith added bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch labels Apr 1, 2022
@thchr
Copy link
Contributor

thchr commented Apr 1, 2022

This is a variant of #44586. The fix is to use a tuple instead of a vector.

@evetion
Copy link
Member Author

evetion commented Apr 1, 2022

Thanks for the reference! But the fix is only a workaround, right?

In this case, by converting to a Matrix, the in between disk array, normally only lazy loaded, is unintentionally materialized. There are probably other packages with useful in between types that will be impacted by this breaking change.

@thchr
Copy link
Contributor

thchr commented Apr 1, 2022

I'm not sure I understand; my suggestion was just to change [a, b] to (a, b) which will give you a heterogeneous tuple with type Tuple{ArchGDAL.RasterBand{UInt8}, ArchGDAL.IRasterBand{UInt8}}. I can't see how this could affect materialization/laziness?

@evetion
Copy link
Member Author

evetion commented Apr 1, 2022

Sorry for the confusion, the tuple does indeed work and won't affect laziness.

My point was about the apparent design decision about this promotion of types, the one that led you to close your referenced issue, that seems like a unintentionally breaking change.

@vtjnash vtjnash added invalid Indicates that an issue or pull request is no longer relevant and removed bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch labels Apr 1, 2022
@vtjnash
Copy link
Member

vtjnash commented Apr 1, 2022

The [] syntax means vcat promotion. Always has. Seen the linked issues above for other syntax which mean something else (such as T[]).

@evetion
Copy link
Member Author

evetion commented Apr 1, 2022

Quoting @vtjnash here from #44096 for visbility

[a, b] is a general pattern for changing a and b to a common type. If you do not want that, you must specify the type of the array you want (usually Any[...])

(emphasis mine to indicate the common type is not stable)

So we either need to explicitly define the type we want in using [] or define promotion_rules (see docs) for our custom types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Indicates that an issue or pull request is no longer relevant
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants