Issues at accessing Tensor elements and more
- Dominant language
- Scala
- Stars
- 1.5k
- Forks
- 315
- PR merge metrics
- No merged PRs in 30d
Description
I encountered some problems at accessing elements of a sparse tensor, through the function "get". Such function checks if the sought index exists in the list of non-zero elements of the tensor. If it is the case, it returns the element. If it doesn't find it, it returns an error (None). It should return zero though (the sparse array is "filled" with zeroes). An error should only be returned when we are querying for an element outside the bounds of the tensor, but inside the tensor any element not in the list is in fact a zero (or zero-equivalent depending on the type).
I will submit a PR solving this issue. I have added test cases for accessing elements in both dense and sparse arrays.
While having a look at the code, I've seen other concerns that I'll address in the PR:
- The dense tensor class allows to have -1 in its first dimension. I added test cases to show that this works well when accessing elements. The code breaks however when calling the "size" function (I've added the test cases too). This function is inherited from Tensor, which computes the size as "dimensions.product", hence returning a negative number if we have -1 in the first component. I have refactored the code to overwrite "size" for dense tensors.
- Some parts of the sparse tensor class and its corresponding test seem to contemplate the possibility of a -1 dimension too. This does not seem to be possible in sparse tensors: we cannot infer the missing dimension from the raw vector (since it only contains the non-zero elements of the tensor). This is observed in the "toArray" function, which creates an array of size "dimensions.product", which would break with a -1 and there doesn't seem to be a way around it. I have thus removed the "support" for -1 in the case of sparse tensors. In practice it just means that I slightly modified the comparison function and removed test cases for -1 dimensions. I don't think much changes, but maybe we shouldn't be encouraging to set dimensions to -1 in sparse tensors when they are not fully supported. But let's hear what you think about this!
- In the test cases for sparse tensors, the indices were not properly specified. The tests still passed because there are no consistency checks upon the creation of tensors. Since there weren't any test cases for accessing elements, this wasn't evident in the tests. In the current version it is as follows: Tensor.create(Array(22.3, 45.6, 99.3), Seq(5), Some(Seq(Seq(1,2,4)))). However, it should be Tensor.create(Array(22.3, 45.6, 99.3), Seq(5), Some(Seq(Seq(1), Seq(2), Seq(4)))). We are supposed to supply a list of coordinates of the non-zero elements in the tensor. Each of the lists is of size 1 if there's only one dimension, and the size of the outer list should match the amount of elements in the input array.
For the record...
It seems that in many instances of the mleap library, we "trust" the value contained in "dimensions", assuming it won't be -1. In VectorConverters.scala we see this repeatedly. The converters would pretty much all fail if we create a tensor with a -1 dimension. We may at some point reconsider this... We could disable the -1 in the code altogether, or maybe store the true dimensions in the variable, while having a -1 alternative just as syntactic sugar for the creation of the tensor. E.g., if we create a dense tensor with values 1,2,3 and dimensions (-1), then we'll compute the true dimension (3) and store that instead.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the dense and sparse tensor implementations, their get and size behavior, and the corresponding tensor tests. Check the existing VectorConverters.scala handling of dimensions, then verify that missing in-bounds sparse elements return zero, out-of-bounds access errors, and dense and sparse -1 dimensions follow the agreed behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala
- Domain
- machine-learning
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100