Restore changes which apply detector corrections at loading

[Alex Buts]

This is reasonable change which works for SNS and should be deployed by ISIS

[Alex Buts]

refs #8640 restored the changes Revert "Revert "Merge branch 'feature/8057_remove_loaddetectorinfo_dec'""

This reverts commit 7af79c1ffb775a6593dc479f6376d986b62339c9.

Changeset: f385f061347c48ba1bd37c471a353ad3d7e051bc

[Alex Buts]

refs #8640 Minor improvement to ScaleX algorithm description.

Changeset: 7c64386583aa3f05f98733ee0d093a8cf603d70f

[Alex Buts]

The System tests will fail until #8578 is applied, so I am merging it to the ticket.

[Alex Buts]

refs #8640 Change Divide to allow not to report about division by 0

Changeset: dec1efd87b3c211a16dbe9b89d79172ad52ed30f

[Alex Buts]

refs #8640 Deployed full Divide algorithm with quiet option

to avoid long diagnostics messages about division by 0 at diag phase (especially important for MAPS as 0 detectors are just missed there)

Changeset: 5f07e346bfc9da8366aad10cab9e712de6aa619d

[Alex Buts]

refs #8640 minor change to workflow as follows:

1) setting hard_mask_only to true and hard_mask_file to None disables diagnostics (it was found that this combination causes problems to diagnostics)

2) run_diagnostics key has been moved to to workflow section of the instrument parameters (it was sitting near hard_mask_file before)

Changeset: 582b6a10fb947df846f49643230038f7a8743cbe

[Alex Buts]

refs #8640 The commit brings back the changes to system tests

necessary for these tests to pass. All changes have been discussed with correspondent instrument scientists.

Changeset: 3d3a54e8af26dc32858d47e7ed7c4c6821c9ca76

[Alex Buts]

refs #8640 A comment

Changeset: d08a65b290c6aefbcf7c4b5a6a597293ee8bb3f2

[Alex Buts]

refs #8640 resolving conflicts with develop while Merge

Merge branch 'feature/8640_restore_loaddetectorinfo_removal' into develop

Conflicts:

Code/Mantid/scripts/Inelastic/CommonFunctions.py Code/Mantid/scripts/Inelastic/DirectEnergyConversion.py

Changeset: 9711b59b4132e1873d3ac2d4b5022aa72da8d2f1

[Alex Buts]

This ticket can be tested only after #8578 is merged to master.

The work itself is not trivial and can probably be easy verified by Martin only, but a tester should not worry about this as the ticket is just brings back well tested and agreed changes done earlier. I have renegotiated the changes with every instrument scientist concerned.

I've also added two trivial changes, related to further work in this direction: 'BeQuet' option to divide algorithm, forcing this algorithm to report division by 0 on debug level only (warnings about division by 0 was real pest on MAPS where these divisions are legitimate)

fixed small bug noticed when testing the changes again, namely failure when "use hard mask only" and hard_mask_file=None are chosen.

These changes are trivial and can be easy understood after code review.

The ticket changes both Mantid and SystemSests repository. Both have to be merged to correspondent masters.

[Russell Taylor]

I'm not convinced about the addition of the corner-case property to Divide. Couldn't this be handled externally by checking whether the workspace is indeed zero and then just not calling Divide?

But if it's going to be there then for goodness sake spell it right. Actually the name is too general anyway - how would I know by looking at it what it meant? "WarnOnZeroDivide" would be much better.

[Alex Buts]

refs #8640 cosmetic changes to facilitate internal merging

Changeset: d9fe7c8bbc0f81076a93ef15856000d431b18a3c

[Alex Buts]

refs #8640 Replaced BeQuet property by WarnOnZeroDifide

Changeset: ee195548eb39d48da482aec28db66a426119455c

[Alex Buts]

refs #8640 actually it should be "notWarnOnZeroDifide"

as in most cases warning should be default option.

Changeset: 568b0c0e14b83da51e863ce7406060804e6f9711

[Alex Buts]

"Couldn't this be handled externally by checking whether the workspace is indeed zero and then just not calling Divide"

Usually division by 0 should be reported, but not in this special case.

For MAPS you have 30% of detectors which are not connected to anything so it has 0 counting rate for WB vanadium. This generates around 0.3*40K messages which goes forever, always there and absolutely useless as the result of division is filtered at the next step. Merlin recently got similar problem, though for different reason.

Workspace is not 0, only some spectra on this workspace are 0.

Please note, that this ticket should be tested only after #8578

[Martyn Gigg]

I agree with Russell and I still think the property name is wrong. It should simply be WarnOnDivideByZero with default set to true. This then reproduces the previous behaviour and as an option makes much more sense when reading its name.

[Alex Buts]

refs #8640 Changed NotWarnOnZeroDivide to WarnOnZeroDivide

Not sure it is better option but certainly not worth discussing for more then introducing this change.

Changeset: 0e7444fbe981a06af22093e7028a7f0eea31bfa0

[Alex Buts]

refs #8640 And the comment

Changeset: e3705dd09ff8987158fd0ca7ed1dd966d7909a5c

[Russell Taylor]

I agree that Martyn's suggested name - "WarnOnDivideByZero" - is better.

[Russell Taylor]

