New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

math.simplify('1e-10') returns 0 #813

Closed
josdejong opened this Issue Mar 23, 2017 · 14 comments

Comments

Projects
None yet
3 participants
@josdejong
Owner

josdejong commented Mar 23, 2017

I saw this issue on stackoverflow:

http://stackoverflow.com/questions/42838708/why-math-simplify1e-10-tostring-of-math-js-returns-0

When you simplify a very small constant, zero is returned instead.

I haven't looked deeply into it but I think the issue is caused by this piece of code which turns a number into a fraction:

https://github.com/josdejong/mathjs/blob/master/lib/function/algebra/simplify/simplifyConstant.js#L49-L54

    'number': function(s) {
      if (digits(s) <= 15) {
        return math.fraction(s);
      }
      return s;
    },

Apparently Fraction.js rounds such a small value to zero:

math.fraction(1e-10)  // Fraction {d: 1, n: 0, s: 1}

@josdejong josdejong added the bug label Mar 23, 2017

@tetslee

This comment has been minimized.

Contributor

tetslee commented Apr 13, 2017

That piece of code could be improved by doing something like:

    'number': function(s) {
      if (isFinite(s)) { // oh yeah, this check is unrelated but should be added too
        var f = math.fraction(s);
        if (f.valueOf() === s) {
          return f;
        }
      }
      return s;
    },

But even after only explicitly converting fractions that have exact representations, we get the same problem with implicit conversions (that happen later in simplifyConstant):

  math.multiply(math.fraction(2), 1e-15)  // Fraction {d: 1, n: 0, s: 1}

The generated typed-function of math.multiply is converting the 1e-15 to a Fraction. Maybe it should first be doing a check to see that it can be represented exactly as a Fraction, and if not then instead convert the first parameter from Fraction -> number.

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 14, 2017

Maybe it should first be doing a check to see that it can be represented exactly as a Fraction, and if not then instead convert the first parameter from Fraction -> number.

That makes sense, I think your proposal is a neat one (check f.valueOf() === s).

Beyond that, I think we just have to accept this as one of the limitations of fractions. Just when working with numbers (floats) where you have to accept round off errors.

Would you be interested in creating a fix for this?

@tetslee

This comment has been minimized.

Contributor

tetslee commented May 5, 2017

Sorry for the slow reply, I was on holiday for the last few weeks. I've added the pull request #841 for fixing this bug.

The first commit stops simplify explicitly converting to inexact Fractions (fixing math.simplify('1e-10')).

The second commit is an example of fixing the implicit conversion to inexact Fractions, for the case of multiply (fixing math.multiply(math.fraction(2), 1e-15)). It currently breaks a few tests that would have to be looked into, and it would have to be implemented for the other operators too, but first of all do you think this is the right path to go down?

@josdejong

This comment has been minimized.

Owner

josdejong commented May 7, 2017

Thanks @tetslee, hope you had a great time these weeks :)

The first commit in your PR is a nice fix, we should definitely merge that one. (simplify only converting to Fraction when not losing digits)

I think we should work out your second commit in a different way. Working out special cases for every operation with mixed Fractions and numbers will be a huge job (and just for handling edge cases). I think we will only be able to partly solve the problem of not losing digits, whilst giving the users an expectation that it always works. Also, one time returning a number and other times a Fraction may be confusing too. I've tried something similar with numbers and BigNumbers, and that was a painful exercise only resulting in complicated code and confusing, fuzzy behavior when seen from the outside.

Maybe we could try a different sort of solution. Currently when implicitly converting a number to a Fraction, there is a check to see whether we wouldn't loose precision (see code). However, this mechanism which checks whether the value has less than 15 digits just not functions correctly since Fraction cannot handle for example 1e-10. Maybe we should simply replace this check such that the conversion function validates whether it's .valueOf() is the same as the original value instead. What do you think?

@onzag

This comment has been minimized.

onzag commented May 7, 2017

Also math.simplify("111111111111111111 + 111111111111111111") gives 222222222222222200 even when I set the precision to 1024 and bignumbers.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 7, 2017

Thanks @onzag. I see math.simplify doesn't propagate constants with type BigNumber correctly, they are converted to regular numbers. We should address that too.

@tetslee

This comment has been minimized.

Contributor

tetslee commented May 8, 2017

Yep your suggestion is much better, I'll update the PR to do that.
Will also look at fixing the BigNumber problem, I'll just add it to the same PR if it's a small change.

@onzag

This comment has been minimized.

onzag commented May 8, 2017

@josdejong shouldn't everything use bignumber or some sort of wrapper if I you set it with the configuration? For instance I'm not concerned with time, I have over a second to perform every calculation; but precision is very important for me. I'm making a very special programming language and it'll be powered by mathjs in the browser.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 8, 2017

@tetslee cool, your PR looks good to go now - I will wait merging until you checked whether it's a simple fix to reckon with the number/BigNumber setting.

@onzag that's indeed the idea, when you have configured to use BigNumbers, numeric values will be created as BigNumbers, like in math.eval('2 + 3'). When input is a number though, output will be a number too: math.add(2, 3) still returns a number when you've configured to use BigNumbers since the input was two numbers.

@tetslee

This comment has been minimized.

Contributor

tetslee commented May 9, 2017

@josdejong BigNumber support has been added, I think it's ready to merge.
I should note that BigNumbers are treated differently from normal numbers (they are not converted to fractions):

math.create({number:'BigNumber'}).simplify('1/3').toString() // '0.3333333333....'

math.simplify('1/3').toString() // '1 / 3'

It was just simpler to do it that way and you have control over any loss of precision when using BigNumber so I think it's OK.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 9, 2017

Awesome, great job. Indeed BigNumbers shouldn't convert to Fractions. (It would be really cool though if some day we could have Fractions defined with two BigNumbers :) )

@onzag

This comment has been minimized.

onzag commented May 10, 2017

:D

@onzag

This comment has been minimized.

onzag commented May 18, 2017

Any news on the state of this issue?... I keep checking with the math.simplify("111111111111111111 + 111111111111111111") problem I commented, I have been doing npm update since but no bugfixes so far.

@josdejong

This comment has been minimized.

Owner

josdejong commented May 18, 2017

The problem was fixed in v3.13.0 (see history, see PR #841), I forgot to close this issue, sorry.

math.config({number: 'BigNumber'})

math.simplify("111111111111111111 + 111111111111111111").eval() 
    // returns a BigNumber 222222222222222222

and

math.simplify('1e-10')  // returns number 1e-10

@josdejong josdejong closed this Jun 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment