Ticket #7762 (closed: fixed)
Refactor LoadGroupXMLFile inside LoadDetectorsGroupingFile
Reported by: | Arturs Bekasovs | Owned by: | Arturs Bekasovs |
---|---|---|---|
Priority: | minor | Milestone: | Release 3.0 |
Component: | Muon | Keywords: | |
Cc: | Blocked By: | #7715, #7814 | |
Blocking: | Tester: | Wenduo Zhou |
Change History
comment:1 Changed 7 years ago by Arturs Bekasovs
- Milestone changed from Backlog to Release 3.0
- Description modified (diff)
- Summary changed from Make LoadDetectorsGroupingFile use shared pointer for maps to Refactor LoadDetectorsGroupingFile
comment:4 Changed 7 years ago by Arturs Bekasovs
- Summary changed from Refactor LoadDetectorsGroupingFile to Refactor LoadGroupXMLFile inside LoadDetectorsGroupingFile
comment:9 Changed 7 years ago by Arturs Bekasovs
- Blocked By 7814 added
Touches the same stuff. Better to update it all on one go.
comment:10 Changed 7 years ago by Arturs Bekasovs
- Description modified (diff)
Return value optimization should help to avoid performance issues. Much more troubles working with shared pointers.
comment:11 Changed 7 years ago by Arturs Bekasovs
- Description modified (diff)
We will not gain much anyway, so as soon as it works, leave it.
comment:12 Changed 7 years ago by Arturs Bekasovs
- Status changed from new to inprogress
Applies existing function and removed previously used ones.
Refs #7762
Changeset: 77f3137d9c8a4891d80715fe9411fba2b202b973
comment:13 Changed 7 years ago by Arturs Bekasovs
comment:14 Changed 7 years ago by Arturs Bekasovs
Tester:
Please check that test suite for the refactored class is rigorous enough and that it runs successfully. LoadDetectorsGroupingFileTest is what you should look at, as the LoadDetectorsGrouping uses LoadGroupXMLFile. The new function applied - String::parseRange - does have it's own test suite, you might want to look at it as well.
Check that the code changes seem reasonable.
comment:15 Changed 7 years ago by Arturs Bekasovs
- Status changed from inprogress to verify
- Resolution set to fixed
comment:16 Changed 7 years ago by Karl Palmen
- Status changed from verify to verifying
- Tester set to Karl Palmen
comment:17 Changed 7 years ago by Karl Palmen
- Status changed from verifying to reopened
- Resolution fixed deleted
Looks good. I find that the algorithm LoadDetectorsGroupingFile does what I expect.
To make the unit test more rigourous I suggest adding a function that tests that each group value gws->dataY(n)[0] is at least a great as the previous one gws->dataY(n-1)[0] and do a TS_FAIL if it finds the next one is of a lower number. Then with the extremes of the groups tested as at present one has then tested all the detectors.
If this slows down the test a lot, you can limit its use.
comment:18 Changed 7 years ago by Arturs Bekasovs
Good suggestion. Shouldn't slow tests down a lot, as there is usually quite a limited amount of detectors/spectra in the test data.
comment:19 Changed 7 years ago by Arturs Bekasovs
- Status changed from reopened to inprogress
Improved unit tests. Now all the detectors/spectra are checked.
Refs #7762
Changeset: 90caaaf670487c11eed411e12b1ecccc9c65df9e
comment:20 Changed 7 years ago by Arturs Bekasovs
- Status changed from inprogress to verify
- Resolution set to fixed
comment:21 Changed 7 years ago by Wenduo Zhou
- Status changed from verify to verifying
- Tester changed from Karl Palmen to Wenduo Zhou
comment:22 Changed 7 years ago by Wenduo Zhou
- Status changed from verifying to closed
The improved unit test is good. Ticket is thus closed.
comment:23 Changed 7 years ago by Wenduo Zhou
Merge remote-tracking branch 'origin/feature/7762_refactor_loadgroupxmlfile'
Full changeset: 3fd9a31072217eeec1e4a8ab39be37a708a2b0a1
comment:25 Changed 7 years ago by Arturs Bekasovs
- Blocking 7716 removed
(In #7716) Rethinking this, the pairing/naming etc. information is purely cosmetic stuff, which helps the users of MuonAnalysis to find the right groups and pairs quickly. We don't really need to store this information in a workspace, because none of the algorithms actually need it, they operate with group numbers / detector lists only.
That's why, it seems perfectly fine for the code to load/save that stuff to be a part of MuonAnalysis interface.
comment:26 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 8607