bramtayl / Unzip.jl

MIT License
8 stars 1 forks source link

Can't unzip BlockVector #6

Open yha opened 3 years ago

yha commented 3 years ago

Not sure if this is a bug in Unzip or BlockArrays

julia> using Unzip, BlockArrays

julia> m = mortar([Matrix(rand(i,j)) for i=1:2, j=1:3]);

julia> cblocks = [(rand(),rand()) for c in eachcol(m)];

julia> cblocks isa AbstractVector{<:Tuple}
true

julia> unzip(cblocks)
ERROR: type BlockArray has no field columns
Stacktrace:
 [1] getproperty(x::BlockVector{Tuple{Float64, Float64}, Vector{Vector{Tuple{Float64, Float64}}}, Tuple{BlockedUnitRange{Vector{Int64}}}}, f::Symbol)
   @ Base .\Base.jl:33
 [2] unzip(rows::PseudoBlockVector{Tuple{Float64, Float64}, Vector{Tuple{Float64, Float64}}, Tuple{BlockedUnitRange{Vector{Int64}}}})
   @ Unzip C:\Users\sternlab\.julia\packages\Unzip\x2oA5\src\Unzip.jl:249
 [3] top-level scope
   @ REPL[60]:1
bramtayl commented 3 years ago

Hmm, not 100% sure what's going on there. The method it's pointing to is

function unzip(rows)
    # stuff
    _collect(
        (@inbounds Rows(())),
         # stuff
    ).columns
end

And my understanding is that the Base collect method should return something of a type matching the first argument, so it's puzzling that it's returning a BlockVector

yha commented 2 years ago

Same thing happens with OffsetArrays:

julia> using OffsetArrays, Unzip

julia> unzip(OffsetVector([(i,i+1) for i=1:5], -1))  # should output two offset vectors
ERROR: type OffsetArray has no field columns

I think the issue is that Base._collect uses similar to construct the destination array. similar makes no guarantee to preserve the type, and in some cases it has to return a differently-typed wrapper to conform to the requires axes (such as offset or block axes). In this example you do want to call similar to construct the output vectors (each element of columns) as offset vectors, but not to construct the Rows object. Maybe Rows should be constructed explicitly and then filled with copyto! or similar?

bramtayl commented 2 years ago

Thanks! Want to make a PR?

bramtayl commented 2 years ago

Or even become a maintainer?

yha commented 2 years ago

I'm not sure I really understand how this package works, but I'll try to take a look

bramtayl commented 2 years ago

I looked into this. I had to add some more similar methods for different kinds of axes. As of #10, the OffsetArrays example works. The BlockArrays example doesn't work, because we need an extra disambiguation method for the similar method in OffsetArrays and the similar method here. I'm not sure of the best way to solve that.