Ticket #8494 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

Move the Muon workflow out of the MuonAnalysis interface code

Reported by: Arturs Bekasovs Owned by: Arturs Bekasovs
Priority: major Milestone: Release 3.2
Component: Muon Keywords: Maintenance
Cc: Blocked By: #8506, #8534, #8550, #8597, #8702
Blocking: Tester: Gesner Passos

Description (last modified by Arturs Bekasovs) (diff)

Muon Interface has quite a long sequence of things it does to the loaded workspace before it actually gets plotted on the screen. Currently, all these steps are buried in the depths of the interface code. This is bad.

Firstly, because it makes them difficult to test. Currently, only the individual algorithms are tested, but not how they are working together.

Secondly, this makes it impossible to use the workflow outside the interface, which is currently required for #6473.

To solve similar kinds of problems, a notion of Workflow algorithm was introduced. Those are not doing much work themselves, but are instead setting parameters for child algorithms. This seems like a right thing to use inside Muon Analysis.

So, the initial plan is:

  1. Find out how many and what kind of algorithms are needed.
  2. Write rigorous tests for them.
  3. Implement algorithms so that tests are passing.
  4. Refactor MuonAnalysis interface code to use these algorithms instead.

Attachments

muon_workflow.png (110.9 KB) - added by Arturs Bekasovs 7 years ago.
Workflow of MuonAnalysis and MuonLoad

Change History

comment:1 Changed 7 years ago by Arturs Bekasovs

  • Description modified (diff)

comment:2 Changed 7 years ago by Arturs Bekasovs

  • Blocking 6473 added

(In #6473) I am unable to load workspace for fitting properly before the workflow of the MuonAnalysis is moved outside the interface.

comment:3 Changed 7 years ago by Arturs Bekasovs

Deleted

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

comment:4 Changed 7 years ago by Arturs Bekasovs

I will use a following approach to testing - it will be assumed, that the current results produced by MuonAnalysis interface are correct and I will test against them. This will make sure I've not broken anything that works already.

comment:5 Changed 7 years ago by Arturs Bekasovs

  • Status changed from new to inprogress

comment:6 Changed 7 years ago by Arturs Bekasovs

It was decided to implement these algorithms in separate tickets. I will add them as blockers of this one as I will go along and create them. When all of them are done, tester of this ticket should just verify that the overall job was done.

comment:7 Changed 7 years ago by Arturs Bekasovs

  • Blocked By 8506 added

comment:8 Changed 7 years ago by Arturs Bekasovs

  • Summary changed from [Muon] Create workflow algorithms and refactor the interface to use them to [Muon] Move the Muon workflow out of the MuonAnalysis interface code

The workflow was re-thought a bit. Now it utilizes more already-available algorithms. New list of things to implement:

  1. Update LoadMuonNexus - it should return DeadTimes as a TableWorkspace, so we can directly use it for ApplyDeadTimeCorr.
  2. Replace ApplyGroupingFromMuonNexus with MuonLoadGrouping and MuonGroupDetectors.
  3. Update Rebin algorithm to include new option "FullBinsOnly".
  4. Create workflow algorithm MuonCreateWorkspace.
  5. Create top-level workflow algorithm MuonLoad.

While doing all that, again, appropriate part of the MuonAnalysis should be refactored to move out the calculations.

comment:9 Changed 7 years ago by Arturs Bekasovs

  • Blocked By 8534 added

comment:10 Changed 7 years ago by Arturs Bekasovs

  • Blocked By 8550 added

comment:11 Changed 7 years ago by Arturs Bekasovs

  • Blocked By 8597 added

comment:12 Changed 7 years ago by Arturs Bekasovs

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

comment:13 Changed 7 years ago by Arturs Bekasovs

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

comment:14 Changed 7 years ago by Arturs Bekasovs

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

comment:15 Changed 7 years ago by Arturs Bekasovs

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

comment:16 Changed 7 years ago by Arturs Bekasovs

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

comment:17 Changed 7 years ago by Arturs Bekasovs

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

comment:18 Changed 7 years ago by Arturs Bekasovs

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

comment:19 Changed 7 years ago by Arturs Bekasovs

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

comment:20 Changed 7 years ago by Arturs Bekasovs

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

comment:21 Changed 7 years ago by Arturs Bekasovs

  • Blocking 6473 removed

(In #6473) This was mostly done. Option for Rebin is a tiny little thing which stops me from closing that ticket, so I've implemented it in both places and will re-factor that during maintenance.

comment:22 Changed 7 years ago by Arturs Bekasovs

  • Blocked By 8702 added

comment:23 Changed 7 years ago by Arturs Bekasovs

  • Keywords Maintenance added
  • Milestone changed from Release 3.1 to Backlog

Done to an extent which allowed to complete sequential fitting. Closing and checking properly can be left for maintenance.

comment:24 Changed 7 years ago by Arturs Bekasovs

  • Milestone changed from Backlog to Release 3.2

comment:25 Changed 7 years ago by Arturs Bekasovs

  • Summary changed from [Muon] Move the Muon workflow out of the MuonAnalysis interface code to Move the Muon workflow out of the MuonAnalysis interface code

Changed 7 years ago by Arturs Bekasovs

Workflow of MuonAnalysis and MuonLoad

comment:26 Changed 7 years ago by Arturs Bekasovs

  • Status changed from inprogress to verify
  • Resolution set to fixed

Tester:

I wasn't able to move out too much, because the interface uses a lot of intermediate values to allow quick experimenting with loaded data. That's why I couldn't use e.g. MuonLoad to do everything for me and need to replicate the workflow in the interface.

Though one of the biggest aims was achieved - the workflow was analysed, formalised using the diagram attached and expressed as the new MuonLoad algorithm.

So the testing here comes to fetching the latest master and reading through the MuonAnalysis.cpp. Verify that there are no calculations done in the interface code and all it does is passing appropriate workspaces and parameters to algorithms, according to the attached diagram. Try to find places which could potentially be re-factored out.

comment:27 Changed 7 years ago by Gesner Passos

  • Status changed from verify to verifying
  • Tester set to Gesner Passos

comment:28 Changed 7 years ago by Gesner Passos

  • Status changed from verifying to closed

There are no calculations done directly through MuonAnalysis.cpp. It seems that some lines of code could be potentially saved not only in yours but generally if createChildAlgorithm or something likely could be made static.

The real problem is the:

That's why I couldn't use e.g. MuonLoad to do everything for me and need to replicate the workflow in the interface.

New requests or maintenance will require checking both places always. In some places, people defined a HelperClass that was uses to share between both classes, hence, allowing the replication. I'm not quite sure how we could apply here, but I think it is worth discussing with Martyn to see if he could suggest a nice approach.

comment:29 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 9338

Note: See TracTickets for help on using tickets.