Skip to content

Conversation

rboston628
Copy link
Contributor

@rboston628 rboston628 commented Aug 28, 2024

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.

  1. One is the case of very large numbers. Values may match to ten digits, but not match out to the thousandths place, which would make an absolute difference fail.
  2. The other is the case of very small numbers, where the values themselves are below a tolerance. Here, an absolute difference below the tolerance is meaningless, and the numbers should be checked for relative closeness.

The comparison of relative difference was originally written as:

bool CompareWorkspaces::relErr(double x1, double x2, double errorVal) const {
  double num = std::fabs(x1 - x2);
  // how to treat x1<0 and x2 > 0 ?  probably this way
  double den = 0.5 * (std::fabs(x1) + std::fabs(x2));
  if (den < errorVal)
    return (num > errorVal);

  return (num / den > errorVal);
}

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

# import mantid algorithms, numpy and matplotlib
from mantid.simpleapi import *
import matplotlib.pyplot as plt
import numpy as np

tol = 0.01
x1 = 0.00001
x2 = 0.00010
ws1 = CreateWorkspace(DataX=[x1], DataY=[1])
ws2 = CreateWorkspace(DataX=[x2], DataY=[1])
print( (x2-x1)/(0.5 * (x2+x1)) )
assert not (x2-x1)/(0.5 * (x2+x1)) <= tol
res, msg = CompareWorkspaces(ws1, ws2, Tolerance=tol, ToleranceRelErr=True)
print(res)
assert not res

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

  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Do the release notes conform to the release notes guide?

Functional Tests

  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

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.

@rboston628 rboston628 force-pushed the ewm6962-fix-compare-workspaces branch from 4f5b81c to b721688 Compare August 29, 2024 14:12
@rboston628 rboston628 force-pushed the ewm6962-fix-compare-workspaces branch from 4098402 to 0701700 Compare August 30, 2024 13:26
@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
Copy link
Member

Choose a reason for hiding this comment

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

very good comment

Copy link
Contributor Author

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 $\vert x_1 - x_2 \vert &gt; atol$.

It seemed to make more sense to return true for a match. I changed it to return $\vert x_1 - x_2 \vert \leq atol$. This caused certain tests to fail. I changed it to its current form, and the same tests do not fail.

Two possibilities are values such as NaN, or machine precision issues, which could cause the two comparison to be mechanically different.

Copy link
Member

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));
Copy link
Member

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

Copy link
Contributor Author

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.

@rboston628 rboston628 marked this pull request as ready for review August 30, 2024 15:50
@sf1919 sf1919 added this to the Release 6.11 milestone Sep 2, 2024
@sf1919
Copy link
Contributor

sf1919 commented Sep 2, 2024

Please can you remember to add a milestone? it helps with audit trail for the release.

@rboston628 rboston628 force-pushed the ewm6962-fix-compare-workspaces branch from 678870e to eaf3985 Compare September 3, 2024 13:03
@jmborr jmborr merged commit d201724 into main Sep 3, 2024
10 checks passed
@jmborr jmborr deleted the ewm6962-fix-compare-workspaces branch September 3, 2024 15:36
@thomashampson
Copy link
Contributor

thomashampson commented Sep 4, 2024

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.

@rboston628
Copy link
Contributor Author

@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 CompareWorkspacesTest.h as well if desired.

@rboston628
Copy link
Contributor Author

@thomashampson a similar test of the relative comparison will be added in PR #37933

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.

CompareWorkspaces with relative tolerance incorrectly evaluating to true
5 participants