Generalize DetId parsing in TrackingNtuple #18403
Conversation
… TrackerTopology in the ntuple ROOT file
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: DataFormats/TrackerCommon @perrotta, @dmitrijus, @cmsbuild, @slava77, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| const TOBValues& tobVals() const { return tobVals_; } | ||
| const TIBValues& tibVals() const { return tibVals_; } | ||
| const TIDValues& tidVals() const { return tidVals_; } | ||
| const TECValues& tecVals() const { return tecVals_; } |
There was a problem hiding this comment.
This exposure was attempted earlier in #14230 , but it was eventually reverted in #14586 following comments from @davidlange6
@makortel
please check the discussion in #14230 and see if exposure of the internals can be avoided
There was a problem hiding this comment.
@slava77 The difference wrt. #14586/#14230 is that here I'd like to not do the DetId parsing in CMSSW, but dump the constants and do the parsing afterwards (in PyROOT, in principle independently of CMSSW).
If this exposure of TrackerTopology internals is to be avoided at all cost, I can implement the detector generalization to the ntuple in another way (essentially adding a branch for each element the union of possible DetId elements per pixel and strip subdetectors (*), which doesn't look too nice but is the simplest alternative).
(*) e.g. for pixels: layer, ladder, module, side, disk, blade, panel
There was a problem hiding this comment.
it all sounds like a work-around the inability to persist the TrackerTopology or recreate it from an fwlite/root environment.
So, if there was a dictionary for TrackerTopology, it would be enough.
There was a problem hiding this comment.
Kind of, yes. A complication is that the TrackerTopology constants are read from GT (not sure how that would work with fwlite; pardon my ignorance).
There was a problem hiding this comment.
GT is needed to create a TrackerTopology object from scratch.
If it's available in the ntupling cmsRun job and can be persisted, then it should be possible to get all functionality needed from fwlite.
There was a problem hiding this comment.
Ok, I was able to put TrackerTopology in the resulting root file after
- adding default constructor to TrackerTopology
- removing
constfrom all TrackerTopology members - adding dictionaries for TrackerTopology and all the inner structs (PixelBarrelValues etc)
- storing the TrackerTopology to another TTree (as the only branch and entry)
TFileService::make<TrackerTopology>complained that TrackerTopology does not inherit from TObject
From the usage point of view it is a bit unpleasant to have to convert the detid number to DetId object, e.g. ttopo.pxbLayer(ROOT.DetId(303087632)), but it's not that big of a deal.
Would persisting TrackerTopology be the preferred solution then?
There was a problem hiding this comment.
@davidlange6
please comment on the alternative to make TrackerTopology persistable so that the functionality desired in this PR is available outside of cmsRun application
There was a problem hiding this comment.
did you consider just storing the "fields" that get packed into the detid individually? That would be the usual "tuple"-ing approach
There was a problem hiding this comment.
i gather from the comment that you did - seems like the size penalty would be small and avoids duplicating all the parsing of detid code and internals. (squashing the various fields into one set of fields based on subdetid seems not hard)
There was a problem hiding this comment.
Ok. I'll prototype that approach as well (it's probably not a big deal).
|
On 4/21/17 6:10 AM, Matti Kortelainen wrote:
# removing |const| from all TrackerTopology members
I'm curious why this is needed
|
Because of the added default constructor, which itself appears to be needed to read the object from a file. |
|
+1 |
|
I bit the bullet and the alternative implementation is here #18491. Closing this one. |
This PR generalizes the (tracker) DetId parsing in the TrackingNtuple python library to be agnostic on the detector geometry. The chosen approach is to dump the TrackerTopology constants to the output root file, and use them in the python library. While it leads to some code duplication, it allows flexible debugging (including the TrackerTopology constants) without increasing the ntuple size (and if all the pieces of DetIds were stored on separate branches, one would have to deal with the fact that BPix/FPix and TIB/TID/TOB/TEC DetIds have different structure).
Tested in 9_1_0_pre2, no changes expected.
@rovere @VinInn @ebrondol