Fix #1401: bug in map and forEach of SparseMatrix not handling unorde…

…red indexes correctly

HISTORY.md

@@ -2,6 +2,8 @@
 
 # not yet published, version 5.5.0
 
+- Fix #1401: methods `map` and `forEach` of `SparseMatrix` not working
+  correctly when indexes are unordered.
 - Upgrade tiny-emitter to v2.1.0 (#1397).
 
 

src/type/matrix/SparseMatrix.js

@@ -876,29 +876,35 @@ function factory (type, config, load, typed) {
       // k0 <= k < k1 where k0 = _ptr[j] && k1 = _ptr[j+1]
       const k0 = matrix._ptr[j]
       const k1 = matrix._ptr[j + 1]
-      // row pointer
-      let p = minRow
-      // loop k within [k0, k1[
-      for (let k = k0; k < k1; k++) {
-        // row index
-        const i = matrix._index[k]
-        // check i is in range
-        if (i >= minRow && i <= maxRow) {
-          // zero values
-          if (!skipZeros) {
-            for (let x = p; x < i; x++) { invoke(0, x - minRow, j - minColumn) }
+
+      if (skipZeros) {
+        // loop k within [k0, k1[
+        for (let k = k0; k < k1; k++) {
+          // row index
+          const i = matrix._index[k]
+          // check i is in range
+          if (i >= minRow && i <= maxRow) {
+            // value @ k
+            invoke(matrix._values[k], i - minRow, j - minColumn)
           }
-          // value @ k
-          invoke(matrix._values[k], i - minRow, j - minColumn)
         }
-        // update pointer
-        p = i + 1
-      }
-      // zero values
-      if (!skipZeros) {
-        for (let y = p; y <= maxRow; y++) { invoke(0, y - minRow, j - minColumn) }
+      } else {
+        // create a cache holding all defined values
+        const values = {}
+        for (let k = k0; k < k1; k++) {
+          const i = matrix._index[k]
+          values[i] = matrix._values[k]
+        }
+
+        // loop over all rows (indexes can be unordered so we can't use that),
+        // and either read the value or zero
+        for (let i = minRow; i <= maxRow; i++) {
+          const value = (i in values) ? values[i] : 0
+          invoke(value, i - minRow, j - minColumn)
+        }
       }
     }
+
     // store number of values in ptr
     ptr.push(values.length)
     // return sparse matrix
@@ -931,26 +937,30 @@ function factory (type, config, load, typed) {
       // k0 <= k < k1 where k0 = _ptr[j] && k1 = _ptr[j+1]
       const k0 = this._ptr[j]
       const k1 = this._ptr[j + 1]
-      // column pointer
-      let p = 0
-      // loop k within [k0, k1[
-      for (let k = k0; k < k1; k++) {
-        // row index
-        const i = this._index[k]
-        // check we need to process zeros
-        if (!skipZeros) {
-          // zero values
-          for (let x = p; x < i; x++) { callback(0, [x, j], me) } // eslint-disable-line standard/no-callback-literal
+
+      if (skipZeros) {
+        // loop k within [k0, k1[
+        for (let k = k0; k < k1; k++) {
+          // row index
+          const i = this._index[k]
+
+          // value @ k
+          callback(this._values[k], [i, j], me)
+        }
+      } else {
+        // create a cache holding all defined values
+        const values = {}
+        for (let k = k0; k < k1; k++) {
+          const i = this._index[k]
+          values[i] = this._values[k]
+        }
+
+        // loop over all rows (indexes can be unordered so we can't use that),
+        // and either read the value or zero
+        for (let i = 0; i < rows; i++) {
+          const value = (i in values) ? values[i] : 0
+          callback(value, [i, j], me)
         }
-        // value @ k
-        callback(this._values[k], [i, j], me)
-        // update pointer
-        p = i + 1
-      }
-      // check we need to process zeros
-      if (!skipZeros) {
-        // zero values
-        for (let y = p; y < rows; y++) { callback(0, [y, j], me) } // eslint-disable-line standard/no-callback-literal
       }
     }
   }

test/type/matrix/SparseMatrix.test.js

@@ -1309,7 +1309,7 @@ describe('SparseMatrix', function () {
         ])
     })
 
-    it('should process non-zero values', function () {
+    it('should process non-zero values only', function () {
       const m = new SparseMatrix(
         [
           [1, 0],
@@ -1395,7 +1395,7 @@ describe('SparseMatrix', function () {
       assert.deepStrictEqual(output, [])
     })
 
-    it('should process non-zero values', function () {
+    it('should process non-zero values only', function () {
       const m = new SparseMatrix(
         [
           [1, 0],
@@ -1430,6 +1430,68 @@ describe('SparseMatrix', function () {
     })
   })
 
+  describe('index ordering', () => {
+    const orderedSparseMatrix = new SparseMatrix({
+      values: [1, 3, 2, 4],
+      index: [0, 1, 0, 1],
+      ptr: [0, 2, 4],
+      size: [2, 2]
+    })
+
+    const unorderedSparseMatrix = new SparseMatrix({
+      values: [3, 1, 4, 2],
+      index: [1, 0, 1, 0],
+      ptr: [0, 2, 4],
+      size: [2, 2]
+    })
+
+    const expectedLogs = [
+      { value: 1, index: [ 0, 0 ] },
+      { value: 3, index: [ 1, 0 ] },
+      { value: 2, index: [ 0, 1 ] },
+      { value: 4, index: [ 1, 1 ] }
+    ]
+
+    it('should have parsed the two test matrices correctly', () => {
+      assert.deepStrictEqual(orderedSparseMatrix.toArray(), [[1, 2], [3, 4]])
+      assert.deepStrictEqual(unorderedSparseMatrix.toArray(), [[1, 2], [3, 4]])
+    })
+
+    it('should run forEach on a sparse matrix with ordered indexes', function () {
+      const logs = []
+      orderedSparseMatrix.forEach((value, index) => logs.push({ value, index }))
+
+      assert.deepStrictEqual(logs, expectedLogs)
+    })
+
+    it('should run map on a sparse matrix with ordered indexes', function () {
+      const logs = []
+      orderedSparseMatrix.map((value, index) => {
+        logs.push({ value, index })
+        return value
+      })
+
+      assert.deepStrictEqual(logs, expectedLogs)
+    })
+
+    it('should run forEach on a sparse matrix with unordered indexes', function () {
+      const logs = []
+      unorderedSparseMatrix.forEach((value, index) => logs.push({ value, index }))
+
+      assert.deepStrictEqual(logs, expectedLogs)
+    })
+
+    it('should run map on a sparse matrix with unordered indexes', function () {
+      const logs = []
+      unorderedSparseMatrix.map((value, index) => {
+        logs.push({ value, index })
+        return value
+      })
+
+      assert.deepStrictEqual(logs, expectedLogs)
+    })
+  })
+
   describe('clone', function () {
     it('should clone the matrix properly', function () {
       const m1 = new SparseMatrix(