CheckWorkspacesMatch should use tolerance property on NumericAxis comparison #309

DanNixon · 2015-02-26T10:04:02Z

Fixes #11179.

To test:

Review code changes and unit test
Load the files attached to the Trac ticket
Run CheckWorkspacesMatch on both pairs of workspaces, ensuring CheckAxis is enabled, CheckSpectraMap is disabled and tolerance is set to 1e-10
On master this fails, with this fix it should pass

All being well this should fix the failing system test on OSX 10.9.

and use in CheckWorkspacesMatch Refs #11179

Refs #11179

DanNixon · 2015-02-26T10:17:55Z

Jenkins, retest this please.

DanNixon · 2015-02-26T15:34:45Z

Jenkins, retest this please.

DanNixon · 2015-02-26T20:07:01Z

Jenkins, retest this please.

quantumsteve · 2015-02-26T23:45:20Z

Would you please add unit tests for the special case when one or more of the values is Inf or NaN?
I think this should replace the current behavior of virtual bool operator==(const Axis &) const because we should never use operator== to compare floating point values. Can we create an optional argument for the tolerance?
Consider replacing your for loop with std::equal and a binary predicate.
The system test is still failing on os x because m_values.size() is not equal.

Refs #11179

DanNixon · 2015-02-27T09:49:22Z

Done.
operator== can only take a single parameter as the other reference, I agree that it should probably use a tolerance through (1e-15?).
Done
This ticket was just to fix the issue with CheckWorkspacesMatch, I'm still looking at what's wrong with the OSX.

Refs #11179

quantumsteve · 2015-02-27T14:38:32Z

There is an error on Windows.

Would you replace your global variable and function with a functor?
Agreed, that shouldn't stop it from being merged into master.

Refs #11179

quantumsteve · 2015-02-27T16:18:31Z

Code/Mantid/Framework/Algorithms/src/CheckWorkspacesMatch.cpp

