ArrayInterface.jl icon indicating copy to clipboard operation
ArrayInterface.jl copied to clipboard

Bug in `dense_dims` for reshaped views

Open ranocha opened this issue 3 years ago • 4 comments

Reported in https://github.com/JuliaArrays/ArrayInterface.jl/pull/156#issuecomment-851002133:

julia> using ArrayInterface

julia> u_base = randn(10, 10); u_view = view(u_base, 3, :); u_reshaped_view = reshape(u_view, 1, size(u_base, 2));

julia> ArrayInterface.dense_dims(u_reshaped_view)

As confirmed by @chriselrod in https://github.com/JuliaArrays/ArrayInterface.jl/pull/156#issuecomment-851293134, this is a bug:

The correct answer for u_reshaped_view would be (False(),False()).

ranocha avatar May 31 '21 10:05 ranocha

resolved with #161

Tokazama avatar Jun 10 '22 10:06 Tokazama

I fixed it only for the case of reshaped versions of abstract vectors. It still fails for reshaped views of higher-dimensional arrays, e.g.,

(@v1.7) pkg> activate --temp
  Activating new project at `/tmp/jl_4vH1Gz`

(jl_4vH1Gz) pkg> add ArrayInterface
    Updating registry at `~/.julia/registries/General.toml`
   Resolving package versions...
    Updating `/tmp/jl_4vH1Gz/Project.toml`
  [4fba245c] + ArrayInterface v6.0.14

julia> using ArrayInterface

julia> u_base = randn(10, 10, 10); u_view = view(u_base, 3, :, :); u_reshaped_view = reshape(u_view, 1, size(u_base, 2) * size(u_base, 3));

julia> ArrayInterface.dense_dims(u_reshaped_view)

julia> ArrayInterface.dense_dims(u_reshaped_view) |> isnothing
true

Thus, this issue is not resolved in general.

ranocha avatar Jun 10 '22 11:06 ranocha

I guess in that example we could know that no dimensions are dense because u_view doesn't have any dense dimensions, but I'm not sure we can do any better at identifying instances where they are dense.

Tokazama avatar Jun 10 '22 11:06 Tokazama

#307 tests all the examples in this issue. I'm assuming there are still times when we don't detect dense dimensions just because we can't know in some situations without knowing the size.

Tokazama avatar Jun 11 '22 02:06 Tokazama