Rounding issue for negative numbers about mathsteps HOT 13 CLOSED

added the small tag, cause it's a one line fix.

from mathsteps.

Hm, wait really?

I think toFixed only rounds after the decimal point.

I'm confused where you'd get the rounding to -1953000 (but eager to find out!)

from mathsteps.

@evykassirer I haven't inserted a console.log to check but I think since -1953125 < 1 it executes line 54 (https://github.com/socraticorg/mathsteps/blob/master/lib/simplifyExpression/arithmeticSearch/index.js#L54) and

> parseFloat((-1953125).toPrecision(4))
-1953000

from mathsteps.

OMG haha yeah that's it!

Thanks for finding that. So it just needs to be changed to if (-1 < result < 1) { then, yeah?

from mathsteps.

I think so; I am not quite sure that the goal of that line was (use toPrecision instead of toFixed when < 1) but if you say so!

By the way, I found this error by writing a generative test (like Haskell's Quickcheck) for simplifyExpression. It ended up being quite useless; the project seems very well tested and my code found errors which were just missing implementations, but yeah... (if it had any value I would have made a PR).

from mathsteps.

oh, we should add a comment there then! (you can add it if you decide to take on this PR)

toFixed makes sense in most cases, and rounds to 4 digits after the decimal.

However, for something like 0.0000834567, it'd be nice to keep 4 digits of accuracy, not including the leading 0s (ie round to 0.00008346). So toPrecision is used to keep precision for small numbers (ones that don't have any digits before the decimal point). That's why it should be changed to -1 < 0 < 1 -- I guess we just forgot that negative numbers exist when writing this 😛

from mathsteps.

Also neat that you tried that - I'm super glad our tests are solid!! Great to hear.

If anything wasn't implemented but didn't have an issue for implementing it in the future (and you think it would be nice to cover) feel free to make an issue for it!

from mathsteps.

I looked played around with this issue for a bit this morning.

Changing that line causes a regression in test:
test/solveEquation/solveEquation.test.js for case:

solveEquation for = ( u )/( 0.3) = 4u + 6.28 -> u = -9.42:

expected: -9.4195
actual: -9.42

Am I good to change the expectation to -9.42 now that the precision is changed?

from mathsteps.

thanks for taking this on!

It's actually expecting -9.42 (which is the right answer) and getting -9.4195 ... so that's too bad :(

But the only reason it was getting the answer right before was because it was rounding to -9.420, so it was just rounding luckily. This is an issue that was already happening, so your change didn't create the problem, it just revealed it for this case.

I think the best thing to do here is comment out that test and move it below the comment about rounding errors (a few lines lower). We've been doing a bunch of thinking about solutions in #64 if you're interested.

How's that sound?

from mathsteps.

@emars just checking in - how's this going? need help with anything?

from mathsteps.

Hey sorry, I got busy with work and completely forgot lol.

I'll tackle this tomorrow unless somebody else is interested. Let me know.

from mathsteps.

@emars still wanna tackle this?

(if you're not free/interested that's fine - we can open it up as a good first issue for newcomers)

from mathsteps.

I'm going to close this and use a new issue to more accurately describe the issue for newcomers :)

from mathsteps.