`CompareWorkspaces` with relative tolerance incorrectly evaluating to true

[rboston628]

Describe the bug

CompareWorkspaces is not properly handling relative differences when the size of the values is smaller than the tolerance.

This error is due to L1361-1362 in CompareWorkspaces.cpp. I am unsure why this was originally implemented this way.

The tests of CompareWorkspaces are under-developed for testing relative errors. Only a single test tries to make use of relative differences, while more should be used.

To Reproduce
Open workbench. Run the following:

# 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

The relative tolerance is > 1, and the relative tolerance is 0.01, and yet the last assert will fail (meaning the workspaces compare equal).

Expected behavior
Checks of relative error with CompareWorkspaces should fail if the relative error is above the tolerance. Suggested implementation

bool CompareWorkspaces::relErr(const double x1, const double x2, const double rtol) const {
  const double numerator = std::fabs(x1 - x2);
  if (numerator == 0.)
    return true;

  const double denomenator = 0.5 * std::fabs(x1 + x2);

  return numerator < rtol * denomenator;
}

To make this easier to add unit tests, making this method static and public would be helpful.

Platform/Version (please complete the following information):

• OS: Ubuntu
• OS Version: 22.04.4
• Mantid Version: 6.10.20240823

Additional context

Was discovered while writing tests in PR #37878
Will be fixed by PR #37889

[rboston628]

It's worth noting the doc-string for the method says the following:

//------------------------------------------------------------------------------------------------
/** Function which calculates relative error between two values and analyses if
this error is within the limits
* requested. When the absolute value of the difference is smaller then the value
of the error requested,
* absolute error is used instead of relative error.

@param x1       -- first value to check difference
@param x2       -- second value to check difference
@param errorVal -- the value of the error, to check against. Should  be large
then 0

@returns true if error or false if the value is within the limits requested
*/

This explanation does not occur in the algorithm documentation.

This behavior is not desired. It may make sense in certain contexts to perform an absolute comparison instead for very small differences. It does not always make sense, and users should choose when it makes sense. This behavior should be controlled by a separate tolerance, as the scale when this makes sense is not the same as the desired relative precision.

For simplicity, CompareWorkspaces should just do what the user asks, and not try to out-think the users. If the user wants a relative comparison, do a relative comparison. If they want to control for machine precision, they can do so on their own.

---

I thought about this more, and actually I don't think there is ever any reason for this implementation.

The way it was implemented switches to absolute differences if the standard size of the values is very small. That situation -- very small sizes -- is exactly the situations where we would want to use a relative tolerance vs. an absolute one in the first place.

The situation that might make more sense is if the size were $>1$; then an absolute difference below the tolerance ensures a relative difference below the tolerance.

[rboston628]

Fixes generate failures in the following test suites:

        240 - AlgorithmsTest_CreateUserDefinedBackgroundTest (Failed)
        1605 - python.algorithms.SaveISISReflectometryORSOTest.SaveISISReflectometryORSOTest (Failed)
        1642 - python.WorkflowAlgorithms.BayesQuasiTest.BayesQuasiTest (Failed)
        2002 - MantidQtWidgetsCommonTestQt5_AlgorithmProgressDialogPresenterTest (Failed)

[rboston628]

The following systems tests are also impacted:

SystemTests.PaalmanPingsMonteCarloAbsorptionTest.AnnulusTest
SystemTests.DirectILLAutoProcessTest.DirectILLAuto_PANTHER_Powder_Test
SystemTests.ILLDirectGeometryReductionTest.IN6

[rboston628]

This is worth noting for the future. The code that was implemented was

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);
}

However, what I believe it wanted to implement, was something like this method from TableColumn.h

bool compareVectorsRelError(const std::vector<Type> &newVector, double tolerance) const {
    for (size_t i = 0; i < m_data.size(); i++) {
      double num = fabs((double)m_data[i] - (double)newVector[i]);
      double den = (fabs((double)m_data[i]) + fabs((double)newVector[i])) / 2;
      if (den < tolerance && num > tolerance) {
        return false;
      } else if (num / den > tolerance) {
        return false;
      }
    }
    return true;
  }

These are not identical, though at first glance they might seem similar.
Try playing with it here.