Input checking in af::sparse

[rstub]

This report is motivated by https://stackoverflow.com/questions/49867065/arrayfire-sparse-matrix-issues. In that SO post the OP used input in effectively COO format to create a sparse matrix in CSR format:

int row[] = {0,0,0,1,1,2,2};
int col[] = {0,1,2,0,1,0,2};
double values[] = { 3,3, 4,3,10,4,3};
array rr = sparse(3,3,array(7,values),array(7,row),array(7,col));
af_print(rr);
af_print(dense(rr));

One can fix this by either using the correct row indices:

int row[] = {0,3,5,7};
int col[] = {0,1,2,0,1,0,2};
float values[] = {3,3,4,3,10,4,3};
array rr = sparse(3,3,array(7,values),array(4,row),array(7,col));
af_print(rr);
af_print(dense(rr)); 

or the correct format:

int row[] = {0,0,0,1,1,2,2};
int col[] = {0,1,2,0,1,0,2};
float values[] = { 3,3, 4,3,10,4,3};
array rr = sparse(3,3,array(7,values),array(7,row),array(7,col), AF_STORAGE_COO);
af_print(rr);
af_print(dense(rr));

But why did the OP not get an error when trying to create a CSR matrix with incorrect input?

[pavanky]

@rstub short of going through the entire row and checking if the elements are sorted, there is no easy way to verify COO vs CSR.

For example the following is both valid COO and CSR:

int row[] = {0,1,2};
int col[] = {0,1,2,};
double values[] = {1,1,1};

That said, forcing the user to choose the format may be the better solution instead of arrayfire making assumptions.

@umar456 This change does not break ABI but breaks C++ API. I think that should be OK ?

[rstub]

While there are cases that are valid for different sparse matrix representations, there are some necessary conditions for the different formats:

• COO: row, col and values must have equal length
• CSR: col and values must have equal length and row must have length numberOfRows +1
• CSC: row and values must have equal length and col must have length numberOfCols + 1

These necessary (but not sufficient) conditions should be checked within af::sparse independent of there being a default format (currently CSR) or not.

[pavanky]

@rstub Ok I did not realize this was broken. Do you want to send in a patch to fix this ?

[rstub]

@pavanky I have created test cases and parameter validation here: https://github.com/RInstitute/arrayfire/tree/sparse
The new tests fail with the current version but succeed with my changes. However, now some other tests are failing, e.g.:

[ RUN      ] Sparse/0.DeepCopy
/home/ralf/devel/arrayfire/test/testHelpers.hpp:455: Failure
      Expected: 0u
      Which is: 0
To be equal to: alloc_buffers
      Which is: 9
/home/ralf/devel/arrayfire/test/sparse.cpp:205: Failure
      Expected: size_of_alloc * 2
      Which is: 24576
To be equal to: lock_bytes
      Which is: 15360
The number of bytes allocated by the deep copy do not match the original array
/home/ralf/devel/arrayfire/test/sparse.cpp:209: Failure
      Expected: buffers_per_sparse * 2
      Which is: 24
To be equal to: lock_buffers
      Which is: 15
The number of buffers allocated by the deep copy do not match the original array
[  FAILED  ] Sparse/0.DeepCopy, where TypeParam = float (6 ms)

Do you know what's going on here? Or should I open a PR anyway to discuss this?

[pavanky]

@rstub you will need to destroy the af_array you created in the tests.

Also why not use the C++ API for the tests instead?

[pavanky]

@rstub If you want to test for the failure cases, use EXPECT_THROW which should work with C++ API.

[9prady9]

@pavanky For tests, we are using C-API alone. I don't remember the exact reason behind it but @umar456 can elaborate on that.

[rstub]

@pavanky Thanks, using af_release_array() helped. BTW, I used the C API because I used the test ISSUE_1745 as starting point ...