Ticket #11179 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

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

ElasticWindowMultipleTest-mismatch.nxs (466.5 KB) - added by Dan Nixon 6 years ago.
II.AnalysisElWinMulti.nxs (441.1 KB) - added by Dan Nixon 6 years ago.

Change History

comment:1 Changed 6 years ago by Dan Nixon

  • Status changed from new to assigned

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:3 Changed 6 years ago by Dan Nixon

Unit test changes to NumericAxis

Refs #11179

Changeset: d2f9d86da0a6ca062b43aaf4572bafae644d26a5

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:5 Changed 6 years ago by Dan Nixon

Jenkins, retest this please.

Changed 6 years ago by Dan Nixon

Changed 6 years ago by Dan Nixon

comment:6 Changed 6 years ago by Dan Nixon

Jenkins, retest this please.

comment:7 Changed 6 years ago by Dan Nixon

Jenkins, retest this please.

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

Test inf and NaN use std::equal

Refs #11179

Changeset: 180b6e76d38de56206eea2b04b7b4005f6a7c971

comment:11 Changed 6 years ago by Dan Nixon

  1. Done.
  2. operator== can only take a single parameter as the other reference, I agree that it should probably use a tolerance through (1e-15?).
  3. Done
  4. 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

Correct NaN and infinity comparison

Refs #11179

Changeset: 3573429bddae777131b38a664ee09bfb9014f1d5

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

Note: See TracTickets for help on using tickets.