Ticket #8839 (closed: fixed)
Refactor LoadMuonNexus to use MuonGroupDetectors for AutoGroup
Reported by: | Arturs Bekasovs | Owned by: | Arturs Bekasovs |
---|---|---|---|
Priority: | major | Milestone: | Release 3.2 |
Component: | Muon | Keywords: | Maintenance |
Cc: | Blocked By: | ||
Blocking: | #8835 | Tester: | Karl Palmen |
Description
Currently AutoGroup is implemented as a separate piece of code. For sake of consistency, it should be better to use loaded grouping (which we get for DetectorGroupingTable output option) and just pass it to MuonGroupDetectors. It will allow to localize any future problems and avoid something like this: #8835.
Should make sure the functionality is covered by unit tests before making any changes.
Change History
comment:3 Changed 7 years ago by Arturs Bekasovs
- Status changed from new to inprogress
Refs #8839. Add tests for AutoGroup option.
Single period one is passing. Multi period one is not, which is the problem in the current code (#8835).
Changeset: f2a087e8c9dc871909fea38dc77b13101327f0e2
comment:4 Changed 7 years ago by Arturs Bekasovs
Refs #8839. Remove old grouping code and clean-up a bit.
Changeset: 061be3e42a6579b95628b67c6c762d2fc688be5a
comment:5 Changed 7 years ago by Arturs Bekasovs
Refs #8839. Remove commented code from the tests.
Changeset: bf6ef80524f8e7ee2e7e5de0325e2e7affedcf8c
comment:6 Changed 7 years ago by Arturs Bekasovs
Refs #8839. Refactor grouping loading method.
It now returns the grouping, so that we can use it outside the method.
Changeset: 456307c9dcd9142382cdc86365e2a2c9ab6da253
comment:7 Changed 7 years ago by Arturs Bekasovs
Refs #8839. Make test more robust against exceptions and fix typo.
Changeset: d7702f2fabea5a38d0d50788c511663da73e765a
comment:8 Changed 7 years ago by Arturs Bekasovs
Refs #8839. Add grouping code
Changeset: e7351dad731281dbca99b6bd92069e8a9d11e1ff
comment:9 Changed 7 years ago by Arturs Bekasovs
Refs #8839. Fix the multi-period grouping.
For some reason, previous implementation was returning grouped workspaces as OutputWorkspaceN while still returning ungrouped workspaces as OutputWorkspace. I don't really understand why we would need OutputWorkpsaceN properties at all when we can just return a group.
Changeset: ed2eb059262db047ceed78ad7928f97532930ffa
comment:10 Changed 7 years ago by Arturs Bekasovs
Tester:
Verify that sensible unit tests were added covering AutoGroup functionality.
Make sure AutoTestData is in your user directories, and run the following script:
LoadMuonNexus("emu00006473.nxs", OutputWorkspace="emu_ungrouped") LoadMuonNexus("emu00006473.nxs", AutoGroup=True, OutputWorkspace="emu_grouped") LoadMuonNexus("MUSR00015189.nxs", OutputWorkspace="musr_ungrouped") LoadMuonNexus("MUSR00015189.nxs", AutoGroup=True, OutputWorkspace="musr_grouped")
emu_ungrouped should have 32 spectra, emu_grouped should have 2 spectra. All of musr_ungrouped workspaces should have 64 spectra, and all of musr_grouped workspaces should have 2 spectra.
Make sure you have SystemTests/Data in your user directories, and run the foloowing:
LoadMuonNexus("emu00031895.nxs", AutoGroup=True, OutputWorkspace="emu_no_groups")
Warning saying "Unable to load grouping from the file. Grouping not applied." should be printed to the log and all of emu_no_groups workspaces should contain 96 detectors.
comment:11 Changed 7 years ago by Arturs Bekasovs
- Status changed from inprogress to verify
- Resolution set to fixed
comment:12 Changed 7 years ago by Arturs Bekasovs
- Status changed from verify to reopened
- Resolution fixed deleted
comment:13 Changed 7 years ago by Arturs Bekasovs
- Status changed from reopened to inprogress
Refs #8839. Remove unused variable declarations.
Changeset: 0d7fe3664215ff36a979ead545df35fba036ee93
comment:14 Changed 7 years ago by Arturs Bekasovs
- Status changed from inprogress to verify
- Resolution set to fixed
comment:15 Changed 7 years ago by Karl Palmen
- Status changed from verify to verifying
- Tester set to Karl Palmen
comment:16 Changed 7 years ago by Karl Palmen
- Status changed from verifying to verify
- Tester Karl Palmen deleted
Test runs OK for first script, but the grouping was not as expected. Both musr were grouped and neither emu was grouped, but test did not tell me to check grouping.
I could not run the second script to check the warning, because I could not find the file to load (emu00031895.nxs). Also because of a bug in Trac, the second script was obscured by a horizontal scroll bar and could not be seen. I got at it my replying to the comment to get at the source and then cancelling the reply.
Because I can't find emu00031895.nxs even in the data archive. I pass this to testing pool.
comment:17 Changed 7 years ago by Arturs Bekasovs
Following Karl's comments:
Test runs OK for first script, but the grouping was not as expected. Both musr were grouped and neither emu was grouped, but test did not tell me to check grouping.
When I speak about grouping I mean detector grouping, not the workspace grouping. MUSR is multi-period run (read contains multiple workspaces), EMU is single-period, and the task is to check that detector grouping is applied in both cases.
I could not run the second script to check the warning, because I could not find the file to load (emu00031895.nxs).
One should be able to find emu00031895.nxs in Data folder of the systemtests repository.
comment:18 Changed 7 years ago by Karl Palmen
- Status changed from verify to verifying
- Tester set to Karl Palmen
comment:19 Changed 7 years ago by Karl Palmen
It works, but I did get Mantid to crash when running the second script with the data of the workspace of the same name being displayed I'll create a separate ticket for this.
comment:20 Changed 7 years ago by Karl Palmen
- Status changed from verifying to closed
Merge remote-tracking branch 'origin/feature/8839_loadmuonnexus_autogroup_using_muongroupdet'
Full changeset: 872f7f3aef4b417bee18ea029eb6bee59431346f
comment:21 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 9683
(In #8835) This problem should be solved when I change the way it works.