google / mathsteps

Step by step math solutions for everyone
https://socratic.org
Apache License 2.0
2.12k stars 274 forks source link

Roundoff error when calculating perfect root of a constant #223

Closed rickpock closed 6 years ago

rickpock commented 6 years ago

Example:

nthRoot(64, 3) is simplified as 2 * 2 instead of the expected 4.

  1) simplify nthRoot nthRoot(64, 3) -> 4:

      AssertionError [ERR_ASSERTION]: '2 * 2' deepEqual '4'
      + expected - actual

      -2 * 2
      +4

This is due to a roundoff error in the math.nthRoot functions. If math.nthRoot doesn't evaluate to an integer due to roundoff error, it's assumed not to be a perfect root, and the radicand is factored first.

lib/simplifyExpression/functionsSearch/nthRoot.js:

  const radicandValue = parseFloat(radicandNode.value);
  const rootValue = parseFloat(rootNode.value);
  const nthRootValue = math.nthRoot(radicandValue, rootValue);
  // Perfect root e.g. nthRoot(4, 2) = 2
  if (nthRootValue % 1 === 0) {
    ...
  }
> math.nthRoot(64, 3);
3.9999999999999996
evykassirer commented 6 years ago

Thanks for finding this issue and reporting it!

Damn - is this one of those floats being inaccurate problems? It'd be nice if mathJS's math.nthRoot gave the right value - that's the main issue here, yeah?

rickpock commented 6 years ago

I suspect the underlying inaccuracy of floats is what causes this to happen.

I recommend checking it by rounding the return value from nthRoot, raising it to the exponent and seeing if that matches the radicand. I have this mostly done, but can't commit/push until this evening. Also, I've never pushed to OSS, so I might bungle my way through.

By the way, I encountered this while starting to implement support for nthRoot(-1, n). I had nthRoot(-64, 3) -> -4 as a test case, and I had to dig into why it's failing.

evykassirer commented 6 years ago

that sounds like a slightly hacky but pretty good fix! If you have any questions on how to contribute your fix, I am happy to help with that :) and no rush - this evening is totally fine

And ooo support for that would be awesome! Thanks for looking into that

rickpock commented 6 years ago

Thanks. Do I need to be added to the project team somehow in order to push?

This is where I get confused on contributing. My normal workflow is to create a new branch, make the fix on that branch, push the branch to github, then make a pull request.

I'm unable to push the branch to github.

[rickpocklington: ~/mathsteps (fix223)]$ git push --set-upstream origin fix223
ERROR: Permission to socraticorg/mathsteps.git denied to rickpock.
fatal: Could not read from remote repository.
evykassirer commented 6 years ago

ah! yeah I got used to that at work + with my own projects too ^_^

You need to fork the repo, copy your changes there, then push to your fork - then you can PR from that fork