Port SiStripDetSummary from subdetector-DetId classes to TrackerTopology #20274
Conversation
Changed both printSummary and printDebug, for the classes in CondFormats/SiStripObjects and CalibFormats/SiStripObjects. Needed for SiStripDetSummary that is sometimes using a TrackerTopology (all these classes need to be changed because the methods are called by DummyCondObjPrinter, which is templated on the object type). SiStripDetVOff : changed implementation for getLVoffCounts and getHVoffCounts methods (SiStripDetSummary is not needed there)
…move initialization to a configure method (and get TrackerTopology from EventSetup)
…s TrackerTopology from EventSetup to printSummary
|
The code-checks are being triggered in jenkins. |
|
A new Pull Request was created by @pieterdavid (Pieter David) for master. It involves the following packages: CalibFormats/SiStripObjects @perrotta, @ghellwig, @monttj, @vazzolini, @kmaeshima, @arunhep, @cerminar, @dmitrijus, @cmsbuild, @franzoni, @slava77, @ggovi, @vanbesien, @lpernie can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
+code-checks |
|
@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:
|
|
This pull request has been submitted more than a week ago, with so far only feedback from reconstruction and the SiStrip commissioning team (@avartak is testing the changes in that context). |
|
+1 |
| #include "CondFormats/SiStripObjects/interface/SiStripDetSummary.h" | ||
|
|
||
| void SiStripDetSummary::add(const DetId & detid, const float & value) | ||
| void SiStripDetSummary::add(DetId detid, float value) |
There was a problem hiding this comment.
Hi @pieterdavid
Probably a naive question, but why are you removing the const'ness of the DetId at this stage?
There was a problem hiding this comment.
Hi @forthommel
I changed them from "pass by const reference" to "pass by value" because that is simpler (I was changing the implementation of this method and my eye fell on it).
Both DetId and float are small (32 bits), so there is no expensive copy to avoid with const & (a reference should be at least as big, and needs to be dereferenced, which is actually more work). See http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in for a longer explanation.
There was a problem hiding this comment.
Thanks a lot for your detailed explanation, @pieterdavid !
|
+1 |
|
everything looks fine for the part affecting the strips commissioning code |
|
+1 |
|
merge |
|
This pull request is breaking new pull request tests. See #20442 |
|
I think that that's because since testing this one, the payload inspector
PR by @mmusich was merged, which seems to use the old constructor still.
I'm sorry, I missed that (I don't have my laptop with me now, I can have a
proper look tomorrow evening at the earliest).
…On 8 Sep 2017 10:45 pm, "Chris Jones" ***@***.***> wrote:
This pull request is breaking new pull request tests. See #20442
<#20442>
The error is
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_4_X_2017-09-08-1100/src/CondCore/SiStripPlugins/plugins/SiStripDetVOff_PayloadInspector.cc: In member function 'virtual bool {anonymous}::SiStripDetVOffTest::fill(const std::vector, std::allocator > > >&)':
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_4_X_2017-09-08-1100/src/CondCore/SiStripPlugins/plugins/SiStripDetVOff_PayloadInspector.cc:171:22: error: no matching function for call to 'SiStripDetSummary::SiStripDetSummary()'
SiStripDetSummary summaryHV;
^~~~~~~~~
In file included from /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_4_X_2017-09-08-1100/src/CondCore/SiStripPlugins/plugins/SiStripDetVOff_PayloadInspector.cc:6:0:
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_4_X_2017-09-08-1100/src/CondFormats/SiStripObjects/interface/SiStripDetSummary.h:31:12: note: candidate: SiStripDetSummary::SiStripDetSummary(const TrackerTopology*)
explicit SiStripDetSummary(const TrackerTopology* tTopo) : computeMean_(true), trackerTopo_(tTopo)
```
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#20274 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIeN3bW4YM0FyoJy3CbqVns3oXqYywwBks5sgaeBgaJpZM4PCuH7>
.
|
|
Hi @pieterdavid, I've noticed your PR might have broken the use I made in mine of |
Changed SiStripDetSummary to store a
const TrackerTopology*. This class is mostly used from theprintSummaryandprintDebugmethods of CondFormats/SiStripObjects and CalibFormats/SiStripObjects classes, so I also added aconst TrackerTopology*argument to those. The rest of the commits update the modules that call these methods.CC: @OlivierBondu @alesaggio @vidalm ; tracked at #19398