willow-ahrens / Finch.jl

Sparse tensors in Julia and more! Datastructure-driven array programing language.
http://willowahrens.io/Finch.jl/
MIT License
158 stars 15 forks source link

`MethodError` in 1-D Tensor indexing #427

Closed mtsokol closed 6 months ago

mtsokol commented 6 months ago

Hi @willow-ahrens,

I noticed that 1-D Tensor indexing fails with MethodError, here's a code to reproduce:

using Finch

a = range(1,100,step=1) |> collect
b = Finch.Tensor(Finch.Dense(Finch.Element(0)), a)

a[30]
b[30]

a[[1,2,3]]
b[[1,2,3]] # fails with ERROR: MethodError: no method matching keys(::IndexLinear, ::Tensor{DenseLevel{Int64, ElementLevel{0, Int64, Int64, Vector{Int64}}}})

a[range(start=10, stop=20)]
b[range(start=10, stop=20)] # fails with ERROR: MethodError: no method matching keys(::IndexLinear, ::Tensor{DenseLevel{Int64, ElementLevel{0, Int64, Int64, Vector{Int64}}}})

Is it something easy to fix?


I also noticed that for n-dimensional tensors all dimensions must be provided when indexing (for example, t1 which is 2-D must be indexed with t1[i1, i2], where for t1[i1] it fails with AssertionError: ndims(arr) == length(inds)). I don't mind keeping it that way as I can infer : slices for the implicit dimensions on the Python side.

willow-ahrens commented 6 months ago

# fails with ERROR: MethodError: no method matching keys(::IndexLinear, ::Tensor{DenseLevel{Int64, ElementLevel{0, Int64, Int64, Vector{Int64}}}})

^ I think this means we're missing a fallback for indexing with vectors.

We want for getindex(x, i::Vector) to hit the same fallback as getindex(A, i::Vector, j::Int), but instead it's hitting the same fallback that we would hit with getindex(A, i::Vector{Int}) or getindex(A, i::Vector{CartesianIndex}), or getindex(A, i::Array{CartesianIndex}). All of those methods attempt to index a multidimensional array with the linear index, so we could conceivably do something like:

julia> A = [11 12 13; 14 15 16; 17 18 19]
3×3 Matrix{Int64}:
 11  12  13
 14  15  16
 17  18  19

julia> A[[1, 2, 3, 4]]
4-element Vector{Int64}:
 11
 14
 17
 12

I think that we should write our own specialization of getindex for the case where the index is a single vector, to check whether the array is 1D or not. if it's 1D, we can call getindex_helper or whatever we usually do. If it's not 1D, we should then error.

We should create an issue for supporting the more complicated form of getindex(arr::Tensor, idx::Array) where idx might be multidimensional. I think a good starting point could be handling this by casting arr to an AsArray, though we could also write a dedicated finch fallback for this at some point.

willow-ahrens commented 6 months ago

closed in 429