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

zero(u::StructArray) broken in Julia v1.8.0-rc1 #230

Closed
ranocha opened this issue Jun 10, 2022 · 7 comments
Closed

zero(u::StructArray) broken in Julia v1.8.0-rc1 #230

ranocha opened this issue Jun 10, 2022 · 7 comments

Comments

@ranocha
Copy link
Contributor

ranocha commented Jun 10, 2022

The generic fallback changed from

zero(x::AbstractArray{T}) where {T} = fill!(similar(x), zero(T))

in Julia v1.7.3 to

zero(x::AbstractArray{T}) where {T} = fill!(similar(x, typeof(zero(T))), zero(T))

in Julia v1.8.0-rc1. This means that we had

julia> using Pkg; Pkg.status(["StructArrays", "StaticArrays"])
      Status `~/.julia/environments/v1.7/Project.toml`
  [90137ffa] StaticArrays v1.4.4
  [09ab397b] StructArrays v0.6.8

julia> using StructArrays, StaticArrays

julia> u = StructArray([SVector(1.0)])
1-element StructArray(::Vector{Float64}) with eltype SVector{1, Float64}:
 [1.0]

julia> zero(u)
1-element StructArray(::Vector{Float64}) with eltype SVector{1, Float64}:
 [0.0]

in Julia v1.7 but get

julia> using Pkg; Pkg.status(["StructArrays", "StaticArrays"])
Status `~/.julia/environments/v1.8/Project.toml`
  [90137ffa] StaticArrays v1.4.6
  [09ab397b] StructArrays v0.6.8

julia> using StructArrays, StaticArrays

julia> u = StructArray([SVector(1.0)])
1-element StructArray(::Vector{Float64}) with eltype SVector{1, Float64}:
 [1.0]

julia> zero(u)
1-element Vector{SVector{1, Float64}}:
 [0.0]

in Julia v1.8.0-rc1. This breaks a lot of code, for example in Trixi.jl relying on OrdinaryDiffEq.jl, see https://github.com/trixi-framework/Trixi.jl/runs/6830499545?check_suite_focus=true#step:6:244.

To fix this, it would be great if #218 or #94 were finished. Is there anything I can do to help you with that?

@piever
Copy link
Collaborator

piever commented Jun 12, 2022

To be honest, #218 can probably be merged already. The minor issue that's holding me back is that IMO ideally reshape and similar with "offset indices" (i.e. AbstractUnitRange) applied to a StructArray should yield a StructArray of OffsetArrays, and not vice versa.

On the other hand, getting that right seems extremely brittle, as StructArrays should mimic the exact dispatches that OffsetArrays is using. But maybe it's not so bad.

D you know if one can rely on the exact dispatch chosen by OffsetArrays (ie Tuple{OffsetAxisKnownLength,Vararg{OffsetAxisKnownLength}} for similar and Tuple{OffsetAxis,Vararg{OffsetAxis}} for reshape)? In that case, I can probably just add the correct signatures. Otherwise it may be best to just merge the PR as is.

cc: @timholy (esp. on how to intercept the OffsetArrays fallback for similar with AbstractUnitRange)

@ranocha
Copy link
Contributor Author

ranocha commented Jun 12, 2022

Thanks a lot! Sorry, I do not know a lot about the internals of OffsetArrays.jl.

@ranocha
Copy link
Contributor Author

ranocha commented Jun 15, 2022

The minor issue that's holding me back is that IMO ideally reshape and similar with "offset indices" (i.e. AbstractUnitRange) applied to a StructArray should yield a StructArray of OffsetArrays, and not vice versa.

Is this a major blocking issue, @piever, or just something that would be nice to have and could also be changed/implemented later? If the latter is the case, I would be very happy to see #218 merged soon to let use test Julia v1.8.0-rc1 with Trixi.jl.

@piever
Copy link
Collaborator

piever commented Jun 15, 2022

Is this a major blocking issue, @piever, or just something that would be nice to have and could also be changed/implemented later?

Changing it is (very slightly) breaking, so I would like to avoid tweaking this too many times. That being said, #218 now implements it correctly, so if there is no further feedback in the next couple of days I'll go ahead and merge and tag a release.

@ranocha
Copy link
Contributor Author

ranocha commented Jun 15, 2022

Thanks a lot, @piever!

@piever
Copy link
Collaborator

piever commented Jun 17, 2022

Fixed by #218

@piever piever closed this as completed Jun 17, 2022
@ranocha
Copy link
Contributor Author

ranocha commented Jun 19, 2022

Thanks a lot!

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

2 participants