LoadISISNexus with partial spectra loading does not assign correct monitors to loaded spectra

[Alex Buts]

Take ISIS nexus histogram mode data file LET00018118.nxs (any recent 11doors should do) and run the following script:

Load(Filename='LET00018118.nxs', OutputWorkspace='LET00018118', LoaderName='LoadISISNexus', LoaderVersion=2)
ExtractSingleSpectrum(InputWorkspace='LET00018118', OutputWorkspace='BLET0001773', WorkspaceIndex=73733)


Load(Filename='LET00018118.nxs', OutputWorkspace='ALET00018118', LoaderName='LoadISISNexus', LoaderVersion=2, SpectrumMin=73734, SpectrumMax=73734)

Then look at the detectors for spectra BLET00018118 and ALET00018118. The first would be Spectrum index 0 Spectrum No 73,734, monitor (this is correct) and the second would be Spectrum index 0 Spectrum No 1 (A detector, corresponding to the first detector in the whole loaded instrument spectra) This is wrong.

[Alex Buts]

refs #10088 Failing attempt to change loading

(does not work correctly)

Changeset: f17db4d62b10ac2eaee1b78b8e6c573337501bc6

[Alex Buts]

refs #10088 Change necessary for working without precompiled headers

Changeset: 7026fcf1ff04db4f7e8c5edd7b3dada751172bb8

[Alex Buts]

refs #10088 Map is build seems correctly but detID-s are still wrong

Changeset: 8cdfaf2eb7b9910235d94397ce9160a32404a734

[Alex Buts]

refs #10088 Mainly done, checks and optimization is needed

Changeset: 9db55ebed6bce4051ed2c393764c93339af6c168

[Alex Buts]

refs #10088 Got rid of m_spectra_list in favor of m_specNum2SpecID map

Changeset: 5783a2aa7e7cdc06683dcba257898ca76d35eb00

[Alex Buts]

refs #10088 Unit tests to check the changes and correct behavior

+ final bugs fixed for LoadISISNexus2

Changeset: c39baa82c348f4e160bcbc45d8a8610af878e2c1

[Alex Buts]

refs #10088 Doxygen errors

Changeset: accdf340bc898d2dcd4c5842921a901bc403e966

[Alex Buts]

refs #10088 GCC warnings

Changeset: 032ff6f0233c00563775a5df74b6e58d4c786fac

[Alex Buts]

refs #10088 Prospective Coverity warnings

Changeset: b6b27700fad6ca2914395f8abaac80bbbfaf581c

[Alex Buts]

refs #10088 a bit more tests for different options

especially correspondence between spectra and detectors maps.

Changeset: 86656b220058344ab822ed955fb196370111766d

[Alex Buts]

both ways of loading a spectra in the script above should now return the same result.

I have also added unit tests which verify correct behaviour. A tester may try them first to ensure they fail before this fix. Now they pass.

A careful tester may pick a different ISIS nexus file (histogram more), try to load different spectra and spectra ranges and ensure that he gets the expected results. (partially loaded workspace's spectra have spectra numbers and detectors ID-s corresponding to the full workspace spectra and detector's ID-s.

[Martyn Gigg]

I'm happy that the code now seems to be doing the correct thing. I tried the LET example along with a mutli-period dataset from POLREF to check the partial loading of different spectra from multiple periods and it all looks fine.

Given the limited scope of the changes I'd be happy for this to make the patch. There is one minor thing that I think would improve understanding of the code and that is the name m_specInd2specNum_map and the associated method buildSpectraInd2SpectraNumMap. I think the term spectra index is confusing as it's not a term we use very often as we more often refer to it has workspace index. It is also not good style to have numbers in functions names and we should use the word to so could we rename that to something like m_wsIndexToSpecNumMap and buildWSIndexToSpecNumMap.

[Alex Buts]

Spectra index is confusing term but this is what is written as the column name in the detectors table. Despite I do not like it, I tried to adhere to what is there.

This issue have no relation to the ticket and reflects personal preferences so if it worth renaming (may be it worth, I would agree with this -- but it should have wider scope), please write different ticket for it.

[Martyn Gigg]

Merge remote-tracking branch 'origin/bugfix/10088_PartialSpectraLoading'

Full changeset: ​6d060d104414ccfac23795ad37dc27d0fdfdea1d

[Alex Buts]

Cherry-pick fixes from bugfix/10088_PartialSpectraLoading into next

Changes:

refs #10088 Failing attempt to change loading

(does not work correctly) (cherry picked from commit f17db4d62b10ac2eaee1b78b8e6c573337501bc6)

Conflicts:

Code/Mantid/Framework/DataHandling/src/LoadISISNexus2.cpp

refs #10088 Change necessary for working without precompiled headers (cherry picked from commit 7026fcf1ff04db4f7e8c5edd7b3dada751172bb8)

refs #10088 Map is build seems correctly but detID-s are still wrong (cherry picked from commit 8cdfaf2eb7b9910235d94397ce9160a32404a734)

refs #10088 Mainly done, checks and optimization is needed (cherry picked from commit 9db55ebed6bce4051ed2c393764c93339af6c168)

refs #10088 Got rid of m_spectra_list in favor of m_specNum2SpecID map (cherry picked from commit 5783a2aa7e7cdc06683dcba257898ca76d35eb00)

refs #10088 Unit tests to check the changes and correct behavior

+ final bugs fixed for LoadISISNexus2 (cherry picked from commit c39baa82c348f4e160bcbc45d8a8610af878e2c1)

Conflicts:

Code/Mantid/Framework/DataHandling/test/LoadISISNexusTest.h

refs #10088 Doxygen errors (cherry picked from commit accdf340bc898d2dcd4c5842921a901bc403e966)

refs #10088 GCC warnings (cherry picked from commit 032ff6f0233c00563775a5df74b6e58d4c786fac)

refs #10088 Prospective Coverity warnings (cherry picked from commit b6b27700fad6ca2914395f8abaac80bbbfaf581c)

refs #10088 a bit more tests for different options

especially correspondence between spectra and detectors maps. (cherry picked from commit 86656b220058344ab822ed955fb196370111766d)

Changeset: d39162507815916773b333de461f807df3fa8342

[Stuart Campbell]

This ticket has been transferred to github issue ​10930