Deprecation of T**DetId in CalibTracker #19393
Conversation
|
A new Pull Request was created by @alesaggio for master. It involves the following packages: CalibTracker/SiStripChannelGain @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| uint32_t getTIDwheel ( DetId detId ) { return ((detId.rawId()>>11) & 0x3); }; | ||
| uint32_t getTECside ( DetId detId ) { return ((detId.rawId()>>18) & 0x3); }; | ||
| uint32_t getTECring ( DetId detId ) { return ((detId.rawId()>> 5) & 0x7); }; | ||
| uint32_t getTECwheel ( DetId detId ) { return ((detId.rawId()>>14) & 0xF); }; |
There was a problem hiding this comment.
@mdelcourt I think you are the main user for this code.
Can you just have a cross-check look? Thanks.
|
tracked at #19398 |
|
I would suggest to take a different approach for these macros: instead of recoding the detid methods (or TrackerTopology methods), you could instantiate your own TrackerTopology. The only thing you have to do is to hardcode one instance of the TOBValues struct (and others). I think that it makes the code more readable and easy to change since all hardcoded values are there and not in the various methods. It makes the code more similar to the rest of CMSSW, and moves the burden/risk to the definition of these structs, in sync with the geometry. Maybe (later?) this could even be read from trackerParameters.xml |
|
eg a new constructor to TrackerTopology TrackerTopology::TrackerTopology(enum LegacyTrackerNumbering) it could have a rather big impact on any code called from a tight loop. |
|
but at least centralizes all the hardwired numbers that will eventually be wrong |
|
I added a header file (LegacyTrackerTopology.h) that hardcodes TrackerTopology instances as discussed above. Please let me know what you think |
| #include <boost/lexical_cast.hpp> | ||
| #include <TChain.h> | ||
| #include <TFile.h> | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
@alesaggio is this still needed after having introduced the LegacyTopology stuff?
There was a problem hiding this comment.
No, indeed! I'll fix it
|
|
||
| namespace LegacyTrackerTopology { | ||
|
|
||
| inline static std::unique_ptr<TrackerTopology> getTrackerTopology() |
There was a problem hiding this comment.
Hi @alesaggio,
I think there might be a bit of ambiguity on what "Legacy" actually means in this context for what concern the Pixel part. Your implementation here is for Phase-0 Pixel Tracker only - I think it should be written somewhere as a disclaimer or allow the possibility to construct the Topology in this way also for Phase-I detector (since Phase0 Strips overlap in time with Phase-I Pixel, i.e. the current detector)
| unsigned layer = isTIB ? tTopo_->tibLayer(detid) : tTopo_->tobLayer(detid); | ||
| bool stereo = isTIB ? tTopo_->tibStereo(detid) : tTopo_->tobStereo(detid); | ||
|
|
||
| return subdetLabel(detid)+"_layer"+boost::lexical_cast<std::string>(layer)+(stereo?"s":"a"); |
There was a problem hiding this comment.
in light of #19645 I guess you can get rid of the ugly boost::lexical_cast in favor of std::to_string
2 similar comments
| #include <memory> | ||
|
|
||
| // WARNING: this header has been introduced to call a TrackerTopology | ||
| // object whenever it is not possible to access it through an EventSetup. |
There was a problem hiding this comment.
what are the cases where the EventSetup can not deliver this?
TrackerTopologyEP and relevant upstream can be set up to provide the phase-0 topology just as it is done in normal running.
There was a problem hiding this comment.
Hi @slava77, a couple of use cases are:
- from within a ROOT macro (as in
CalibTracker/SiStripChannelGain/test/UsefullCode/TimeHistory/PlotMacro.C) - from a payload inspector application (see discussion at: [92X] Add Payload Inspector classes for SiStripApvGains #19193 (comment) and following)
The proposed implementation is actually inspired by David's #19393 (comment), though I am not sure this is exactly what he had in mind at the time.
|
-1 merge conflicts also, the hardcoded pieces in LegacyTrackerTopology.h do not seem appropriate, given that there are ways to get the details from ES as it's done for all other tracker configurations. |
|
I'm sorry, I did not think about that, I will move it somewhere else. |
| } | ||
| } | ||
| } | ||
| } catch (...) { |
There was a problem hiding this comment.
@alesaggio @pieterdavid I think that the catch(...) construct is disallowed in CMSSW, at least it is flagged as violation of a code rule by the static analyzer.
There was a problem hiding this comment.
Thanks for reviewing. I added an explicit catch for the remaining ones that xerces may throw
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 . differences in HLT directory https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_3_X_2017-07-30-1100+19393/21650/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/HLT.html are probably because of this extra commit 8e59646 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
| @@ -7,6 +7,8 @@ | |||
|
|
|||
| from CalibTracker.SiStripLorentzAngle.MeasureLA_cff import METHOD_WIDTH, METHOD_PROB1, METHOD_AVGV2, METHOD_AVGV3, METHOD_RMSV2, METHOD_RMSV3 | |||
|
|
|||
| process.load('Configuration.Geometry.GeometryIdeal_cff') | |||
There was a problem hiding this comment.
hi @alesaggio - you may want to confirm this is the geometry CFF that you want. It is usually the wrong one.
There was a problem hiding this comment.
Hi @davidlange6, how can I know which one is correct? I have no clue. Maybe @mmusich or @dimattia know?
There was a problem hiding this comment.
Hi @alesaggio, I think that Configuration.Geometry.GeometryRecoDB_cff shall suffice for this use-case. We can follow to this one in another PR, the CalibTracker/SiStripLorentzAngle needs anyway heavy maintenance.
|
+1 |
In the modified files we don't have a way to access the edm::EventSetup, hence the TrackerTopology cannot be retrieved. The only solution is to get specific pieces of code from T**DetId.h (in such a way that it's not easy to use from elsewhere).
cc @pieterdavid @OlivierBondu @vidalm