Sparse tensor #3
Conversation
|
@karllessard I think I need you to approve running the workflows for this PR. |
|
I approved the CI run. GitHub changed actions so all first time contributions to a repo require approval so after this is integrated you'll be fine. |
|
@karllessard @Craigacp I am getting many JavaDoc errors on the old code. Did we want to fix them now? Also, I submitted a fix to one class in sparse with a JavaDoc error, but the same 1 workflow awaiting approval came back up. |
| * @param shape the shape of the dense array represented by this sparse array. | ||
| * @return the byte sparse array. | ||
| */ | ||
| public static ByteSparseNdArray sparseTensorOf( |
There was a problem hiding this comment.
In the Java NdArray space, we don't talk about "tensors" but simply about "arrays". So I would suggest to rename this method and the following by simply sparseOf
There was a problem hiding this comment.
Ok, renamed all to sparseOf
| * <pre>{@code | ||
| * FloatSparseNdArray st = new FloatSparseNdArray( | ||
| * StdArrays.of(new long[][] {{0, 0}, {1, 2}}, | ||
| * StdArrays.of(new float[] {1f, 2f}, |
There was a problem hiding this comment.
Whenever possible in your example, I would suggest to use NdArrays instead of passing by StdArrays, which is slower. Something like:
new FloatSparseNdArray(
StdArrays.of(new long[][] {{0, 0}, {1, 2}}), <----- missing closing parentheses by the way...
NdArrays.ofFloats(1f, 2f),
Shape.of(3, 4)
);
There was a problem hiding this comment.
Actually NdArrays.ofFloats(...) is wrong, it should be NdArrays.vectorOf(...), for the values. I have changed the values argument to NdArrays.vectorOf(...), and fixed closing parenthesis. Also, changed
setValues(StdArrays.ndCopyOf(valuesArray)) to setValues(NdArrays.vectorOf(valuesArray)) in the write(ByteDataBuffer) methods.
ndarray/src/main/java/org/tensorflow/ndarray/impl/sparse/AbstractSparseNdArray.java
Show resolved
Hide resolved
ndarray/src/main/java/org/tensorflow/ndarray/impl/sparse/AbstractSparseNdArray.java
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * Performs a binary search on the indices array to locate the the index of the specified |
There was a problem hiding this comment.
Ok found in 3 places and fixed.
| import java.nio.ReadOnlyBufferException; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| public class ByteSparseWindow extends SparseWindow<Byte, ByteNdArray> implements ByteNdArray { |
There was a problem hiding this comment.
Can we rename this package and its classes to slice instead? (i.e. org.tensorflow.ndarray.impl.sparse.slice.ByteSparseSlice`)
Because window is already being used by DataBufferWindow, which is a different concept and act differently (allowing to "slide" a view inside a buffer)
|
Amazing job @JimClarke5 ! Please see my comments, mostly minor stuff |
Change sparseTensorOf method names to sparseOf Add more javadoc descriptions od Indices and Values in SparseNdArray interface.
… zero, false, or null. added test cases
'org.tensorflow.ndarray.impl.sparse.SparseNdArray' and 'org.tensorflow.ndarray.impl.sparse.slice.ObjectSparseSlice'. Added test 'org.tensorflow.ndarray.impl.sparse.StringSparseNdArrayTest' to test Object sparse array using String. I had to modify slowEquals in 'AbstractNdArray' to handle null values in equals Also change javadoc from "nonzero" to "non-default" to be more accurate when zero is not the default.
| * }</pre> | ||
| */ | ||
| public class SparseNdArray<T, U extends NdArray<T>> extends AbstractSparseNdArray<T, U> | ||
| implements org.tensorflow.ndarray.SparseNdArray<T, U> { |
There was a problem hiding this comment.
The name conflict between the interface and the "object" implementation is a bit annoying but I'm ok keeping it like that for now and readjust later if needed, since that SparseNdArray is a "private" implementation.
karllessard
left a comment
There was a problem hiding this comment.
Thanks @JimClarke5 , you have done an amazing job!
Add Support for Sparse Arrays