Sparse matrices should be range-checked

[vkostyukov]

Marko Batic found this issue. La4j can't rely on IndexOutOfBoundException anymore.

[vkostyukov]

We have bunch of methods in the vector interface like:

interface Vector {
  double get(int i, int j);
  double set(int i, int j, double value);
  ...
}

Simply speaking: all the methods that takes indices should perform an explicit range check before doing a job. Something like this:

  double get(int i, int j) {\
    makeSureIndicesAreGood(i, j); // I believe we already have similar method
    ... // do the reset of the job
  }

[DWiechert]

I assume you would like these methods to throw some sort of exception from them. Is the standard to make a custom library exception or would a general IllegalArgumentException be sufficient?

[vkostyukov]

Right. We have to keep the behavior unchanged from a dense version that throws IndexOutOfBoundsException. Could also please add unit-tests to make sure that all the vectors and matrices (no matter whether it's dense or sparse implementations) throws the same exception IndexOutOfBounds..?

[DWiechert]

Sure, I can start working on this. And to clarify, this is for both the Vector and Matrix interfaces right?

[vkostyukov]

Right. Sparse matrices as well as sparse vectors have the same issue.

[vkostyukov]

BTW, we probably have the same issue with Basic1DMatrix(). Please check it.

[DWiechert]

Compiling a list of the Vector and Matrix implementations that need index range checks:

• Vector
  • AbstractVector
  • SparseVector
  • CompressedVector
  • BasicVector
• Matrix
  • AbstractMatrix
  • AbstractCompressedMatrix
  • Basic1DMatrix
  • Basic2DMatrix
  • CCSMatrix
  • CRSMatrix

Does this list look about right to you? Also, I'm not sure if you care to implement range checks on a test only classes:

• MockVector
• MockMatrix

[vkostyukov]

I'm pretty sure there is nothing to do with BasicVector and BasicMatrix2D since they are just wrappers around double arrays, which are range-checked by default. We only have to implement it for:

• SparseVector
• Basic1DMatrix
• CCSMatrix
• CRSMatrix

[DWiechert]

Ok, I see what you mean about those 2 that just wrap the array and getting the check automatically. And I found how the abstract(s) "loop back" to the interface which then go to the specific implementations. That was my confusion.

However, it still seems CompressedVector needs the range check, unless I'm mis-understanding that comment.

[vkostyukov]

Right. I misstyped it: SparseVector is just an interface, a concrete implementation is CompressedVector, which should be range-checked.

[DWiechert]

Ok, I'll work on adding those range checks to the following then:

• CompressedVector
• Basic1DMatrix
• CCSMatrix
• CRSMatrix

[vkostyukov]

Awesome! Thanks!

[sylvia43]

I'd like to help out on this project? Do you have any suggestions where I should start? It seems like this task is in progress.

[vkostyukov]

Hi @anubiann00b!

That is great! You can start with this low landing fruit: #182. Feel free to ask any questions in that issue.

[SamoylovMD]

I believe we could avoid big changes if we just add throwing IndexOutOfBoundsException to AbstractCompressedMatrix.get().

[DWiechert]

@vkostyukov based on the conversation above, why do we need range checks on Basic1DMatrix? Isn't the range check provided for us because the data is just stored in an array?

[SamoylovMD]

@DWiechert I believe so. Array implementation gives us range check out of the box, so we really need a range check only in CCS and CRS.

[DWiechert]

@vkostyukov, I'd be happy to work on another issue for this project if you wouldn't mind pointing out a few with higher priority.

[vkostyukov]

@DWiechert, please take a look at this ticket: #172. Feel free to start the discussion there. And thanks for the help!