Some aspect of the changes in this ticket has had a noticeable impact on performance tests at both the unit and system level. It's hard to tell what given the number of changes and the fact that they were (I think) all merged to develop at the same time.

The effect is best seen by looking at ​https://builds.sns.gov/view/Wall%20display/job/ornl_test_rhel6_develop/System_tests_performance/overview_plot.htm - see the step around number 690 in a number of the tests.

[Alex Buts]

puzzling. I would not be surprised by changes to ISIS direct -- 2 different algorithms are deployed and one disabled at different stages of reduction workflow. As the changes fix bug with event mode, 5% performance decrease is certainly accepted and compensated by 5% gain for different reduction algorithms.

But how it can affect Sans2D or HFIR is beyond my knowledge. Do they use the same python common functions as ISIS direct? Did they use scaleX before and affected by the changes?

What would Martyn say?

[Alex Buts]

refs #8640 resolving conflicts with develop while Merge

Merge branch 'feature/8640_restore_loaddetectorinfo_removal' into develop

Conflicts:

Code/Mantid/scripts/Inelastic/CommonFunctions.py Code/Mantid/scripts/Inelastic/DirectEnergyConversion.py

Changeset: 9711b59b4132e1873d3ac2d4b5022aa72da8d2f1

[Martyn Gigg]

Merge remote-tracking branch 'origin/master' into feature/8640_restore_loaddetectorinfo_removal

Conflicts:

Code/Mantid/Framework/Geometry/inc/MantidGeometry/Instrument/ParameterMap.h Code/Mantid/Framework/Geometry/src/Instrument/ParameterMap.cpp

Refs #8640

Changeset: 599bb85be73960b2bbd4812cbfcf6ebb358be062

[Martyn Gigg]

Merge branch 'feature/8640_restore_loaddetectorinfo_removal' into develop

Conflicts:

Code/Mantid/Framework/Geometry/src/Instrument/ParameterMap.cpp

Refs #8640

Changeset: df2661a71fa54a46b5d7eae0170a0bbe5d89087a

[Martyn Gigg]

Fix mistake in merge conflict resolution.

Refs #8640

Changeset: 8264c2d0966ecdd24e73f92788bb37adc461d1da

[Martyn Gigg]

I've done a merge of master to the branch to fix a conflict in ParameterMap.cpp from when Ricardo added a new method.

I'm also not convinced that these changes are responsible for performance test changes. The ​DirectInelasticDiagnostic one, for example, shows the big jump for a commit that only changed OptimizeLatticeForCellType (rev 692) which this workflow doesn't touch.

[Russell Taylor]

You're not looking at the right link to see the changes between system test runs. The link to a commit just shows the last change. Since a given system test run typically covers multiple commits you need to look at the 'diff' link, which in this case is ​this.

[Martyn Gigg]

Put back const char versions of ParameterMap get methods.

Removing them had an impact on performance when the get methods are called with C strings in loops as a std::string had to be constructed each time. Refs #8640

Changeset: 8b299df95897425ae07c1ede65aaa9dcebe166fe

[Martyn Gigg]

Ah yes of course, thanks. I've tracked the difference back to removing the const char methods in ParameterMap so I've put them back. The original set of changes were actually done and merged to develop incrementally: ​https://github.com/mantidproject/mantid/commit/7bff58ace35899aee71a7301d14c435a843f541a but the merge was reverted before the last release. That commit and its parents makes it easier to see the individual commit history that lead to the changes.

[Martyn Gigg]

LoadDetectorInfo now just loads parameters & adjust det positions

Refs #8640

Changeset: 541bdc92ad40e63f1fe51ed1738d7a11a5cd3157

[Martyn Gigg]

Move MARI det parameters to instrument level

This helps the performance of the LoadDetectorInfo algorithm. Also fixed a spelling error in LET_Definition.xml comment Refs #8640

Changeset: aeac566108ecf507ec841f503a880e81093631b1

[Martyn Gigg]

Swap out UpdateInstrumentFromFile for LoadDetectorInfo

This will allow users to overwrite the delays/pressures/thicknesses if required. Refs #8640

Changeset: a7c322cc24ccd65182448c76846dff3376d07884

[Martyn Gigg]

Paralleize creating vectors from the NeXus file.

Also kill a warning from the test. Refs #8640

Changeset: 836e755591e391cf253f09b4ba3895c0a277a3bb

[Martyn Gigg]

Merge branch 'feature/8640_restore_loaddetectorinfo_removal' into develop

Conflicts:

Code/Mantid/scripts/Inelastic/CommonFunctions.py

Refs #8640

Changeset: f98b042f35e86bb4ad9ff29fb566081c5faf87fb

[Martyn Gigg]

Merge remote-tracking branch 'origin/feature/8640_restore_loaddetectorinfo_removal'

Full changeset: ​eb7d72da85ee29a5550f68c2d44dbe2ea5fb534f

[Martyn Gigg]

Merge remote-tracking branch 'origin/feature/8640_restore_loaddetectorinfo_removal'

Full changeset: ​85df5a89961bdbbd5527b8362166a8e3d90eba4b

[Stuart Campbell]

(In #8578) This ticket has been transferred to github issue ​9422

[Stuart Campbell]

This ticket has been transferred to github issue ​9484