Enhance helper.decimalPlaces #5992
Conversation
decimalPlaces now uses almostWhole to avoid floating point roundoff errors. almostWhole required a small change to avoid issue when epsilon is so small it doesn't change the sum.
|
Thanks for the contribution, but I feel it would be better to implement rounding function of floating point numbers separately. Currently, this helper method is only used for tick generation and it doesn’t require rounding. As described here, it is impossible to know the right precision only from the result, so I think it should be handled before calling |
|
Thanks for the comments. I'll use this function to calculate the digits in the tick spacing and then format all the ticks with that many digits using toFixed() or toExponential(). For that simple use It matters that the difference between 0.3 and 0.1 requires 1 digit, not 17. Of course you are correct that 0.19999999999999998 has 17 digits but that’s not very useful. Unfortunately, rounding ahead of time doesn’t work reliably. For example, 3.1415e-34 has 38 digits, not 46 as currently returned. As you probably know, the proposed function looks for a large gap in the digits. Such gaps are reasonably good indicators of the end of significant digits especially when working with "nice" values for tick spacing. |
| @@ -126,7 +126,7 @@ module.exports = function() { | |||
| }; | |||
| helpers.almostWhole = function(x, epsilon) { | |||
| var rounded = Math.round(x); | |||
| return (((rounded - epsilon) < x) && ((rounded + epsilon) > x)); | |||
| return (((rounded - epsilon) <= x) && ((rounded + epsilon) >= x)); | |||
| e *= 10; | ||
| p++; | ||
| if (x > 0) { | ||
| while (x < 0.99 || !helpers.almostWhole(x, 1e-6)) { |
There was a problem hiding this comment.
This however fails with large numbers, for example (current version works with these):
expect(helpers.decimalPlaces(12345678.1234)).toBe(4); // this PR results to 9
expect(helpers.decimalPlaces(1234567890.1234567)).toBe(7); // this PR results to 6There was a problem hiding this comment.
Those are good tests and I agree the result is not acceptable. I will review...
|
Hi @costerwi, are you still planning to pursue this PR? It's been inactive for awhile, so please let us know. Thanks! |
|
Sorry, I've been distracted by other things lately. Probably won't get back to this for a while. |
|
@costerwi would you like to just submit your changes to |
|
Yes, please feel free to PR the improved almostWhole. Thanks! |
|
Ok. Thanks for suggesting the fixes! I've sent a PR for the agreed upon parts in #6120 I'm going to close this PR just for the time being to keep the review queue clear and help us get through the outstanding PRs. When you feel like you have a fix you want us to review for Thanks again for this! |
I think the new helper.decimalPlaces() function introduced in #5786 will be very helpful for generating ticks of fixed decimal places or scientific notation. Unfortunately, there were some tricky floating point issues which could fool it into returning too many decimals.
I've added some of those difficult cases to the tests and proposed an alternative calculation method which returns better results.