Ticket #8494 (closed: fixed)
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:
- Find out how many and what kind of algorithms are needed.
- Write rigorous tests for them.
- Implement algorithms so that tests are passing.
- Refactor MuonAnalysis interface code to use these algorithms instead.
Attachments
Change History
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: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: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: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:
- Update LoadMuonNexus - it should return DeadTimes as a TableWorkspace, so we can directly use it for ApplyDeadTimeCorr.
- Replace ApplyGroupingFromMuonNexus with MuonLoadGrouping and MuonGroupDetectors.
- Update Rebin algorithm to include new option "FullBinsOnly".
- Create workflow algorithm MuonCreateWorkspace.
- 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:12 Changed 7 years ago by Arturs Bekasovs
comment:13 Changed 7 years ago by Arturs Bekasovs
comment:14 Changed 7 years ago by Arturs Bekasovs
comment:15 Changed 7 years ago by Arturs Bekasovs
comment:16 Changed 7 years ago by Arturs Bekasovs
comment:17 Changed 7 years ago by Arturs Bekasovs
comment:18 Changed 7 years ago by Arturs Bekasovs
comment:19 Changed 7 years ago by Arturs Bekasovs
comment:20 Changed 7 years ago by Arturs Bekasovs
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: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: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
- Attachment muon_workflow.png added
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