Ticket #7765 (inprogress)
Refactor GroupDetectors2
| Reported by: | Arturs Bekasovs | Owned by: | Anyone |
|---|---|---|---|
| Priority: | minor | Milestone: | Backlog |
| Component: | Framework | Keywords: | Maintenance |
| Cc: | Blocked By: | #7772 | |
| Blocking: | Tester: |
Description (last modified by Arturs Bekasovs) (diff)
Use LoadDetectorsGroupingFile as a child algorithm to deal with files.
Change History
comment:1 Changed 7 years ago by Arturs Bekasovs
- Blocked By 2474 removed
- Description modified (diff)
- Blocking 2474 added
- Summary changed from Move code for loading files out of GroupDetectors2 to Refactor GroupDetectors2
comment:9 Changed 7 years ago by Arturs Bekasovs
- Blocking 2474 removed
(In #2474) Sorry for these blocking status changes, Trac had some strange internal error. Now it seems fine.
comment:9 Changed 7 years ago by Arturs Bekasovs
- Blocking 2474 added
Sorry for these blocking status changes, Trac had some strange internal error. Now it seems fine.
comment:10 Changed 7 years ago by Russell Taylor
Careful here - if you're talking about the *List algorithm properties here, we usually have to avoid removing these because it would break people's scripts. Also, we haven't had universally good experiences with these specialised workspaces (though MaskWorkspace has caused more trouble than GroupingWorkspace). Be sure to talk to Nick or Martyn before you start on this.
comment:11 Changed 7 years ago by Arturs Bekasovs
Russell, sorry for my poor explanation - I was not talking about removing *List properties, just about converting them internally to GroupingWorkspace so that a single method could be used to apply grouping no matter what combination of input parameters is provided. That's roughly how it is currently working, but custom data structures are used instead.
Speaking about GroupingWorkspace, couple of my tickets are touching it in some sense, and it actually sounds like a great idea to store such kind of information, I am surprised it caused problems. But apparently for now that's the only standard thing I can use.
There is almost nobody around this week, so I am basically outlining myself some work, but will surely discuss all these ideas with Martyn or Nick before starting.
comment:12 Changed 7 years ago by Arturs Bekasovs
- Description modified (diff)
The bit about adding .map support to LoadDetectorsGroupingFile was moved to #7772.
comment:14 Changed 7 years ago by Arturs Bekasovs
- Status changed from new to inprogress
- Description modified (diff)
After looking at the code more closely, it turns out that refactoring it to use GroupingWorkspace isn't making much sense. However, it is still true that the code for dealing with files should be moved to LoadDetectorsGroupingFile so that's what I will do in this ticket.
comment:16 Changed 7 years ago by Arturs Bekasovs
Initial code for using LoadDetectorsGroupingFile as a child algorithm.
Refs #7765
Changeset: d9753890136f7c59cb1046f851a8c58f819695fc
comment:17 Changed 7 years ago by Arturs Bekasovs
Progress report and other finishing touches.
Refs #7765
Changeset: 51ad7f9de437474e3dea710cd494fd076d8839f8
comment:18 Changed 7 years ago by Arturs Bekasovs
Make the test use .map extension for the Map file.
Before refactoring, algorithm was treating .xml as XML file, and everything else as Map. Now all the Map files should have .map extension, and error is thrown on anything except .xml and .map.
Refs #7765
Changeset: e002eae95f717b54d6cc733c963f8014f98ab199
comment:19 Changed 7 years ago by Arturs Bekasovs
Fix the bug with using grouping ws indexes, instead of input ws ones.
Refs #7765
Changeset: cb5b2f6a3f1d87501ab763f5e060e853cff7b7c7
comment:20 Changed 7 years ago by Arturs Bekasovs
Need to figure out what to do with the duplicate spectra IDs in the input (failing unit tests). Muon interface in particular needs this, but it couldn't be handled by GroupingWorkspace.
comment:21 Changed 7 years ago by Arturs Bekasovs
- Blocking 7716 added
(In #7716) Need to decide whether these algorithms will need to deal specifically with duplicate IDs.
comment:22 Changed 7 years ago by Arturs Bekasovs
- Milestone changed from Release 3.0 to Backlog
Will not have time to finish it in this iteration.
comment:24 Changed 7 years ago by Arturs Bekasovs
- Blocking 2474 removed
(In #2474) I am not really keen on this one anymore, as I've solved the problem I was having in a different way.
Though it still sounds like a good thing to implement at some point, so leaving it with Anyone.
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 7 years ago by Arturs Bekasovs
- Component changed from Framework to Muon
MuonAnalysis uses MuonGroupDetectors now, so the test for duplicate detector IDs should removed.
comment:27 Changed 7 years ago by Arturs Bekasovs
- Owner changed from Arturs Bekasovs to Anyone
- Component changed from Muon to Framework
I would like to concentrate on Muon stuff, which this one isn't a part of anymore.
However, this seems to be a useful thing to do at some point, so whoever decides this is necessary - feel free to use the code.
comment:28 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 8610
