-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixes #3100 Bug on simple rounding #3136
Fixes #3100 Bug on simple rounding #3136
Conversation
Thanks Brian for your PR! Indeed you can leave A bit more context: in mathjs, there are two "levels" of functions:
Some feedbacks:
|
Thank you @josdejong for the detailed feedback!
My first solution for this bug was actually in the high level function
I will get to work on these revisions today. Thanks again! |
1377c37
to
c53d6d4
Compare
I believe I implemented the changes you requested to this bug fix, @josdejong. Let me know if you want me to adjust anything else. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, this looks good! I made a few inline remarks, can you have a look at those?
I fixed the missing argument in the two regular number functions. I also converted the if...else statements to use the ternary operators as you suggested, and I feel it does make the code look much cleaner. You also asked for unit tests to verify an edge case where a number rounded to the precision of epsilon is not nearly equal to the original number. I spent more time trying to come up with an input that could make that happen than I did making the revisions to the code, but I could not come up with anything. Admittedly, I am not a mathematician (I even had to google the spelling of mathematician, lol) I am just an aspiring software developer pretending to be good at math to work on your software. 😉 But here is the logic as I understand it: Rounding a number to the precision of an epsilon value can not change the number by more than half of the epsilon, which using our default epsilon of 1e-12 would be 5e-13. I ran a bunch of tests while watching the values calculated in nearlyEqual and the relative error that our difference is compared to was usually in the range of xe-11 as shown here using the original value from the bug report:
If you know of an edge case, or can steer me in the right direction to figure one out, I'd be happy to add some lines to the test file for this. Otherwise, I'm content knowing that we should never actually get a false value returned from the nearlyEqual function call. Let me know what you think, I welcome any advice you have good or bad. Thanks! |
😂 I appreciate your work, thanks!
Thinking about it, it makes sense that we do not reach this second case of the original if/else: first we round function (x, n) {
const xEpsilon = roundNumber(x, epsilonExponent)
return roundNumber(xEpsilon, n)
} I think that makes sense: the first step gets rid of any round-off errors, and the second step does the rounding to the actually desired precision. I had one more thought: in case of BigNumber (defaulting to 64 digits), the user will not be able to round to say 20 digits with the default configuration of |
return value was mathematically impossible by user input. Adding dynamic epsilon logic to cover cases when a user requests to round a number to a higher precision than epsilon in the config file. Also adding tests to cover dynamic epsilon cases.
I removed the nearlyEqual comparisons and added a dynamic epsilon to the big number logic to cover cases where users asked for numbers to be rounded to a higher precision than epsilon. I then realized that the same thing could occur if a user tries to round a regular number to more than 12 decimals, so I added the dynamic epsilon there as well. In addition, I added test cases to cover the dynamic epsilon logic and threw in some comments so others will know what the logic is for. Let me know what you think. Thanks! |
Thanks for the updates. You're right, we have to respect the |
I cannot find any new inline comments. I can see the old resolved ones if I look at the older commits, but I see no comments on the newest commit. |
You should see them when scrolling up in this very tab "Conversation" |
Unfortunately, I do not see them in the 'Conversation' tab either. Did you hit 'Submit' on your review in the 'Code' tab? I believe that will prevent others from seeing your comments. |
I found this thread at Stack Overflow ealier when I was looking for the missing comments: https://stackoverflow.com/questions/10248666/how-to-view-line-comments-in-github Apparently, if the comments show 'pending' on your screen nobody else can see them. The first answer talks about how to resolve them being pending. However, I can see them in your screenshot so I'll get to work on them. As far as the (n < 15) and (n < 64) comparison, the low level roundNumber function will only round to 15 decimal places so if I add one to n and it is already 15 it will cause an error. I was under the impression that big numbers were limited to 64 decimals and was making a similar verification. What is the max number of decimals for a big number then? Regarding the new Bignumber and toNumber() logic, that was the only way to make the unit test pass. If I did it any other way it would make other scenarios not work (I believe it was the complex number one). The logic in my head was that the original function was supposed to have a big number type as both the x and the n so I decided to give big number types to the returned function and use it as written (except for the xEplsilon part). This was the original returned funtion:
I can try to find another solution though, if you'd like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clicked "submit review" now, can you see the comments now?
Yes, I can see them now |
Well, I have no new commits for you today. In fact, I am leaning towards recommending for you to just reject this pull request and the associated issue all together. I thought rounding to epsilon precision before rounding a number to a whole value was safe mathematically and would handle round off error. I later on thought that if someone wanted to round to a higher precision than epsilon, we could just round to one higher decimal place first, then round to what they requested. However, I have now found an edge case than breaks both of those theories. If a user wants to round the actual number of 1.4999999999995 (13 decimal places) without specifying a number of decimal places and we round it to epsilon precision first it will round to 2 instead of 1 like it should. This example could also be changed to break any epsilon with a negative 'e' value. Similarly, if the user wants We cannot use nearly equal to compare the final rounding number or the solution wouldn't fix the example in the example posted in the issue. So, what is worse round off errors or these errors that we would induce with the fix to round off errors? |
😅 that's good to hear. Sorry, my bad. I didn't see that I was inside a review mode and that the comments where pending.
Hm. Good to think this through. Isn't the thing that you can't fully represent |
@BrianFugate I have given this some more thought. As far as I can see though there is not a problem, as long as you recon with the configured |
Hi, just a comment Configuring Previously I mistakenly thought that epsilon did not work. My mistake was because I thought it was absolute when currently it's relative. |
@dvd101x ah, you're right. Originally Brian had this construct |
With this latest commit I removed the dynamic epsilon logic and made it just skip the roundoff error correction if the user requests the same or more decimals as config.epsilon has. I also added a test to the unit test file to verify that our outputs change if epsilon is changed during runtime. Look closely at the values of the assertations so you are sure this is how you want it to operate. Let me know what you think. Thanks! |
Thanks for the updates Brian, I think this is exactly what we're looking for. I still want to think it over in the weekend to be sure we're not overlooking anything obvious here 🤔. Do you still have concerns on whether this actually makes sense? |
@josdejong by trying a shotgun approach of testing:
I found the following cases where the expression
|
Thanks @dvd101x, this helps a lot. So, we need to test (a couple of) these cases to see whether the current implementation of this PR gives the right results. If not, I think we have to re-introduce the initial approach of @BrianFugate with |
round function. Adding test case for changing epsilon at runtime. Both tests for changing epsilon at runtime also verify the false nearlyEqual scenario.
I reintroduced the nearlyEqual comparison and immediately realized that my test case for changing epsilon at runtime was also a false case in the nearlyEqual comparison. In addition, I added a test using bignumbers to the same changing epsilon test and it also is a false in with nearly equal. I feel confident in this now finally, thank you @dvd101x for helping get me back on track! Thanks! |
@BrianFugate @dvd101x can you come up with a unit test for each of the four function signatures that fails when we remove the EDIT: sorry, I see that both number and BigNumber implementations of |
I do not think there is a case where we can make
Unless there are plans to make the low level function |
I think indeed that we could remove the OK then, let's leave it as it is right now, with 4x the same implementation, OK? |
Sounds good to me! |
OK merging your PR now. Brian, thanks a lot for your patience and for thinking along. This was trickier than I had expected. |
Published now in |
Fixed rounding bug described by @adshrc in issue #3100. I started with Jos' advice in the comments of the issue and also found help from the mathjs documentation here and from a similar issue here.
I addition to fixing the bug, I added two lines to the round function unit test. One for @adshrc's exact bug and a second for essentially the same bug that would also occur if the number of digits was specified with a second arg to the function call.
Thanks for the opportunity to contribute!
FYI, I didn't move David's name in the AUTHORS file. It looks like it made the opposite move in his last pull request that you just approved...