+        result = axis_name + " values mismatch";
+        return false;
+      }
+    } else if (!ax1->isSpectra() && !ax1->operator==(*ax2)) {


I think we are better off using the overloaded operator== instead of this first if statement. RefAxis and NumericAxis both return true for isNumeric(), but have different implementations of operator==.

The ISISIndirectAnalysis system test passes on os x 10.9 if we remove the first if.

DanNixon · 2015-02-27T16:25:04Z

Not checking with the tolerance provided would pretty much defeat the point of this ticket, I cant see why removing the first if would make any difference as it would be doing the same comparison as it always has.

quantumsteve · 2015-02-27T16:52:13Z

retest this please

quantumsteve · 2015-02-27T17:03:01Z

Yes, using the provided tolerance would be better.

Running ISISIndirectAnalysisTest on master, CheckWorkspacesMatch is false because of the equality comparison between NumericAxis. In this branch, it fails because of the equality comparison between RefAxis.

quantumsteve · 2015-02-27T18:59:00Z

The win7 build has the following error. I'm going to try retesting.

18:14:29 ISISLiveEventDataListener-[Error] Caught a runtime exception.
18:14:29 ISISLiveEventDataListener-[Error] Exception message: Operation of receiving Events header timed out.

quantumsteve · 2015-02-27T18:59:07Z

retest this please

abuts · 2015-02-28T13:30:19Z

Jenkins, Retest this please

Refs #11179

DanNixon · 2015-03-02T11:37:20Z

Jenkins, retest this please.

quantumsteve · 2015-03-02T14:32:58Z

retest this please

Refs #11179

quantumsteve · 2015-03-02T15:05:40Z

Code/Mantid/Framework/API/inc/MantidAPI/NumericAxis.h

@@ -65,6 +65,8 @@ class MANTID_API_DLL NumericAxis : public Axis {
  virtual void setValue(const std::size_t &index, const double &value);
  size_t indexOfValue(const double value) const;
  virtual bool operator==(const Axis &) const;
+  virtual bool equalWithinTolerance(const Axis &axis2,
+                                    const double tolerance = 0.0) const;


May we settle on a single default tolerance or remove the default value? 1e-15 is being used for operator==.

Refs #11179

quantumsteve · 2015-03-02T20:29:27Z

retest this please

quantumsteve · 2015-03-02T21:51:53Z

I tested this last week and found it fixed the issues comparing those two workspaces listed above. The new changes made today look good and it's passing all tests on jenkins. Let's

…_use_tolerance_on_numericaxis CheckWorkspacesMatch should use tolerance property on NumericAxis comparison

DanNixon added 2 commits February 25, 2015 16:51

Implement equal to tolerance function in NumericAxis

9016870

and use in CheckWorkspacesMatch Refs #11179

Unit test changes to NumericAxis

d2f9d86

Refs #11179

DanNixon added High Priority An issue or pull request that if not addressed is severe enough to postponse a release. In Progress Framework Issues and pull requests related to components in the Framework labels Feb 26, 2015

DanNixon added this to the Release 3.4 milestone Feb 26, 2015

DanNixon removed the In Progress label Feb 26, 2015

quantumsteve self-assigned this Feb 26, 2015

Test inf and NaN use std::equal

180b6e7

Refs #11179

operator== should use tolerance by default

916e91c

Refs #11179

DanNixon added 2 commits February 27, 2015 14:57

Use functor for equality with tolerance check

65a6bac

Refs #11179

Correct NaN and infinity comparison

3573429

Refs #11179

quantumsteve reviewed Feb 27, 2015
View reviewed changes

DanNixon added the In Progress label Feb 28, 2015

Fix RefAxis comparison in CheckWorkspacesMatch

0ab23dd

Refs #11179

DanNixon removed the In Progress label Mar 2, 2015

Decalre equalWithinTolerance virtual

4486d7c

Refs #11179

quantumsteve reviewed Mar 2, 2015
View reviewed changes

Remove default tolerance in equalWithinTolerance

5bbd613

Refs #11179

quantumsteve added a commit that referenced this pull request Mar 2, 2015

Merge pull request #309 from mantidproject/11179_checkworkspacesmatch…

128ea4a

…_use_tolerance_on_numericaxis CheckWorkspacesMatch should use tolerance property on NumericAxis comparison

quantumsteve merged commit 128ea4a into master Mar 2, 2015

quantumsteve deleted the 11179_checkworkspacesmatch_use_tolerance_on_numericaxis branch March 2, 2015 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CheckWorkspacesMatch should use tolerance property on NumericAxis comparison #309

CheckWorkspacesMatch should use tolerance property on NumericAxis comparison #309

DanNixon commented Feb 26, 2015

DanNixon commented Feb 26, 2015

DanNixon commented Feb 26, 2015

DanNixon commented Feb 26, 2015

quantumsteve commented Feb 26, 2015

DanNixon commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

quantumsteve Feb 27, 2015

DanNixon commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

abuts commented Feb 28, 2015

DanNixon commented Mar 2, 2015

quantumsteve commented Mar 2, 2015

quantumsteve Mar 2, 2015

quantumsteve commented Mar 2, 2015

quantumsteve commented Mar 2, 2015

CheckWorkspacesMatch should use tolerance property on NumericAxis comparison #309

CheckWorkspacesMatch should use tolerance property on NumericAxis comparison #309

Conversation

DanNixon commented Feb 26, 2015

DanNixon commented Feb 26, 2015

DanNixon commented Feb 26, 2015

DanNixon commented Feb 26, 2015

quantumsteve commented Feb 26, 2015

DanNixon commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

quantumsteve Feb 27, 2015

Choose a reason for hiding this comment

DanNixon commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

quantumsteve commented Feb 27, 2015

abuts commented Feb 28, 2015

DanNixon commented Mar 2, 2015

quantumsteve commented Mar 2, 2015

quantumsteve Mar 2, 2015

Choose a reason for hiding this comment

quantumsteve commented Mar 2, 2015

quantumsteve commented Mar 2, 2015