From c8e4bbd5731fe05dde4711f833fecc6f995bdae4 Mon Sep 17 00:00:00 2001 From: David Contreras Date: Thu, 1 Aug 2024 07:54:20 -0600 Subject: [PATCH] fix: function `map` not always working with matrices (#3242) * Removed maxArgumentCount in favor of applyCallback * Making a pure _recurse function * Added cbrt tests, removed unnecesary changes in functions. * Fixed main bottleneck * Restored back function before unintended change * Fix format --------- Co-authored-by: Jos de Jong --- src/function/matrix/map.js | 4 +-- src/type/matrix/DenseMatrix.js | 11 ++---- src/type/matrix/SparseMatrix.js | 13 +++---- src/utils/function.js | 14 -------- test/unit-tests/function/matrix/map.test.js | 6 +++- test/unit-tests/utils/function.test.js | 39 +-------------------- 6 files changed, 15 insertions(+), 72 deletions(-) diff --git a/src/function/matrix/map.js b/src/function/matrix/map.js index 950a05b748..816d8ec53c 100644 --- a/src/function/matrix/map.js +++ b/src/function/matrix/map.js @@ -61,11 +61,11 @@ function _map (array, callback) { const recurse = function (value, index) { if (Array.isArray(value)) { return value.map(function (child, i) { - // we create a copy of the index array and append the new index value + // we create a copy of the index array and append the new index value return recurse(child, index.concat(i)) }) } else { - // invoke the callback function with the right number of arguments + // invoke the callback function with the right number of arguments return applyCallback(callback, value, index, array, 'map') } } diff --git a/src/type/matrix/DenseMatrix.js b/src/type/matrix/DenseMatrix.js index c27b59aab2..9bea6d6cf5 100644 --- a/src/type/matrix/DenseMatrix.js +++ b/src/type/matrix/DenseMatrix.js @@ -5,7 +5,7 @@ import { isInteger } from '../../utils/number.js' import { clone, deepStrictEqual } from '../../utils/object.js' import { DimensionError } from '../../error/DimensionError.js' import { factory } from '../../utils/factory.js' -import { maxArgumentCount } from '../../utils/function.js' +import { applyCallback } from '../../utils/applyCallback.js' const name = 'DenseMatrix' const dependencies = [ @@ -550,7 +550,6 @@ export const createDenseMatrixClass = /* #__PURE__ */ factory(name, dependencies DenseMatrix.prototype.map = function (callback) { // matrix instance const me = this - const args = maxArgumentCount(callback) const recurse = function (value, index) { if (isArray(value)) { return value.map(function (child, i) { @@ -558,13 +557,7 @@ export const createDenseMatrixClass = /* #__PURE__ */ factory(name, dependencies }) } else { // invoke the callback function with the right number of arguments - if (args === 1) { - return callback(value) - } else if (args === 2) { - return callback(value, index) - } else { // 3 or -1 - return callback(value, index, me) - } + return applyCallback(callback, value, index, me, 'map') } } diff --git a/src/type/matrix/SparseMatrix.js b/src/type/matrix/SparseMatrix.js index cc37179215..4f573827d2 100644 --- a/src/type/matrix/SparseMatrix.js +++ b/src/type/matrix/SparseMatrix.js @@ -5,7 +5,7 @@ import { clone, deepStrictEqual } from '../../utils/object.js' import { arraySize, getArrayDataType, processSizesWildcard, unsqueeze, validateIndex } from '../../utils/array.js' import { factory } from '../../utils/factory.js' import { DimensionError } from '../../error/DimensionError.js' -import { maxArgumentCount } from '../../utils/function.js' +import { applyCallback } from '../../utils/applyCallback.js' const name = 'SparseMatrix' const dependencies = [ @@ -854,12 +854,9 @@ export const createSparseMatrixClass = /* #__PURE__ */ factory(name, dependencie const rows = this._size[0] const columns = this._size[1] // invoke callback - const args = maxArgumentCount(callback) const invoke = function (v, i, j) { // invoke callback - if (args === 1) return callback(v) - if (args === 2) return callback(v, [i, j]) - return callback(v, [i, j], me) + return applyCallback(callback, v, [i, j], me, 'map') } // invoke _map return _map(this, 0, rows - 1, 0, columns - 1, invoke, skipZeros) @@ -890,11 +887,11 @@ export const createSparseMatrixClass = /* #__PURE__ */ factory(name, dependencie // invoke callback const invoke = function (v, x, y) { // invoke callback - v = callback(v, x, y) + const value = callback(v, x, y) // check value != 0 - if (!eq(v, zero)) { + if (!eq(value, zero)) { // store value - values.push(v) + values.push(value) // index index.push(x) } diff --git a/src/utils/function.js b/src/utils/function.js index be3b4cb64a..2287a1138d 100644 --- a/src/utils/function.js +++ b/src/utils/function.js @@ -87,17 +87,3 @@ export function memoizeCompare (fn, isEqual) { return memoize } - -/** - * Find the maximum number of arguments expected by a typed function. - * @param {function} fn A typed function - * @return {number} Returns the maximum number of expected arguments. - * Returns -1 when no signatures where found on the function. - */ -export function maxArgumentCount (fn) { - return Object.keys(fn.signatures || {}) - .reduce(function (args, signature) { - const count = (signature.match(/,/g) || []).length + 1 - return Math.max(args, count) - }, -1) -} diff --git a/test/unit-tests/function/matrix/map.test.js b/test/unit-tests/function/matrix/map.test.js index e3599a863c..899e8257b3 100644 --- a/test/unit-tests/function/matrix/map.test.js +++ b/test/unit-tests/function/matrix/map.test.js @@ -66,11 +66,15 @@ describe('map', function () { it('should invoke a typed function with correct number of arguments (4)', function () { // cbrt has a syntax cbrt(x, allRoots), but it should invoke cbrt(x) here assert.deepStrictEqual(math.map([1, 8, 27], math.cbrt), [1, 2, 3]) + assert.deepStrictEqual(math.map(math.matrix([1, 8, 27]), math.cbrt), math.matrix([1, 2, 3])) + assert.deepStrictEqual(math.map(math.matrix([1, 8, 27], 'sparse'), math.cbrt), math.matrix([1, 2, 3], 'sparse')) }) it('should invoke a typed function with correct number of arguments (5)', function () { - // cbrt has a syntax cbrt(x, allRoots), but it should invoke cbrt(x) here + // format has a syntax format(x, options), but it should invoke format(x) here assert.deepStrictEqual(math.map([1, 8, 27], math.format), ['1', '8', '27']) + assert.deepStrictEqual(math.map(math.matrix([1, 8, 27]), math.format), math.matrix(['1', '8', '27'])) + assert.deepStrictEqual(math.map(math.matrix([1, 8, 27], 'sparse'), math.format), math.matrix(['1', '8', '27'], 'sparse')) }) it('should throw an error if called with unsupported type', function () { diff --git a/test/unit-tests/utils/function.test.js b/test/unit-tests/utils/function.test.js index 967152e39b..33f9d63efe 100644 --- a/test/unit-tests/utils/function.test.js +++ b/test/unit-tests/utils/function.test.js @@ -1,5 +1,5 @@ import assert from 'assert' -import { maxArgumentCount, memoize, memoizeCompare } from '../../../src/utils/function.js' +import { memoize, memoizeCompare } from '../../../src/utils/function.js' import { deepStrictEqual } from '../../../src/utils/object.js' describe('util.function', function () { @@ -108,41 +108,4 @@ describe('util.function', function () { assert.strictEqual(execCount, 5) }) }) - - describe('maxArgumentCount', function () { - it('should calculate the max argument count of a typed function', function () { - const a = function () {} - a.signatures = { - 'number, number': function () {}, - number: function () {} - } - assert.strictEqual(maxArgumentCount(a), 2) - - const b = function () {} - b.signatures = { - number: function () {}, - 'number, number': function () {} - } - assert.strictEqual(maxArgumentCount(b), 2) - - const c = function () {} - c.signatures = { - number: function () {}, - BigNumber: function () {} - } - assert.strictEqual(maxArgumentCount(c), 1) - - const d = function () {} - d.signatures = { - 'number,number': function () {}, - number: function () {}, - 'number,any,number': function () {} - } - assert.strictEqual(maxArgumentCount(d), 3) - }) - - it('should return -1 for regular functions', function () { - assert.strictEqual(maxArgumentCount(function () {}), -1) - }) - }) })