Skip to content
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

floor and cell with precision #1967

Merged

Conversation

rnd-debug
Copy link
Contributor

@rnd-debug rnd-debug commented Sep 14, 2020

Provides ceil(x,n) and floor(x,n). Replies to #1901.

  • Shamelessly steals from https://github.com/sindresorhus/round-to as round-to seems to have a nice handling of rounding precision.
  • Had to import Decimal in order to use myBigNumber.toDecimalPlaces(n.toNumber(), Decimal.ROUND_CEIL)

@rnd-debug rnd-debug changed the title floor and cell with precision WIP: floor and cell with precision Sep 14, 2020
@josdejong
Copy link
Owner

@josdejong josdejong commented Sep 16, 2020

Thanks @rnd-debug , looks good at first sight 👍 . I hope to review your PR coming weekend.

@rnd-debug
Copy link
Contributor Author

@rnd-debug rnd-debug commented Sep 18, 2020

One test case is failing on Firefox. Still haven't found the time to check what is happening. Hoping to do it this weekend.

@rnd-debug rnd-debug changed the title WIP: floor and cell with precision floor and cell with precision Sep 19, 2020
@rnd-debug
Copy link
Contributor Author

@rnd-debug rnd-debug commented Sep 19, 2020

@josdejong failing tests fixed: ready to be merged.
[Edit] codecov reports coverage changes on an unchanged file (median.js).

@josdejong
Copy link
Owner

@josdejong josdejong commented Sep 23, 2020

Your PR looks good @rnd-debug , thanks 👍 . Let's just ignore the codecov warnings, I'm not sure what's going on there but the new functionality is well tested.

A few feedback points:

  1. Can you add the new syntax math.floor(x, n) to the main comment in floor.js (used for the docs), like you did for math.ceil(x, n)?
  2. I didn't think about it before, but maybe we should also implement rounding with precision for the function fix, then all related functions (round, floor, ceil, fix) have the same behavior. What do you think?

@rnd-debug
Copy link
Contributor Author

@rnd-debug rnd-debug commented Sep 23, 2020

@josdejong fixing the 'fix' looks like a reasonable idea: will give it more time this weekend. Keeping in mind docs + trying to hack codecov by providing more tests.

@rnd-debug
Copy link
Contributor Author

@rnd-debug rnd-debug commented Sep 27, 2020

@josdejong I am confused about how to deal with rounding errors. According to the proposal for floor(x,n), we will get the following:

mathjs.floor(7.999999999999999, 2);                             // 8  But should it be 8?
mathjs.floor(-7.999999999999999, 2);                           //  -8 

When the external library gives us :

roundTo.down(7.999999999999999, 2);                        // 7.99   not cool
roundTo.down(-7.999999999999999, 2);                      // -8  

Similar issues can appear with mathjs.ceil. Should we be compliant to the external library?

@josdejong
Copy link
Owner

@josdejong josdejong commented Sep 27, 2020

@josdejong I am confused about how to deal with rounding errors.

...

Similar issues can appear with mathjs.ceil. Should we be compliant to the external library?

If you calculate ceil(0.1 + 0.2, 1), what whould you expect to get? 0.1 + 0.2 returns 0.30000000000000004, and technically ceil(0.30000000000000004, 1) should return 0.4. But that's not what we expect: we expect the result to be 0.3, right?

To work around these issues with equal, larger, smaller, but also ceil, fix, floor in mathjs, we first apply a rounding to EPSILON (1e-12 by default) to get rid of those round off errors. After that, we can do the actualy thing we want to do: checking equality, or applying ceil/fix/floor. Does that make sense?

@rnd-debug
Copy link
Contributor Author

@rnd-debug rnd-debug commented Sep 27, 2020

@josdejong Ok, now I got the big picture. Then the branch is ready to be merged.
Also provided a couple of tests for round that seemed to raise the coverage.

* math.round(-4.2) // returns number -4
* math.round(-4.7) // returns number -5
* math.round(3.22, 1) // returns number 3.2
* math.round(3.88, 1) // returns number 3.8
Copy link
Owner

@josdejong josdejong Sep 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think math.round(3.88, 1) returns 3.9 instead of 3.8, right?

@josdejong
Copy link
Owner

@josdejong josdejong commented Sep 27, 2020

Thanks for implementing this extension for fix too, and for the extensive unit tests 👍 ! All looks good, I'll fix the typo I saw in the comments of round.

@josdejong josdejong merged commit 9f06dad into josdejong:develop Sep 27, 2020
4 checks passed
@josdejong
Copy link
Owner

@josdejong josdejong commented Oct 7, 2020

Available now in v7.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants