Repository: josdejong/mathjs
Pull request #841 (2017-05-05 05:33:47)

Stop simplify converting numbers to inexact Fractions
Partial fix for #813.

simplifyConstant changed to only explicitly convert to Fractions when the Fraction can be expressed exactly.

To fix this problem fully, all binary operators with typed-functions that have one Fraction type (e.g. 'Fraction, number') should convert all parameters to Fractions if they can all be expressed exactly, otherwise convert all parameters to the 'number' type.


Patch:
diff --git a/lib/core/typed.js b/lib/core/typed.js
index 147abd3c0..e15d9f0e8 100644
--- a/lib/core/typed.js
+++ b/lib/core/typed.js
@@ -45,7 +45,7 @@ exports.create = function create(type) {
     { name: 'Object',          test: function (x) { return typeof x === 'object' } },
     { name: 'null',            test: function (x) { return x === null } },
     { name: 'undefined',       test: function (x) { return x === undefined } },
-    
+
     { name: 'OperatorNode',    test: function (x) { return x && x.isOperatorNode } },
     { name: 'ConstantNode',    test: function (x) { return x && x.isConstantNode } },
     { name: 'SymbolNode',      test: function (x) { return x && x.isSymbolNode } },
@@ -111,8 +111,9 @@ exports.create = function create(type) {
       from: 'number',
       to: 'Fraction',
       convert: function (x) {
-        if (digits(x) > 15) {
-          throw new TypeError('Cannot implicitly convert a number with >15 significant digits to Fraction ' +
+        var f = new type.Fraction(x);
+        if (f.valueOf() !== x) {
+          throw new TypeError('Cannot implicitly convert a number to a Fraction when there will be a loss of precision ' +
               '(value: ' + x + '). ' +
               'Use function fraction(x) to convert to Fraction.');
         }
diff --git a/lib/function/algebra/simplify/simplifyConstant.js b/lib/function/algebra/simplify/simplifyConstant.js
index c7a0677e7..5230395a1 100644
--- a/lib/function/algebra/simplify/simplifyConstant.js
+++ b/lib/function/algebra/simplify/simplifyConstant.js
@@ -21,54 +21,82 @@ function factory(type, config, load, typed, math) {
       return _toNumber(math[fnname].apply(null, args));
     }
     catch (ignore) {
-    // sometimes the implicit type conversion causes the evaluation to fail, so we'll try again using just numbers
-      args = args.map(function(x){ return x.valueOf(); });
+      // sometimes the implicit type conversion causes the evaluation to fail, so we'll try again after removing Fractions
+      args = args.map(function(x){
+        if (x.isFraction) {
+          return x.valueOf();
+        }
+        return x;
+      });
       return _toNumber(math[fnname].apply(null, args));
     }
   }
 
   var _toNode = typed({
     'Fraction': _fractionToNode,
-    'number': _numberToNode,
-    'BigNumber': function(s) {
-      return _numberToNode(s._toNumber());
+    'number': function(n) {
+      if (n < 0) {
+        return unaryMinusNode(new ConstantNode(-n));
+      }
+      return new ConstantNode(n);
+    },
+    'BigNumber': function(n) {
+      if (n < 0) {
+        return unaryMinusNode(new ConstantNode(n.negated().toString(), 'number'));
+      }
+      return new ConstantNode(n.toString(), 'number');
     },
     'Complex': function(s) {
       throw 'Cannot convert Complex number to Node';
     }
   });
 
+  // convert a number to a fraction only if it can be expressed exactly
+  function _exactFraction(n) {
+    if (isFinite(n)) {
+      var f = math.fraction(n);
+      if (f.valueOf() === n) {
+        return f;
+      }
+    }
+    return n;
+  }
+
+  // Convert numbers to a preferred number type in preference order: Fraction, number, Complex
+  // BigNumbers are left alone
   var _toNumber = typed({
-    'Fraction': function(s) { return s; },
-    'BigNumber': function(s) {
-      if (s.decimalPlaces() <= 15) {
-        return math.fraction(s.toNumber())
+    'string': function(s) {
+      if (config.number === 'BigNumber') {
+        return math.bignumber(s);
       }
-      return s.toNumber();
-    },
-    'number': function(s) {
-      if (digits(s) <= 15) {
+      else if (config.number === 'Fraction') {
         return math.fraction(s);
       }
-      return s;
+      else {
+        return _exactFraction(parseFloat(s));
+      }
     },
+
+    'Fraction': function(s) { return s; },
+
+    'BigNumber': function(s) { return s; },
+
+    'number': function(s) {
+      return _exactFraction(s);
+    },
+
     'Complex': function(s) {
       if (s.im !== 0) {
         return s;
       }
-      if (digits(s.re) <= 15) {
-        return math.fraction(s.re);
-      }
-      return s.re;
+      return _exactFraction(s.re);
     },
   });
 
-  function _numberToNode(n) {
-    if (n < 0) {
-      return new OperatorNode('-', 'unaryMinus', [new ConstantNode(-n)])
-    }
-    return new ConstantNode(n);
+  function unaryMinusNode(n) {
+    return new OperatorNode('-', 'unaryMinus', [n]);
   }
+
   function _fractionToNode(f) {
     var n;
     var vn = f.s*f.n;
@@ -122,7 +150,10 @@ function factory(type, config, load, typed, math) {
       case 'SymbolNode':
         return node;
       case 'ConstantNode':
-        return _toNumber(node.value);
+        if (node.valueType === 'number') {
+          return _toNumber(node.value);
+        }
+        return node;
       case 'FunctionNode':
         if (math[node.name] && math[node.name].rawArgs) {
           return node;


Test patch:
diff --git a/test/function/algebra/simplify.test.js b/test/function/algebra/simplify.test.js
index 94ea9d4a4..4c6b27c61 100644
--- a/test/function/algebra/simplify.test.js
+++ b/test/function/algebra/simplify.test.js
@@ -25,6 +25,35 @@ describe('simplify', function() {
     simplifyAndCompare('3+2/4', '7/2');
   });
 
+  it('should preserve the value of BigNumbers', function() {
+    var bigmath = math.create({number: 'BigNumber', precision: 64});
+    assert.deepEqual(bigmath.simplify('111111111111111111 + 111111111111111111').eval(), bigmath.eval('222222222222222222'));
+    assert.deepEqual(bigmath.simplify('1 + 111111111111111111').eval(), bigmath.eval('111111111111111112'));
+    assert.deepEqual(bigmath.simplify('1/2 + 11111111111111111111').eval(), bigmath.eval('11111111111111111111.5'));
+    assert.deepEqual(bigmath.simplify('1/3 + 11111111111111111111').eval(), bigmath.eval('11111111111111111111.33333333333333333333333333333333333333333333'));
+    assert.deepEqual(bigmath.simplify('3 + 1 / 11111111111111111111').eval(), bigmath.eval('3 + 1 / 11111111111111111111'));
+  });
+
+  it('should not change the value of numbers when converting to fractions (1)', function() {
+    simplifyAndCompareEval('1e-10', '1e-10');
+  });
+
+  it('should not change the value of numbers when converting to fractions (2)', function() {
+    simplifyAndCompareEval('0.2 * 1e-14', '2e-15');
+  });
+
+  it.skip('should not change the value of numbers when converting to fractions (3)', function() {
+    // TODO this requires that all operators and functions have the correct logic in their 'Fraction' typed-functions.
+    //      Ideally they should convert parameters to Fractions if they can all be expressed exactly,
+    //      otherwise convert all parameters to the 'number' type.
+    simplifyAndCompareEval('1 - 1e-10', '1 - 1e-10');
+    simplifyAndCompareEval('1 + 1e-10', '1 + 1e-10');
+    simplifyAndCompareEval('1e-10 / 2', '1e-10 / 2');
+    simplifyAndCompareEval('(1e-5)^2', '(1e-5)^2');
+    simplifyAndCompareEval('min(1, -1e-10)', '-1e-10');
+    simplifyAndCompareEval('max(1e-10, -1)', '1e-10');
+  });
+
   it('should simplify non-rational expressions with no symbols to number', function() {
     simplifyAndCompare('3+sin(4)', '2.2431975046920716');
   });
diff --git a/test/function/arithmetic/addScalar.test.js b/test/function/arithmetic/addScalar.test.js
index 115beac44..6e1ba7f56 100644
--- a/test/function/arithmetic/addScalar.test.js
+++ b/test/function/arithmetic/addScalar.test.js
@@ -80,10 +80,10 @@ describe('add', function() {
     assert.deepEqual(add(math.fraction(1,3), 1), math.fraction(4,3));
   });
 
-  it('should throw an error when converting a number with 15+ digits to fraction', function() {
+  it('should throw an error when converting a number to a fraction that is not an exact representation', function() {
     assert.throws(function () {
       add(math.pi, math.fraction(1,3))
-    }, /Cannot implicitly convert a number with >15 significant digits to Fraction/);
+    }, /Cannot implicitly convert a number to a Fraction when there will be a loss of precision/);
   });
 
   it('should add strings to numbers', function() {