more changes and some extra comments

Framework/Algorithms/inc/MantidAlgorithms/CompareWorkspaces.h

@@ -76,8 +76,8 @@ class MANTID_ALGORITHMS_DLL CompareWorkspaces final : public API::Algorithm {
  "testing process.";
  }
 
- static bool withinAbsoluteTolerance(double x1, double x2, double atol);
- static bool withinRelativeTolerance(double x1, double x2, double rtol);
+ static bool withinAbsoluteTolerance(double x1, double x2, double atol, bool const nanEqual = false);
+ static bool withinRelativeTolerance(double x1, double x2, double rtol, bool const nanEqual = false);
 
 private:
  /// Initialise algorithm

Framework/Algorithms/src/CompareWorkspaces.cpp

@@ -22,6 +22,7 @@
 #include "MantidGeometry/Crystal/IPeak.h"
 #include "MantidGeometry/Instrument/ComponentInfo.h"
 #include "MantidGeometry/Instrument/DetectorInfo.h"
+#include "MantidKernel/FloatingPointComparison.h"
 #include "MantidKernel/Unit.h"
 
 namespace Mantid::Algorithms {
@@ -71,23 +72,18 @@ int compareEventLists(Kernel::Logger &logger, const EventList &el1, const EventL
  const auto &e1 = events1[i];
  const auto &e2 = events2[i];
 
- bool diffpulse = false;
- bool difftof = false;
- bool diffweight = false;
- if (std::abs(e1.pulseTime().totalNanoseconds() - e2.pulseTime().totalNanoseconds()) > tolPulse) {
- diffpulse = true;
- ++numdiffpulse;
- }
- if (std::abs(e1.tof() - e2.tof()) > tolTof) {
- difftof = true;
- ++numdifftof;
- }
+ bool diffpulse =
+ !withinAbsoluteDifference(e1.pulseTime().totalNanoseconds(), e2.pulseTime().totalNanoseconds(), tolPulse);
+ bool difftof = !withinAbsoluteDifference(e1.tof(), e2.tof(), tolTof);
+ bool diffweight = !withinAbsoluteDifference(e1.weight(), e2.weight(), tolWeight);
  if (diffpulse && difftof)
- ++numdiffboth;
- if (std::abs(e1.weight() - e2.weight()) > tolWeight) {
- diffweight = true;
- ++numdiffweight;
- }
+ numdiffboth++;
+ if (diffpulse)
+ numdiffpulse++;
+ if (difftof)
+ numdifftof++;
+ if (diffweight)
+ numdiffweight++;
 
  bool same = (!diffpulse) && (!difftof) && (!diffweight);
  if (!same) {
@@ -148,6 +144,8 @@ void CompareWorkspaces::init() {
  "Very often such logs are huge so making it true should be "
  "the last option.");
 
+ declareProperty("NaNsEqual", false, "Whether NaN values should compare as equal with other NaN values.");
+
  declareProperty("NumberMismatchedSpectraToPrint", 1, "Number of mismatched spectra from lowest to be listed. ");
 
  declareProperty("DetailedPrintIndex", EMPTY_INT(), "Mismatched spectra that will be printed out in details. ");
@@ -172,13 +170,14 @@ void CompareWorkspaces::exec() {
  m_parallelComparison = false;
 
  double const tolerance = getProperty("Tolerance");
+ bool const nanEqual = getProperty("NaNsEqual");
  if (getProperty("ToleranceRelErr")) {
- this->m_compare = [tolerance](double const x1, double const x2) -> bool {
- return CompareWorkspaces::withinRelativeTolerance(x1, x2, tolerance);
+ this->m_compare = [tolerance, nanEqual](double const x1, double const x2) -> bool {
+ return CompareWorkspaces::withinRelativeTolerance(x1, x2, tolerance, nanEqual);
  };
  } else {
- this->m_compare = [tolerance](double const x1, double const x2) -> bool {
- return CompareWorkspaces::withinAbsoluteTolerance(x1, x2, tolerance);
+ this->m_compare = [tolerance, nanEqual](double const x1, double const x2) -> bool {
+ return CompareWorkspaces::withinAbsoluteTolerance(x1, x2, tolerance, nanEqual);
  };
  }
 
@@ -1049,6 +1048,7 @@ void CompareWorkspaces::doPeaksComparison(PeaksWorkspace_sptr tws1, PeaksWorkspa
  }
 
  const bool isRelErr = getProperty("ToleranceRelErr");
+ const bool checkAllData = getProperty("CheckAllData");
  for (int i = 0; i < tws1->getNumberPeaks(); i++) {
  const Peak &peak1 = tws1->getPeak(i);
  const Peak &peak2 = tws2->getPeak(i);
@@ -1127,7 +1127,8 @@ void CompareWorkspaces::doPeaksComparison(PeaksWorkspace_sptr tws1, PeaksWorkspa
  << "value1 = " << s1 << "\n"
  << "value2 = " << s2 << "\n";
  recordMismatch("Data mismatch");
- return;
+ if (!checkAllData)
+ return;
  }
  }
  }
@@ -1163,6 +1164,8 @@ void CompareWorkspaces::doLeanElasticPeaksComparison(const LeanElasticPeaksWorks
 
  const double tolerance = getProperty("Tolerance");
  const bool isRelErr = getProperty("ToleranceRelErr");
+ const bool checkAllData = getProperty("CheckAllData");
+ const bool nanEqual = getProperty("NaNsEqual");
  for (int peakIndex = 0; peakIndex < ipws1->getNumberPeaks(); peakIndex++) {
  for (size_t j = 0; j < ipws1->columnCount(); j++) {
  std::shared_ptr<const API::Column> col = ipws1->getColumn(j);
@@ -1229,10 +1232,10 @@ void CompareWorkspaces::doLeanElasticPeaksComparison(const LeanElasticPeaksWorks
  // bool mismatch = !m_compare(s1, s2)
  // can replace this if/else, and isRelErr and tolerance can be deleted
  if (isRelErr && name != "QLab" && name != "QSample") {
- if (!withinRelativeTolerance(s1, s2, tolerance)) {
+ if (!withinRelativeTolerance(s1, s2, tolerance, nanEqual)) {
  mismatch = true;
  }
- } else if (!withinAbsoluteTolerance(s1, s2, tolerance)) {
+ } else if (!withinAbsoluteTolerance(s1, s2, tolerance, nanEqual)) {
  mismatch = true;
  }
  if (mismatch) {
@@ -1242,7 +1245,8 @@ void CompareWorkspaces::doLeanElasticPeaksComparison(const LeanElasticPeaksWorks
  << "value1 = " << s1 << "\n"
  << "value2 = " << s2 << "\n";
  recordMismatch("Data mismatch");
- return;
+ if (!checkAllData)
+ return;
  }
  }
  }
@@ -1356,12 +1360,15 @@ this error is within the limits requested.
 @param x1 -- first value to check difference
 @param x2 -- second value to check difference
 @param atol -- the tolerance of the comparison. Must be nonnegative
+@param nanEqual -- whether two NaNs compare as equal
 
 @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
- return !(std::abs(x1 - x2) > atol);
+bool CompareWorkspaces::withinAbsoluteTolerance(double const x1, double const x2, double const atol,
+ bool const nanEqual) {
+ if (nanEqual && std::isnan(x1) && std::isnan(x2))
+ return true;
+ return Kernel::withinAbsoluteDifference(x1, x2, atol);
 }
 
 //------------------------------------------------------------------------------------------------
@@ -1371,24 +1378,15 @@ this error is within the limits requested.
 @param x1 -- first value to check difference
 @param x2 -- second value to check difference
 @param rtol -- the tolerance of the comparison. Must be nonnegative
+@param nanEqual -- whether two NaNs compare as equal
 
 @returns true if relative difference is within the tolerance; false otherwise
 @returns true if error or false if the relative value is within the limits requested
 */
-bool CompareWorkspaces::withinRelativeTolerance(double const x1, double const x2, double const rtol) {
- // calculate difference
- double const num = std::abs(x1 - x2);
- // return early if the values are equal
- if (num == 0.0)
+bool CompareWorkspaces::withinRelativeTolerance(double const x1, double const x2, double const rtol,
+ bool const nanEqual) {
+ if (nanEqual && std::isnan(x1) && std::isnan(x2))
  return true;
- // create the average magnitude for comparison
- double const den = 0.5 * (std::abs(x1) + std::abs(x2));
- // return early, possibly avoids a multiplication
- // NOTE if den<1, then divsion will only make num larger
- // NOTE if den<1 but num<=rtol, we cannot conclude anything
- if (den <= 1.0 && num > rtol)
- return false;
- // NOTE !(num > rtol*den) is not the same as (num <= rtol*den)
- return !(num > (rtol * den));
+ return Kernel::withinRelativeDifference(x1, x2, rtol);
 }
 } // namespace Mantid::Algorithms

Framework/Algorithms/src/DetectorEfficiencyCor.cpp

@@ -123,7 +123,7 @@ void DetectorEfficiencyCor::exec() {
  int64_t numHists = m_inputWS->getNumberHistograms();
  auto numHists_d = static_cast<double>(numHists);
  const auto progStep = static_cast<int64_t>(ceil(numHists_d / 100.0));
- auto &spectrumInfo = m_inputWS->spectrumInfo();
+ auto const &spectrumInfo = m_inputWS->spectrumInfo();
 
  PARALLEL_FOR_IF(Kernel::threadSafe(*m_inputWS, *m_outputWS))
  for (int64_t i = 0; i < numHists; ++i) {

Framework/Algorithms/test/CompareWorkspacesTest.h

@@ -1193,6 +1193,60 @@ class CompareWorkspacesTest : public CxxTest::TestSuite {
  TS_ASSERT_EQUALS(alg.getPropertyValue("Result"), PROPERTY_VALUE_TRUE);
  }
 
+ void test_equal_tableworkspaces_match() {
+ std::string const col_type("double"), col_name("aColumn");
+ std::vector<double> col_values{1.0, 2.0, 3.0};
+ // create the table workspaces
+ Mantid::API::ITableWorkspace_sptr table1 = WorkspaceFactory::Instance().createTable();
+ table1->addColumn(col_type, col_name);
+ for (double val : col_values) {
+ TableRow newrow = table1->appendRow();
+ newrow << val;
+ }
+ Mantid::API::ITableWorkspace_sptr table2 = WorkspaceFactory::Instance().createTable();
+ table2->addColumn(col_type, col_name);
+ for (double val : col_values) {
+ TableRow newrow = table2->appendRow();
+ newrow << val;
+ }
+
+ Mantid::Algorithms::CompareWorkspaces alg;
+ alg.initialize();
+ TS_ASSERT_THROWS_NOTHING(alg.setProperty("Workspace1", table1));
+ TS_ASSERT_THROWS_NOTHING(alg.setProperty("Workspace2", table2));
+ TS_ASSERT(alg.execute());
+ TS_ASSERT_EQUALS(alg.getPropertyValue("Result"), PROPERTY_VALUE_TRUE);
+ }
+
+ void test_tableworkspace_NaNs_fails() {
+ std::string const col_type("double"), col_name("aColumn");
+ std::vector<double> col_values1{1.0, 2.0, 3.0};
+ std::vector<double> col_values2{1.0, 2.0, std::numeric_limits<double>::quiet_NaN()};
+ // create the table workspaces
+ Mantid::API::ITableWorkspace_sptr table1 = WorkspaceFactory::Instance().createTable();
+ table1->addColumn(col_type, col_name);
+ for (double val : col_values1) {
+ TableRow newrow = table1->appendRow();
+ newrow << val;
+ }
+ Mantid::API::ITableWorkspace_sptr table2 = WorkspaceFactory::Instance().createTable();
+ table2->addColumn(col_type, col_name);
+ for (double val : col_values2) {
+ TableRow newrow = table2->appendRow();
+ newrow << val;
+ }
+
+ Mantid::Algorithms::CompareWorkspaces alg;
+ alg.initialize();
+ TS_ASSERT_THROWS_NOTHING(alg.setProperty("Workspace1", table1));
+ TS_ASSERT_THROWS_NOTHING(alg.setProperty("Workspace2", table2));
+ TS_ASSERT(alg.execute());
+ TS_ASSERT_EQUALS(alg.getPropertyValue("Result"), PROPERTY_VALUE_FALSE);
+
+ ITableWorkspace_sptr table = AnalysisDataService::Instance().retrieveWS<TableWorkspace>("compare_msgs");
+ TS_ASSERT_EQUALS(table->cell<std::string>(0, 0), "Table data mismatch");
+ }
+
  void test_tableworkspace_different_column_names_fails() {
  auto table1 = setupTableWorkspace();
  table1->getColumn(5)->setName("SomethingElse");

Framework/Algorithms/test/Q1DWeightedTest.h

@@ -29,6 +29,11 @@ using Mantid::DataHandling::LoadNexusProcessed;
 using Mantid::DataHandling::LoadSpice2D;
 using Mantid::DataHandling::MoveInstrumentComponent;
 
+namespace {
+bool const USE_NANS_EQUAL(true);
+bool const USE_NANS_NOT_EQUAL(false);
+} // namespace
+
 class Q1DWeightedTest : public CxxTest::TestSuite {
 public:
  void testName() { TS_ASSERT_EQUALS(radial_average.name(), "Q1DWeighted") }
@@ -277,6 +282,11 @@ class Q1DWeightedTest : public CxxTest::TestSuite {
  compareWorkspaces(refWedges, outputWedges);
  }
 
+ /**
+ * The result and the expected value used in this test are two matrix
+ * workspaces with NaNs in all y-values and in all e-values.
+ * Is this even a meaningful test?
+ */
  void testShapeTableResultsAsymm() {
  // test the results computed by the table shape method against those from
  // the usual method when asymmetricWedges is set to true
@@ -295,7 +305,7 @@ class Q1DWeightedTest : public CxxTest::TestSuite {
  populateAlgorithm(refWS, refWedges, false, true, 4);
  TS_ASSERT_THROWS_NOTHING(radial_average.execute())
 
- compareWorkspaces(refWedges, outputWedges);
+ compareWorkspaces(refWedges, outputWedges, USE_NANS_EQUAL);
  }
 
  void testShapeCorrectOrder() {
@@ -509,7 +519,7 @@ class Q1DWeightedTest : public CxxTest::TestSuite {
  }
  }
 
- void compareWorkspaces(std::string refWS, std::string toCompare) {
+ void compareWorkspaces(std::string refWS, std::string toCompare, bool nansEqual = USE_NANS_NOT_EQUAL) {
  WorkspaceGroup_sptr result;
  TS_ASSERT_THROWS_NOTHING(
  result = std::dynamic_pointer_cast<WorkspaceGroup>(AnalysisDataService::Instance().retrieve(toCompare)))
@@ -529,6 +539,7 @@ class Q1DWeightedTest : public CxxTest::TestSuite {
  comparison.setPropertyValue("CheckAllData", "1");
  comparison.setPropertyValue("CheckType", "1");
  comparison.setPropertyValue("ToleranceRelErr", "1");
+ comparison.setProperty("NaNsEqual", nansEqual);
  TS_ASSERT(comparison.execute())
  TS_ASSERT(comparison.isExecuted())
  TS_ASSERT_EQUALS(comparison.getPropertyValue("Result"), "1");

Framework/DataObjects/inc/MantidDataObjects/TableColumn.h

@@ -14,6 +14,7 @@
 #include <vector>
 
 #include "MantidAPI/Column.h"
+#include "MantidKernel/FloatingPointComparison.h"
 #include "MantidKernel/V3D.h"
 
 namespace Mantid {
@@ -64,10 +65,10 @@ template <class Type> class TableColumn : public API::Column {
  std::string name = std::string(typeid(Type).name());
  if ((name.find('i') != std::string::npos) || (name.find('l') != std::string::npos) ||
  (name.find('x') != std::string::npos)) {
- if (length == 4) {
+ if (length == 4) { // cppcheck-suppress knownConditionTrueFalse
  this->m_type = "int";
  }
- if (length == 8) {
+ if (length == 8) { // cppcheck-suppress knownConditionTrueFalse
  this->m_type = "int64";
  }
  }
@@ -78,10 +79,10 @@ template <class Type> class TableColumn : public API::Column {
  this->m_type = "double";
  }
  if (name.find('u') != std::string::npos) {
- if (length == 4) {
+ if (length == 4) { // cppcheck-suppress knownConditionTrueFalse
  this->m_type = "uint32_t";
  }
- if (length == 8) {
+ if (length == 8) { // cppcheck-suppress knownConditionTrueFalse
  this->m_type = "uint64_t";
  }
  }
@@ -221,7 +222,7 @@ template <class Type> class TableColumn : public API::Column {
  // helper function template for equality
  bool compareVectors(const std::vector<Type> &newVector, double tolerance) const {
  for (size_t i = 0; i < m_data.size(); i++) {
- if (fabs((double)m_data[i] - (double)newVector[i]) > tolerance) {
+ if (!Kernel::withinAbsoluteDifference<Type, double>(m_data[i], newVector[i], tolerance)) {
  return false;
  }
  }
@@ -231,17 +232,14 @@ template <class Type> class TableColumn : public API::Column {
  // helper function template for equality with relative error
  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) {
+ if (!Kernel::withinRelativeDifference<Type, double>(m_data[i], newVector[i], tolerance)) {
  return false;
  }
  }
  return true;
  }
 };
+
 /// Template specialisation for long64
 template <>
 inline bool TableColumn<int64_t>::compareVectors(const std::vector<int64_t> &newVector, double tolerance) const {

Framework/Kernel/inc/MantidKernel/FloatingPointComparison.h

@@ -10,6 +10,7 @@
 // Includes
 //-----------------------------------------------------------------------------
 #include "MantidKernel/DllConfig.h"
+#include "MantidKernel/V3D.h"
 
 namespace Mantid {
 namespace Kernel {
@@ -24,8 +25,10 @@ template <typename T> MANTID_KERNEL_DLL T absoluteDifference(T const x, T const
 /// Calculate relative difference between x, y
 template <typename T> MANTID_KERNEL_DLL T relativeDifference(T const x, T const y);
 /// Test whether x, y are within absolute tolerance tol
-template <typename T> MANTID_KERNEL_DLL bool withinAbsoluteDifference(T const x, T const y, T const tolerance);
+template <typename T, typename S = T>
+MANTID_KERNEL_DLL bool withinAbsoluteDifference(T const x, T const y, S const tolerance);
 /// Test whether x, y are within relative tolerance tol
-template <typename T> MANTID_KERNEL_DLL bool withinRelativeDifference(T const x, T const y, T const tolerance);
+template <typename T, typename S = T>
+MANTID_KERNEL_DLL bool withinRelativeDifference(T const x, T const y, S const tolerance);
 } // namespace Kernel
 } // namespace Mantid