-
Notifications
You must be signed in to change notification settings - Fork 128
Fix relative comparison in CompareWorkspaces
#37889
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
Conversation
b96591a
to
a4a06a1
Compare
4f5b81c
to
b721688
Compare
4098402
to
0701700
Compare
@returns true if absolute difference is within the tolerance; false otherwise | ||
*/ | ||
bool CompareWorkspaces::withinAbsoluteTolerance(double const x1, double const x2, double const atol) { | ||
// NOTE !(|x1-x2| > atol) is not the same as |x1-x2| <= atol |
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.
very good comment
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 anyone from the future coming to this PR and wondering about this comment. Mathematically they are identical, but they do not evaluate the same in some of the tests.
The checks throughout the algo were originally written to expect false
for a match. It was originally checking
It seemed to make more sense to return true
for a match. I changed it to return
Two possibilities are values such as NaN
, or machine precision issues, which could cause the two comparison to be mechanically different.
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 was curious why. std::nan
is equal to everything, but isn't greater-than anything
double const num = std::fabs(x1 - x2); | ||
// create a standardized size to compare against | ||
double const den = 0.5 * (std::fabs(x1) + std::fabs(x2)); | ||
return !(num > (rtol * den)); |
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.
please add the comment about this not being the same as <=
from above
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.
Will do. People from the future, please see this note above.
Please can you remember to add a milestone? it helps with audit trail for the release. |
678870e
to
eaf3985
Compare
Is there a reason to not add the test you wrote in the description as a unit test? I think it would be a nice addition. |
@thomashampson I considered adding a test based on the script, but thought that the tests added in PR #37878 effectively cover the situation. I can add this test in |
@thomashampson a similar test of the relative comparison will be added in PR #37933 |
Description of work
Summary of work
CompareWorkspaces
is supposed to be usable for both absolute and relative comparisons.There are two situations where relative differences may be preferred over absolute differences.
The comparison of relative difference was originally written as:
This short-circuits any use of relative differences for case 2 above, instead performing an absolute difference in a situation where absolute difference is irrelevant.
This changes that calculation, to just do relative difference.
If the user asks for the relative difference, the user gets a relative difference.
Fixes #37877 .
Further detail of work
This also caused one unit test and three system tests to fail.
The one unit test could be fixed by ever-so-slightly broadening the tolerance on the comparison.
The three system tests could be fixed by using absolute comparison instead (which is what they would have been doing anyway).
To test:
Try running this script in workbench built from this branch. If the issue has been fixed, then this should pass
Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeepers
will take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.