-
-
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
Nearly equal with relative and absolute tolerance #3152
Conversation
Thanks, this is a great start! Some thoughts:
|
|
(1) You can see the test results by clicking them in the list with checks in this PR, for example https://github.com/josdejong/mathjs/actions/runs/7853061106/job/21432241083?pr=3152. I guess indeed that you somehow have an old version or so (that may also explain why the PR is still testing on Node 16). (5) ha ha, that's true. I'm ok with either |
(1) I was at v 11.9.1 (sorry about that). Should be updated to v12.4.0 by now. Now that it's updated, I see the following errors:
What would be the next steps? |
I think the next step is to work out this PR in detail: update all places where |
Ok, thanks! I will do that and report back. |
Hi, just reporting back. Progress has been slower than what I originally expected but I'm still on it. |
Thanks for the update David, good to know. |
Hi Jos I have some good news! I found a good fit with a relative tolerance the size of the current In the latest submit I made all calls for I would like your advise with the following:
AppendixHere is the mathjs code I used to test for the tolerances: (this works on the current version 12.4.1 config({epsilon:1e-12});
# Define function nearly equal
nearlyEqual(a, b, relTol, absTol) = abs(a-b) <= max(relTol * max(abs(a), abs(b)), absTol);
# Define rtol and atol
rtol = config().epsilon;
atol = rtol * 1e-3;
# Tested various values of x
X = [0, 0.1, 0.5, 1, 10, 1e3, 1e6, 1e12];
x = X[1];
# Ys
powers = -2:-1:-16;
Y = x + map(powers, _(d) = pow(10, d));
# Proposed
nearlyEqualResults = map(Y, _(y) = nearlyEqual(x, y, rtol, atol));
# Current
equalResults = x == Y;
# These are the powers where the current nearly equals differ from the proposed one
differences = nearlyEqualResults != equalResults;
result = {
powersFailing:powers[differences],
x:x,
current:equalResults[differences],
proposed:nearlyEqualResults[differences],
rtol:rtol,
atol:atol
};
print("Testing with:\n
rtol: $rtol
atol: $atol
x: $x\n
yields:
powers faling: $powersFailing
proposal: $proposed
current: $current
", result) I read the following document to realize that |
Yay, that sounds promising!
Looking at the code example, you could definitely use #2953 🤣 |
Thanks!
Absolutely! I was trying to change my point of view about the shorthand and I did.
|
👍 I see you're pushing new commits. Please let me know when the PR is ready for review (or if you hit new challenges). |
Hi Jos, Thanks for the follow up. I'm still tweaking some stuff and will let you know when it's ready for review. |
Hi Jos, this is ready for review |
Thanks, that is good news! I'll look into it asap. |
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.
This looks really solid David, thanks!
Hardly any existing unit test breaks with the new behavior, I'm kind of (happily) surprised by that. I had expected some changes with edge cases of the rounding functions. Or am I over looking something?
I made a couple of inline comments, can you have a look at those?
Hi Jos, thanks for the thorough review! I'm also glad there where not so many errors. The reason might be that the values for the new tolerances were picked to be close to the existing behavior. Maybe at some point in the future the values might change and there will be more discrepancies but at least there is the option to adjust as needed case by case. I will work on the inline comments and report back probably during the weekend. |
Hi Jos, it's ready for review again. I included some inline comments with a few questions. |
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! I've replied to the two left over comments, can you have a look at them again?
Hi Jos, the two left comments were addressed. Please review. |
Thanks David, I'm really happy with how this PR worked out, it's much cleaner than I had expected. Merging this PR now in the |
Thanks Jos! Looking forward to |
Published now in |
Hi,
This initial commit is intended for exploration of #2838. It overwrites the current
nearlyEqual
completely to be as similar to python's Math.isclose as possible (according to the description in the documentation)It currently fails 6 tests:
rotationMatrix
nearlyEqual
(for bigNumber)number / nearlyEqual
No effort was made to either adapt the tests or fix the function. Everything is left as is to review the failing tests and aid in the decision making.
Please note that the functions are changed abruptly only for exploration and can be returned to their original form upon review.
Also no calls for
nearlyEqual
were modified, thus only relative tolerance is being used. If the default values are changed torelTol = Number.EPSILON, absTol = 1e-12
then a few more tests fail.