Ticket #11179 (closed: fixed)
CheckWorkspacesMatch ignores tolerance when checking numeric axis
Reported by: | Dan Nixon | Owned by: | Dan Nixon |
---|---|---|---|
Priority: | critical | Milestone: | Release 3.4 |
Component: | Framework | Keywords: | |
Cc: | Blocked By: | ||
Blocking: | Tester: | Steven Hahn |
Description
And is causing the systemtests to fail on OS X 10.9.
Attachments
Change History
comment:2 Changed 6 years ago by Dan Nixon
- Status changed from assigned to inprogress
Implement equal to tolerance function in NumericAxis
and use in CheckWorkspacesMatch
Refs #11179
Changeset: 9016870764518b6e472e5145e3a0c772124cdbd2
comment:4 Changed 6 years ago by Dan Nixon
- Status changed from inprogress to verify
- Resolution set to fixed
This is being verified as pull request #309.
comment:8 Changed 6 years ago by Steven Hahn
- Status changed from verify to verifying
- Tester set to Steven Hahn
comment:9 Changed 6 years ago by Steven Hahn
1) Would you please add unit tests for the special case when one or more of the values is Inf or NaN?
2) 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?
3) Consider replacing your for loop with std::equal and a binary predicate.
4) The system test is still failing on os x because m_values.size() is not equal. ![screen shot 2015-02-26 at 6 08 25 pm](https://cloud.githubusercontent.com/assets/5920552/6404392/7805542c-bde5-11e4-8c12-cc85dfdbbe3a.png)
comment:10 Changed 6 years ago by Dan Nixon
comment:11 Changed 6 years ago by Dan Nixon
- 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.
comment:12 Changed 6 years ago by Dan Nixon
operator== should use tolerance by default
Refs #11179
Changeset: 916e91c6819857b79ed85e46225a292e36517b73
comment:13 Changed 6 years ago by Steven Hahn
There is an error on Windows.
3) Would you replace your global variable and function with a functor? 4) Agreed, that shouldn't stop it from being merged into master.
comment:14 Changed 6 years ago by Dan Nixon
Use functor for equality with tolerance check
Refs #11179
Changeset: 65a6bace12b0e9d9dd98cb01e29ca9252f7d3047
comment:15 Changed 6 years ago by Dan Nixon
comment:16 Changed 6 years ago by Dan Nixon
Not checking with the tolerance provided would pretty much defeat the point of this ticket, and removing the first if would make the test pass as the values would never be checked.
comment:17 Changed 6 years ago by Steven Hahn
retest this please
comment:18 Changed 6 years ago by Steven Hahn
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.
comment:19 Changed 6 years ago by Steven Hahn
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. `
comment:20 Changed 6 years ago by Steven Hahn
retest this please
comment:21 Changed 6 years ago by Dan Nixon
Fix RefAxis comparison in CheckWorkspacesMatch
Refs #11179
Changeset: 0ab23ddbfee6b612ba715703ed5f3d6205534cde
comment:22 Changed 6 years ago by Dan Nixon
Jenkins, retest this please.
comment:23 Changed 6 years ago by Steven Hahn
retest this please
comment:24 Changed 6 years ago by Dan Nixon
Decalre equalWithinTolerance virtual
Refs #11179
Changeset: 4486d7ca8a0bd00ab5998ae4419954ecf28050c9
comment:25 Changed 6 years ago by Dan Nixon
Remove default tolerance in equalWithinTolerance
Refs #11179
Changeset: 5bbd613b479016329c93075ed6eb1765309d1a56
comment:26 Changed 6 years ago by Steven Hahn
retest this please
comment:27 Changed 6 years ago by Steven Hahn
I tested this last week and found it fixed the issues comparing those two workspaces. The new changes made today look good and it's passing all tests on jenkins. Let's :shipit:
comment:28 Changed 6 years ago by Steven Hahn
- Status changed from verifying to closed
Merge pull request #309 from mantidproject/11179_checkworkspacesmatch_use_tolerance_on_numericaxis
CheckWorkspacesMatch should use tolerance property on NumericAxis comparison
Full changeset: 128ea4abccdb6ea0fc3c722c5924a823737f2c78
comment:29 Changed 5 years ago by Nick Draper
Somehow these slipped through without a resolution. Set to Fixed.
comment:30 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 12018