jonniedie / ComponentArrays.jl

Arrays with arbitrarily nested named components.
MIT License
286 stars 34 forks source link

Resolve ambiguities in https://github.com/jonniedie/ComponentArrays.jl/pull/231 #230

Closed vpuri3 closed 6 months ago

vpuri3 commented 8 months ago

To avoid conflict with

https://github.com/JuliaSparse/SparseArrays.jl/blob/main/src/sparsevector.jl#L1242-L1247

without this PR:

julia> vcat(ones(4), ComponentArray((;a = ones(2), b = ones(2))))
ERROR: MethodError: vcat(::Vector{Float64}, ::ComponentVector{Float64, Vector{Float64}, Tuple{Axis{(a = 1:2, b = 3:4)}}}) is ambiguous.

Candidates:
  vcat(x::AbstractVector, y::ComponentVector)
    @ ComponentArrays ~/.julia/packages/ComponentArrays/7wTG6/src/array_interface.jl:37
  vcat(X::Union{Number, AbstractVecOrMat{<:Number}}...)
    @ SparseArrays ~/.julia/juliaup/julia-1.10.0-beta3+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/SparseArrays/src/sparsevector.jl:1229

Possible fix, define
  vcat(::AbstractVector{<:Number}, ::ComponentVec

with this PR:

julia> vcat(ones(4), ComponentArray((;a = ones(2), b = ones(2))))
8-element Vector{Float64}:
 1.0
 1.0
 1.0
 1.0
 1.0
 1.0
 1.0
 1.0
codecov-commenter commented 8 months ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (931f411) 73.45% compared to head (921f859) 73.32%. Report is 1 commits behind head on main.

Files Patch % Lines
src/array_interface.jl 55.55% 4 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #230 +/- ## ========================================== - Coverage 73.45% 73.32% -0.14% ========================================== Files 23 23 Lines 746 746 ========================================== - Hits 548 547 -1 - Misses 198 199 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vpuri3 commented 8 months ago

ok tests are passing on < 1.10 now. Gonna test on 1.10 next

vpuri3 commented 8 months ago

The vcat ambiguity in 1.10 is pretty crazy:

https://github.com/jonniedie/ComponentArrays.jl/actions/runs/6707386215/job/18225805848?pr=230#step:6:353

MethodError: vcat(::ComponentVector{Float64, Vector{Float64}, Tuple{Axis{(a = 1, b = 2:3, c = ViewAxis(4:10, Axis(a = ViewAxis(1:3, Axis(a = 1, b = 2:3)), b = 4:7)))}}}, ::ComponentVector{Float64, Vector{Float64}, Tuple{Axis{(a = 1, b = 2:3, c = ViewAxis(4:10, Axis(a = ViewAxis(1:3, Axis(a = 1, b = 2:3)), b = 4:7)))}}}, ::ComponentVector{Float64, Vector{Float64}, Tuple{Axis{(a = 1, b = 2:3, c = ViewAxis(4:10, Axis(a = ViewAxis(1:3, Axis(a = 1, b = 2:3)), b = 4:7)))}}}) is ambiguous.

  Candidates:
    vcat(x::ComponentVector, args...)
      @ ComponentArrays ~/work/ComponentArrays.jl/ComponentArrays.jl/src/array_interface.jl:61
    vcat(x::ComponentVector, args::Union{Number, UniformScaling, AbstractVecOrMat}...)
      @ ComponentArrays ~/work/ComponentArrays.jl/ComponentArrays.jl/src/array_interface.jl:62
    vcat(x::ComponentVector, args::Vararg{AbstractVector{T}, N}) where {T, N}
      @ ComponentArrays ~/work/ComponentArrays.jl/ComponentArrays.jl/src/array_interface.jl:63
    vcat(X::Union{Number, AbstractVecOrMat{<:Number}}...)
      @ SparseArrays /opt/hostedtoolcache/julia/1.10.0-beta3/x64/share/julia/stdlib/v1.10/SparseArrays/src/sparsevector.jl:1229
    vcat(A::Union{Number, AbstractArray, UniformScaling}...)
      @ LinearAlgebra /opt/hostedtoolcache/julia/1.10.0-beta3/x64/share/julia/stdlib/v1.10/LinearAlgebra/src/uniformscaling.jl:428
    vcat(A::Union{AbstractArray, UniformScaling}...)
      @ LinearAlgebra /opt/hostedtoolcache/julia/1.10.0-beta3/x64/share/julia/stdlib/v1.10/LinearAlgebra/src/uniformscaling.jl:426
    vcat(A::Union{Number, AbstractArray}...)
      @ Base abstractarray.jl:1992
    vcat(A::AbstractArray...)
      @ Base abstractarray.jl:1991
    vcat(A::AbstractVecOrMat...)
      @ Base abstractarray.jl:1682
    vcat(V::AbstractVector...)
      @ Base abstractarray.jl:1612
    vcat(A::AbstractVecOrMat{T}...) where T
      @ Base abstractarray.jl:1683
    vcat(V::AbstractVector{T}...) where T
      @ Base abstractarray.jl:1613
  To resolve the ambiguity, try making one of the methods more specific, or adding a new method more specific than any of the existing applicable methods.
vpuri3 commented 8 months ago

@jonniedie can you help with ^?

jonniedie commented 8 months ago

Might not be able to get to this today. But how are other array packages handling it? Seems like everyone should be hitting the same problem.

vpuri3 commented 8 months ago

which packages are you talking about?

vpuri3 commented 8 months ago

I made the methods more specific, and tests are passing locally

vpuri3 commented 8 months ago

@jonniedie looks like this is good to go. Bumping version.

vpuri3 commented 8 months ago

all comments have been resolved

vpuri3 commented 8 months ago

@jonniedie can we merge?

vpuri3 commented 8 months ago

Sorry for the delay, was busy with school work. @jonniedie addressed your latest comment

vpuri3 commented 8 months ago

@jonniedie

vpuri3 commented 8 months ago

@jonniedie

vpuri3 commented 7 months ago

@jonniedie

vpuri3 commented 7 months ago

@jonniedie ping

jonniedie commented 7 months ago

Thanks for doing this. One last ask, though: would you mind making a test case of the example you gave above so this will have coverage?