Bug: rounding error with negative numbers

[evykassirer]

There is an issue because of this line of code:

https://github.com/socraticorg/mathsteps/blob/master/lib/simplifyExpression/arithmeticSearch/index.js#L52

Negative numbers go into the if statement right now, which keeps only 4 significant digits

e.g. -1953125 becomes -1953000

That line should be changed to if (-1 < result < 1), which means if the number is 0. .... or -0. .... it will round to only 4 digits after the decimal, which was my original intention (but I forgot about negative numbers 😅)

This is a one line code change (plus adding a test to make sure it's not broken anymore), which is great for a first PR!

to read more about how we found this issue, see #72

[starside]

Hi, I would love to take on this issue. I have been lurking the mailing list for a while, this seems like a good first step.

[nitin42]

Also see #160

[evykassirer]

Hi @starside ! I'm so excited that you're wanting to get involved with mathsteps!

Unfortunately, for the next few weeks I'm planning to block changes to this project, because there are some people working on a huge change to the codebase that'll probably change almost every line of code here, haha - so I wanna wait until that's done before making any other changes (and I'll need to update open issues and everything once it's done too). You can check out what's going on by looking at the porting branch or reading the open pull requests, or chatting with @aliang8 :)

So so sorry that the moment you expressed interest is one when it's hard for you to contribute, but I'd love for you to get involved as soon as this big reshuffle is done! (Or maybe @aliang8 can find some small tasks to get you started in helping with porting, if you're interested 😄 )

Let me know if you have any questions

[starside]

Thanks! I guess I will have to wait, unless @aliang8 has something for me to do. You are moving to a custom parser correct?

[evykassirer]

Yes! It's going to be awesome. We're also changing the way you define simplifying rules to be more intuitive and easier to write. I'm hoping it'll make contributing to mathsteps much easier, and that it'll also make it easier to implement more complicated cool things soon!!

[aliang8]

Hey @starside!!! Sorry for not responding on this thread. I've been somewhat busy with graduation related events. But regardless, I'm super excited for you to join and if you are interested, I do have some tasks for you to do.

Maybe we can move away from this thread and chat privately on gitter?

[starside]

Gitter works for me

[evykassirer]

Hi @starside ! feel free to make this change now, Socratic has decided to postpone the refactor. It should be just a one line fix. Are you still interested?

Let me know if you need help with anything!

[starside]

Hey! I would like to do it if it is still available. I did not notice your message until now.

[evykassirer]

go for it!

[starside]

Ok, I fixed it and wrote a test. I thought using abs might be a nice solution. I can use only conditionals if you prefer. Let me know if I did something not in the spirit of the project, then I issue a pull request?

function evaluateAndRound(node) {
  let result = evaluate(node);
  if ( Math.abs(result) < 1) {
    result  = parseFloat(result.toPrecision(4));
  }
  else {
    result  = parseFloat(result.toFixed(4));
  }
  return result;
}

I also wrote a test

describe('evaluate arithmeticSearch', function () {
  const tests = [
    ['2+2', '4'],
    ['2*3*5', '30'],
    ['6*6', '36'],
    ['9/4', '9/4'], //  does not divide
    ['---1953125', '-1953125'], // verify large negative number round correctly
  ];
  tests.forEach(t => testArithmeticSearch(t[0], t[1]));
});

[evykassirer]

looks good! feel free to make a pull request even when you're not sure if it's right - it's easier to comment on those than here :)

I think the test would be nicer as 16 - 1953125 since it's more likely to be input than ---1953125

looking forward to the PR and excited about this bug fix! thanks so much for taking it on :)

[evykassirer]

OH and I really like the use of abs - good call :)