Ticket #7762 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

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

Description (last modified by Arturs Bekasovs) (diff)

In #7772 a parseRange function was added to Kernel::Strings, so parseRangeText is not needed any more. parseDetectorIDs() and parseSpectrumIDs() are really just parsing the output of parseRangeText, so these could be removed as well.

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:2 Changed 7 years ago by Arturs Bekasovs

  • Description modified (diff)

comment:3 Changed 7 years ago by Arturs Bekasovs

  • Description modified (diff)

comment:4 Changed 7 years ago by Arturs Bekasovs

  • Summary changed from Refactor LoadDetectorsGroupingFile to Refactor LoadGroupXMLFile inside LoadDetectorsGroupingFile

comment:5 Changed 7 years ago by Arturs Bekasovs

  • Blocking 7716 added

comment:6 Changed 7 years ago by Arturs Bekasovs

  • Description modified (diff)

comment:7 Changed 7 years ago by Arturs Bekasovs

  • Blocked By 7715 added

comment:8 Changed 7 years ago by Arturs Bekasovs

  • Description modified (diff)

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

Removed no longer relevant todo.

Refs #7762

Changeset: 3d50341cb146d493682f51839bdd05115077134c

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.

Last edited 7 years ago by Arturs Bekasovs (previous) (diff)

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:24 Changed 7 years ago by Nick Draper

  • Component changed from Framework to Muon

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

Note: See TracTickets for help on using tickets.