Run CheckWorkspaceMatch serially when investigating comparison logs

[Alex Buts]

Changes to checkWorkspaceMatch allowing to run it in parallel certainly accelerate things and fine when things are correct but are absolutely unhelpful if one wants to look why and were things went wrong. logs are produced in parallel and it is impossible to understand what they are reporting.

This is why it is worth to run comparison serially (or build serial logs which is much more difficult task) if one is interested in looking at comparison logs (on debug level)

[Alex Buts]

refs #9022 Should solve the problem

by prohibiting parallel execution for CheckWorkspacesMatch in one selects "debug" and "trace" log levels

Changeset: ac40ab632f7a3f12da8161f265b9bf5c78e234f6

[Alex Buts]

refs #9022 Incorrect comparison fixed

Changeset: 21e067b8216ba6ec172894c7ace2441e1413e765

[Alex Buts]

refs #9022 Histogram data comparison also serialized when requested

Changeset: 6e07846b15f4166beb6f495e94f6587b5bab6dc1

[Alex Buts]

refs #9022 Unit test for log levels

Changeset: 451a7a29e119f8a3797845b21deab690683b8193

[Alex Buts]

refs #9022 Should solve the problem

by prohibiting parallel execution for CheckWorkspacesMatch in one selects "debug" and "trace" log levels

Changeset: 2964bea150fccae89214bc9dbdf999b68898aca0

[Alex Buts]

refs #9022 Incorrect comparison fixed

Changeset: 1e3a3f6b00f7cccaced054178dbb692ce3414155

[Alex Buts]

refs #9022 Histogram data comparison also serialized when requested

Changeset: 7996832f28bd258a27c6ceabbeb0bb8fa81a8ac8

[Alex Buts]

refs #9022 Unit test for log levels

Changeset: f68a043808334cb3ae33ef415a32e78393276ca0

[Alex Buts]

refs #9022 fixing unix build

by removing references to POCO methods which do not exist on Unix

Changeset: 4746ee7705f867548a0effe339359810c697b0c4

[Alex Buts]

To test:

find 2 histogram workspaces which are different on data level* and run CheckWorkspaceMatch on multiprocessor machine before and after applying this patch.

Choose Debug level on results window to see the reason for the data mismatch.

Before the patch CheckWorkspaceMatch generates some gibberish composed by output from different threads. After the patch you will be able to see single (first) mismatched spectra with clear view for what the differences are.

*(easy way to do get such workspaces is to take ISISDirectInelasti system test e.g. MARIReductionFromWorkspace -- it is very fast, set self.sample_mass = 11 (row 176)) and run the test -- it will generate MARIReductionFromFile-mismatch.nxs which is slightly different from the sample MARIReduction.nxs)

[Alex Buts]

this ticket has been created to help with another ticket where it has been initially resided and then rebased to master. As the result track is in mess. Look in Git for code changes -- they are small.

Should be easy to understand and test the changes. Testing guidance -- in the message above.

[Alex Buts]

refs #9022 Reverting CheckWorkspacesMatch to division under fabs

Somebody did a good job making this algorithm look nice but useless, so I spent long time looking in amazement why workspaces which look identical do not match. A coordinate (and sometimes signal and error! can be negative!) This fixes this problem.

Changeset: 7012b8a76eb0dc350247bf6b0749d290cbd7f49f

[Alex Buts]

refs #9022 No, this actually fixes negative values problem

Changeset: cbb4cd6f4f768ab22bc0b51b626474f6b2d13cea

[Alex Buts]

refs #9022 Proper error estimation

Evaluating problem I am having comparing two similar inelastic workspaces I've modified comparison algorithm to work with relative and absolute error. This is actually what you want comparing two workspaces and some kind of this approach was already in place for values close to 0

Changeset: 02176858c3c201cfe94e99e15e6f3150022acef3

[Alex Buts]

refs #9022 after some thinking, I've decided that standard definition

of Relerr with more careful treatment of values near 0 is preferable implementation.

Changeset: 0d47d5b2525525e208d68e17f316f8464342d610

