From 1465ac7bbb90e8f685e5394110a49ade1d1bcebe Mon Sep 17 00:00:00 2001 From: Praise Nnamonu <110940850+praisennamonu1@users.noreply.github.com> Date: Wed, 20 Sep 2023 08:24:17 +0100 Subject: [PATCH] Fix round-off errors in `mod()` (#3011) * changes made to the following files: - mod.js - gcd.js * updated BigNumber implementation and added validating tests * added validating test cases * updated test cases * formatted code * Made updates according to requirement used mathjs floor in mod.js imported mod in gcd.js made mod work for negative divisors wrote and updated tests to validate new behavior * updated mod in arithmetic.js * added tests for modNumber function --------- Co-authored-by: Jos de Jong --- AUTHORS | 1 + src/function/arithmetic/gcd.js | 35 ++++++++++-- src/function/arithmetic/mod.js | 30 ++++++++-- src/plain/number/arithmetic.js | 15 ++--- .../plain/number/arithmetic.test.js | 55 +++++++++++++++++++ test/unit-tests/expression/parse.test.js | 5 +- .../function/arithmetic/mod.test.js | 10 +++- 7 files changed, 128 insertions(+), 23 deletions(-) create mode 100644 test/node-tests/plain/number/arithmetic.test.js diff --git a/AUTHORS b/AUTHORS index 778df79bcd..d7d7d99d87 100644 --- a/AUTHORS +++ b/AUTHORS @@ -230,6 +230,7 @@ Michael Greminger Kiku MaybePixem <47889538+MaybePixem@users.noreply.github.com> Aly Khaled +Praise Nnamonu BuildTools Anik Patel <74193405+Bobingstern@users.noreply.github.com> Vrushaket Chaudhari <82214275+vrushaket@users.noreply.github.com> diff --git a/src/function/arithmetic/gcd.js b/src/function/arithmetic/gcd.js index 46a32b3e8d..d370c498d6 100644 --- a/src/function/arithmetic/gcd.js +++ b/src/function/arithmetic/gcd.js @@ -1,16 +1,20 @@ +import { isInteger } from '../../utils/number.js' import { factory } from '../../utils/factory.js' +import { createMod } from './mod.js' import { createMatAlgo01xDSid } from '../../type/matrix/utils/matAlgo01xDSid.js' import { createMatAlgo04xSidSid } from '../../type/matrix/utils/matAlgo04xSidSid.js' import { createMatAlgo10xSids } from '../../type/matrix/utils/matAlgo10xSids.js' import { createMatrixAlgorithmSuite } from '../../type/matrix/utils/matrixAlgorithmSuite.js' -import { gcdNumber } from '../../plain/number/index.js' import { ArgumentsError } from '../../error/ArgumentsError.js' const name = 'gcd' const dependencies = [ 'typed', + 'config', + 'round', 'matrix', 'equalScalar', + 'zeros', 'BigNumber', 'DenseMatrix', 'concat' @@ -23,7 +27,8 @@ function is1d (array) { return !array.some(element => Array.isArray(element)) } -export const createGcd = /* #__PURE__ */ factory(name, dependencies, ({ typed, matrix, equalScalar, BigNumber, DenseMatrix, concat }) => { +export const createGcd = /* #__PURE__ */ factory(name, dependencies, ({ typed, matrix, config, round, equalScalar, zeros, BigNumber, DenseMatrix, concat }) => { + const mod = createMod({ typed, config, round, matrix, equalScalar, zeros, DenseMatrix, concat }) const matAlgo01xDSid = createMatAlgo01xDSid({ typed }) const matAlgo04xSidSid = createMatAlgo04xSidSid({ typed, equalScalar }) const matAlgo10xSids = createMatAlgo10xSids({ typed, DenseMatrix }) @@ -57,7 +62,7 @@ export const createGcd = /* #__PURE__ */ factory(name, dependencies, ({ typed, m return typed( name, { - 'number, number': gcdNumber, + 'number, number': _gcdNumber, 'BigNumber, BigNumber': _gcdBigNumber, 'Fraction, Fraction': (x, y) => x.gcd(y) }, @@ -89,6 +94,28 @@ export const createGcd = /* #__PURE__ */ factory(name, dependencies, ({ typed, m } ) + /** + * Calculate gcd for numbers + * @param {number} a + * @param {number} b + * @returns {number} Returns the greatest common denominator of a and b + * @private + */ + function _gcdNumber (a, b) { + if (!isInteger(a) || !isInteger(b)) { + throw new Error('Parameters in function gcd must be integer numbers') + } + + // https://en.wikipedia.org/wiki/Euclidean_algorithm + let r + while (b !== 0) { + r = mod(a, b) + a = b + b = r + } + return (a < 0) ? -a : a + } + /** * Calculate gcd for BigNumbers * @param {BigNumber} a @@ -104,7 +131,7 @@ export const createGcd = /* #__PURE__ */ factory(name, dependencies, ({ typed, m // https://en.wikipedia.org/wiki/Euclidean_algorithm const zero = new BigNumber(0) while (!b.isZero()) { - const r = a.mod(b) + const r = mod(a, b) a = b b = r } diff --git a/src/function/arithmetic/mod.js b/src/function/arithmetic/mod.js index f22492fb81..3228137126 100644 --- a/src/function/arithmetic/mod.js +++ b/src/function/arithmetic/mod.js @@ -1,22 +1,26 @@ import { factory } from '../../utils/factory.js' +import { createFloor } from './floor.js' import { createMatAlgo02xDS0 } from '../../type/matrix/utils/matAlgo02xDS0.js' import { createMatAlgo03xDSf } from '../../type/matrix/utils/matAlgo03xDSf.js' import { createMatAlgo05xSfSf } from '../../type/matrix/utils/matAlgo05xSfSf.js' import { createMatAlgo11xS0s } from '../../type/matrix/utils/matAlgo11xS0s.js' import { createMatAlgo12xSfs } from '../../type/matrix/utils/matAlgo12xSfs.js' -import { modNumber } from '../../plain/number/index.js' import { createMatrixAlgorithmSuite } from '../../type/matrix/utils/matrixAlgorithmSuite.js' const name = 'mod' const dependencies = [ 'typed', + 'config', + 'round', 'matrix', 'equalScalar', + 'zeros', 'DenseMatrix', 'concat' ] -export const createMod = /* #__PURE__ */ factory(name, dependencies, ({ typed, matrix, equalScalar, DenseMatrix, concat }) => { +export const createMod = /* #__PURE__ */ factory(name, dependencies, ({ typed, config, round, matrix, equalScalar, zeros, DenseMatrix, concat }) => { + const floor = createFloor({ typed, config, round, matrix, equalScalar, zeros, DenseMatrix }) const matAlgo02xDS0 = createMatAlgo02xDS0({ typed, equalScalar }) const matAlgo03xDSf = createMatAlgo03xDSf({ typed }) const matAlgo05xSfSf = createMatAlgo05xSfSf({ typed, equalScalar }) @@ -62,13 +66,12 @@ export const createMod = /* #__PURE__ */ factory(name, dependencies, ({ typed, m return typed( name, { - 'number, number': modNumber, + 'number, number': _modNumber, 'BigNumber, BigNumber': function (x, y) { if (y.isNeg()) { throw new Error('Cannot calculate mod for a negative divisor') - } - return y.isZero() ? x : x.mod(y) + } return y.isZero() ? x : x.mod(y) }, 'Fraction, Fraction': function (x, y) { @@ -87,4 +90,21 @@ export const createMod = /* #__PURE__ */ factory(name, dependencies, ({ typed, m sS: matAlgo12xSfs }) ) + + /** + * Calculate the modulus of two numbers + * @param {number} x + * @param {number} y + * @returns {number} res + * @private + */ + function _modNumber (x, y) { + // We don't use JavaScript's % operator here as this doesn't work + // correctly for x < 0 and x === 0 + // see https://en.wikipedia.org/wiki/Modulo_operation + + // We use mathjs floor to handle errors associated with + // precision float approximation + return (y === 0) ? x : x - y * floor(x / y) + } }) diff --git a/src/plain/number/arithmetic.js b/src/plain/number/arithmetic.js index 51b12bdcf0..bb006d4f9a 100644 --- a/src/plain/number/arithmetic.js +++ b/src/plain/number/arithmetic.js @@ -157,17 +157,10 @@ log1pNumber.signature = n1 * @private */ export function modNumber (x, y) { - if (y > 0) { - // We don't use JavaScript's % operator here as this doesn't work - // correctly for x < 0 and x === 0 - // see https://en.wikipedia.org/wiki/Modulo_operation - return x - y * Math.floor(x / y) - } else if (y === 0) { - return x - } else { // y < 0 - // TODO: implement mod for a negative divisor - throw new Error('Cannot calculate mod for a negative divisor') - } + // We don't use JavaScript's % operator here as this doesn't work + // correctly for x < 0 and x === 0 + // see https://en.wikipedia.org/wiki/Modulo_operation + return (y === 0) ? x : x - y * Math.floor(x / y) } modNumber.signature = n2 diff --git a/test/node-tests/plain/number/arithmetic.test.js b/test/node-tests/plain/number/arithmetic.test.js new file mode 100644 index 0000000000..f46da26744 --- /dev/null +++ b/test/node-tests/plain/number/arithmetic.test.js @@ -0,0 +1,55 @@ +// test modNumber function in src\plain\number\arithmetic.js +import assert from 'assert' +import { modNumber as mod } from '../../../../lib/cjs/plain/number/aristhmetic' +import approx from '../../../../tools/approx.js' + +const math = require('../../../../lib/cjs/defaultInstance').default +const bignumber = math.bignumber + +describe('mod', function () { + it('should calculate the modulus of booleans correctly', function () { + assert.strictEqual(mod(true, true), 0) + assert.strictEqual(mod(false, true), 0) + assert.strictEqual(mod(true, false), 1) + assert.strictEqual(mod(false, false), 0) + }) + + it('should calculate the modulus of two numbers', function () { + assert.strictEqual(mod(1, 1), 0) + assert.strictEqual(mod(0, 1), 0) + assert.strictEqual(mod(1, 0), 1) + assert.strictEqual(mod(0, 0), 0) + assert.strictEqual(mod(7, 0), 7) + + approx.equal(mod(7, 2), 1) + approx.equal(mod(9, 3), 0) + approx.equal(mod(10, 4), 2) + approx.equal(mod(-10, 4), 2) + approx.equal(mod(8.2, 3), 2.2) + approx.equal(mod(4, 1.5), 1) + approx.equal(mod(0, 3), 0) + approx.equal(mod(-10, 4), 2) + approx.equal(mod(-5, 3), 1) + }) + + it('should handle precise approximation of float approximation', function () { + approx.equal(mod(0.1, 0.01), 0) + approx.equal(mod(0.15, 0.05), 0) + approx.equal(mod(1.23456789, 0.00000000001), 0) + }) + + it('should calculate mod for negative divisor', function () { + assert.strictEqual(mod(10, -4), -2) + }) + + it('should throw an error if used with wrong number of arguments', function () { + assert.throws(function () { mod(1) }, /TypeError: Too few arguments/) + assert.throws(function () { mod(1, 2, 3) }, /TypeError: Too many arguments/) + }) + + it('should throw an error if used with wrong type of arguments', function () { + assert.throws(function () { mod(1, new Date()) }, /TypeError: Unexpected type of argument/) + assert.throws(function () { mod(1, null) }, /TypeError: Unexpected type of argument/) + assert.throws(function () { mod(new Date(), bignumber(2)) }, /TypeError: Unexpected type of argument/) + }) +}) diff --git a/test/unit-tests/expression/parse.test.js b/test/unit-tests/expression/parse.test.js index 1380d538d0..fb8cca3e54 100644 --- a/test/unit-tests/expression/parse.test.js +++ b/test/unit-tests/expression/parse.test.js @@ -1234,7 +1234,10 @@ describe('parse', function () { it('should parse mod %', function () { approx.equal(parseAndEval('8 % 3'), 2) approx.equal(parseAndEval('80% pi'), 1.4601836602551685) - assert.throws(function () { parseAndEval('3%(-100)') }, /Cannot calculate mod for a negative divisor/) + }) + + it('should parse mod % for negative divisors', function () { + assert.strictEqual(parseAndEval('3%(-100)'), -97) }) it('should parse % value', function () { diff --git a/test/unit-tests/function/arithmetic/mod.test.js b/test/unit-tests/function/arithmetic/mod.test.js index 5eddecf4a1..d7f83f7039 100644 --- a/test/unit-tests/function/arithmetic/mod.test.js +++ b/test/unit-tests/function/arithmetic/mod.test.js @@ -34,8 +34,14 @@ describe('mod', function () { approx.equal(mod(-5, 3), 1) }) - it('should throw an error if the divisor is negative', function () { - assert.throws(function () { mod(10, -4) }) + it('should handle precise approximation of float approximation', function () { + approx.equal(mod(0.1, 0.01), 0) + approx.equal(mod(0.15, 0.05), 0) + approx.equal(mod(1.23456789, 0.00000000001), 0) + }) + + it('should calculate mod for negative divisor', function () { + assert.strictEqual(mod(10, -4), -2) }) it('should throw an error if used with wrong number of arguments', function () {