Bug fix/csr matrix set value binary search bug #1190
Conversation
|
Thanks for the fix. In addition to the changes to the CSR matrix, could you also create a new test that fails with the previous implementation? |
Two test cases for setValue have been added in |
|
|
||
| ASSERT_FALSE(A.sorted()); | ||
|
|
||
| ASSERT_EQ(A(0, 0), 1); |
There was a problem hiding this comment.
Same here, please use EXPECT_EQ.
| */ | ||
| CSRGeneralMatrix(count dimension, ValueType zero = 0) | ||
| CSRGeneralMatrix(count dimension, ValueType zero = 0, bool isSorted = true) |
There was a problem hiding this comment.
Would setting the sorted value not require unnecessary effort, if it is set from false to true? Or should the matrix be sorted by default and the setter only allows to create an unsorted version. In this case I would suggest a makeUnsorted() function or similar, instead of a setter-function.
There was a problem hiding this comment.
We could have the matrix sorted by default. If some user does not require that functionality, we could allow them to turn it off with some makeUnosrted() function as you suggest. What we should avoid is polluting the constructors with variables that control an internal behavior of the data structure.
There was a problem hiding this comment.
Latest commit includes the makeUnsorted function and the old constructors
| CSRGeneralMatrix(count nRows, count nCols, ValueType zero = 0) | ||
| : rowIdx(nRows + 1), columnIdx(0), nonZeros(0), nRows(nRows), nCols(nCols), isSorted(true), | ||
| zero(zero) {} | ||
| CSRGeneralMatrix(count nRows, count nCols, ValueType zero = 0, bool isSorted = true) |
| if (value == getZero()) { // remove the nonZero value | ||
| columnIdx.erase(columnIdx.begin() + colIdx); | ||
| nonZeros.erase(nonZeros.begin() + colIdx); | ||
| if (!sorted()) { |
There was a problem hiding this comment.
| if (!sorted()) { | |
| if (isSorted) { |
| colIdx = none; | ||
| } else if (!sorted()) { | ||
|
|
||
| if (!sorted()) { |
There was a problem hiding this comment.
| if (!sorted()) { | |
| if (isSorted) { |
|
|
||
| // update rowIdx | ||
| for (index k = i + 1; k < rowIdx.size(); ++k) { | ||
| --rowIdx[k]; | ||
| rowIdx[k]++; |
There was a problem hiding this comment.
| rowIdx[k]++; | |
| ++rowIdx[k]; |
| rowIdx[k]++; | ||
| // update rowIdx | ||
| for (index k = i + 1; k < rowIdx.size(); ++k) { | ||
| rowIdx[k]++; |
There was a problem hiding this comment.
| rowIdx[k]++; | |
| ++rowIdx[k]; |
…x-set-value-binary-search-bug
| @@ -69,6 +50,7 @@ class CSRGeneralMatrix { | |||
| * Constructs the CSRGeneralMatrix with size @a dimension x @a dimension. | |||
| * @param dimension Defines how many rows and columns this matrix has. | |||
| * @param zero The zero element (default = 0). | |||
| * @param isSorted If the matrix representation should uses sorted vectors. | |||
| @@ -79,6 +61,7 @@ class CSRGeneralMatrix { | |||
| * @param nRows Number of rows. | |||
| * @param nCols Number of columns. | |||
| * @param zero The zero element (default = 0). | |||
| * @param isSorted If the matrix representation should uses sorted vectors. | |||
| @@ -79,6 +61,7 @@ class CSRGeneralMatrix { | |||
| * @param nRows Number of rows. | |||
| * @param nCols Number of columns. | |||
| * @param zero The zero element (default = 0). | |||
| * @param isSorted If the matrix representation should uses sorted vectors. | |||
| */ | |||
| CSRGeneralMatrix(count nRows, count nCols, ValueType zero = 0) | |||
| : rowIdx(nRows + 1), columnIdx(0), nonZeros(0), nRows(nRows), nCols(nCols), isSorted(true), | |||
There was a problem hiding this comment.
| : rowIdx(nRows + 1), columnIdx(0), nonZeros(0), nRows(nRows), nCols(nCols), isSorted(true), | |
| : rowIdx(nRows + 1), columnIdx(0), nonZeros(0), nRows(nRows), nCols(nCols), |
A bug in the binary search of GeneralCSRMatrix caused the collumn index array to be not sorted, if setValue operations are made in the wrong order. An example can be seen in #1104. The binary search function has been replaced at the appropriate places, now allowing setValue operations in any order, while keeping the values sorted. A test has been added to check this behavior for the example from #1104 and two other matrices.