[Alex Buts]

refs #9022 A simple comment

Changeset: 0fdfa950b9874ba1350becebcdc190e9b69d367a

[Alex Buts]

Trying to compare two complex workspaces I tried different ways of comparing the data and calculating the error. Final result which suits the purpose is the standard relative error with careful treatment of small values. As the change is minor and related to CheckWorkspaceMatch algorithm it well fits within this ticket.

[Nick Draper]

The branch

[Nick Draper]

This branch has not Fully been merged to develop, please run a checkbuild and send for testing again.

This wasted 15 minutes of my life, as I foolishly started testing the current state before running git test start.

[Martyn Gigg]

The changes generally look fine but the new function, TOL_ERR, is named incorrectly and is not in the right place. Can it be moved to the first thing inside Algorithms namespace and inside an anonymous namespace. Also please remove the inline keyword and use the proper camelCase standard for the names. I think the function name could be improved slightly while we're here too:

• TOL_ERR -> relErrInLimit
• ERROR_VAL -> errVal

namespace Algorithms
{
  namespace
  {
    /**
     * Function which calculates relative error between two values and analyses if this error is within the limits
     * requested
     * @param x1 -- first value to check difference
     * @param x2 -- second value to check difference
     * @param errVal -- the value of the error, to check against. Should  be positive != 0
     * @returns true if error or false if the value is within the limits requested
     */
     bool relErrInLimit(const double &x1, const double &x2, const double &errVal)
     {
 
       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<errVal) 
         return (num>errVal);
 
       return (num/den>errVal);
     }
   }

[Alex Buts]

refs #9022 Trivial changes to the function arguments

Changeset: 35ae58734a5b92458347e872361c49d4cc89f8d4

[Alex Buts]

The TOL_ERR capital came from "notional" templating of this function into the code where it was initially cut-out. I do not have problem with naming it low case, but making it global does not make sense. It is not good enough to provide homogeneous accuracy along any data range and its meaning is well restricted by the algorithm it is in. It can be properly understood only within this algorithm scope.

I tried to make it generic, but doing this appears to need much more efforts then I was able to assign within the scope of another ticket this one is extracted from.

If we wand generic and proper tollErr function, it should be separate ticket for it. Meanwhile this one has been renamed to relErr.

[Alex Buts]

refs #9022 conflicts with master after long inactivity period

Merge branch 'master' into feature/9022_serialWorkspaceComparison

Conflicts:

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

Changeset: 891c24d9a1c975329309419621c5bd0ad4710788

[Alex Buts]

refs #9022 Missing pre-commit

why it get missing in the first place?

Changeset: 096696236d85f3ab1f0435b87db68c964a3038e5

[Martyn Gigg]

I didn't mean to make the function global. What I meant was just move it to near the top of the CheckWorkspacesMatch.cpp file and put it in an anonymous namespace (this will actually make it far from global). The only reason I think it should be moved is that at the moment it's place in the file makes it look like it is a member function.

My idea was for the top of the file to look like this:

namespace Mantid
{
namespace Algorithms
{
  namespace
  { //anonymous
    /**
     * Function which calculates relative error between two values and analyses if this error is within the limits
    * requested
    * @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 positive != 0

    * @returns true if error or false if the value is within the limits requested
    */
   bool relErr(const double &x1,const double &x2,const double &errorVal)
   {
     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);
   }
  }
... rest of the file

[Alex Buts]

refs #9022 moved error calculating function into anonymous namespace.

nof if I am familiar with this style or finding it clearer then usual file scope function.

Changeset: a4a2820677b34cde570d1aa28e6c428275345749

[Alex Buts]

refs #9022 better way of expressing the same concept

Changeset: 757b060971d0981e0934c4867f0f5648b8213274

[Martyn Gigg]

Merge remote-tracking branch 'origin/feature/9022_serialWorkspaceComparison'

Full changeset: ​b0d8ba3930908612f42ddbb8a4ee7a433c4e7036

[Stuart Campbell]

This ticket has been transferred to github issue ​9865