bsunanda:Run2-hcx114 Correct CaloTowerConstituent map with the merged Ids for Plan1 #17627
Conversation
|
A new Pull Request was created by @bsunanda for CMSSW_9_0_X. It involves the following packages: Geometry/CaloTopology @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
@cmsbuild Please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
| if (std::find(items.begin(),items.end(),hid) == items.end()) { | ||
| items.push_back(hid); | ||
| #ifdef EDM_ML_DEBUG | ||
| std::cout << id << " Depth " << i << ":" << i+sd << " " << hid <<"\n"; |
There was a problem hiding this comment.
@bsunanda - it is better to use LogDebug
| HcalDetId hid(HcalOuter,hcal_ieta*id.zside(),id.iphi(),i+sd); | ||
| items.push_back(hid); | ||
| #ifdef EDM_ML_DEBUG | ||
| std::cout << id << " Depth " << i << ":" << i+sd << " " << hid <<"\n"; |
There was a problem hiding this comment.
@bsunanda - please, use LogDebug
|
+1 The number of CaloTower constituents is reduced when HCal merges DetIds. |
|
Hi @ianna- this is basically accepted practice - see for example #17511
… On Feb 25, 2017, at 9:43 AM, Ianna Osborne ***@***.***> wrote:
@ianna commented on this pull request.
In general, LogDebug should be used instead of std::cout
In Geometry/CaloTopology/src/CaloTowerConstituentsMap.cc:
> @@ -94,35 +96,64 @@ std::vector<DetId> CaloTowerConstituentsMap::constituentsOf(const CaloTowerDetId
if (standardHB_) {
if (id.ietaAbs()<=m_cttopo->lastHBRing()) {
m_hcaltopo->depthBinInformation(HcalBarrel,hcal_ieta,id.iphi(),id.zside(),nd,sd);
- for (int i=0; i<nd; i++)
- items.push_back(HcalDetId(HcalBarrel,hcal_ieta*id.zside(),id.iphi(),i+sd));
+ for (int i=0; i<nd; i++) {
+ HcalDetId hid = m_hcaltopo->mergedDepthDetId(HcalDetId(HcalBarrel,hcal_ieta*id.zside(),id.iphi(),i+sd));
+ if (std::find(items.begin(),items.end(),hid) == items.end()) {
+ items.push_back(hid);
+#ifdef EDM_ML_DEBUG
+ std::cout << id << " Depth " << i << ":" << i+sd << " " << hid <<"\n";
@bsunanda - it is better to use LogDebug
In Geometry/CaloTopology/src/CaloTowerConstituentsMap.cc:
> }
}
if (standardHO_) {
if (id.ietaAbs()<=m_cttopo->lastHORing()) {
m_hcaltopo->depthBinInformation(HcalOuter,hcal_ieta,id.iphi(),id.zside(),nd,sd);
- for (int i=0; i<nd; i++)
- items.push_back(HcalDetId(HcalOuter,hcal_ieta*id.zside(),id.iphi(),i+sd));
+ for (int i=0; i<nd; i++) {
+ HcalDetId hid(HcalOuter,hcal_ieta*id.zside(),id.iphi(),i+sd);
+ items.push_back(hid);
+#ifdef EDM_ML_DEBUG
+ std::cout << id << " Depth " << i << ":" << i+sd << " " << hid <<"\n";
@bsunanda - please, use LogDebug
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@davidlange6 the use of cout maybe common practice but it is not accepted practice from the Framework group given the threading problems. |
|
cout compiled away behind a ifdef.. anyway, one can certainly discuss easy to adopt alternatives (but I don’t consider the current message logger one of them)
… On Feb 25, 2017, at 3:03 PM, Chris Jones ***@***.***> wrote:
@davidlange6 the use of cout maybe common practice but it is not accepted practice from the Framework group given the threading problems.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Yes it is indeed a bug fix for CaloTower. The earlier treatment was not giving correct results as seen by RecoJet code |
|
On 2/25/17 5:41 AM, David Lange wrote:
Hi @bsunanda <https://github.com/bsunanda> @kpedro88
<https://github.com/kpedro88> @slava77 <https://github.com/slava77> - I
assume that this pr is both important for pre5 and also the last thing
we are expecting. is that your understanding too?
I haven't looked at this yet.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17627 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbkbyYlOaerEvjGiPEAhQ2ihHn9Fhks5rgC-kgaJpZM4MLyky>.
|
|
The tests are being triggered in jenkins. |
|
@cmsbuild please test the last build attempt has failed |
|
The tests are being triggered in jenkins. |
cmsbuild
added
tests-pending
tests-started
and removed
tests-started
tests-pending
labels
|
In local tests, using CMSSW_9_0_X_2017-02-25-1100 as a baseline, I see that the Depth-dependent observables are affected by this PR in 2017 workflows (plan-1 versions), although the plots are not very suggestive qualitatively Among the fwlite-based comparisons, the following plots have differences Here is an example from 10224 (similar plots appear also in the standard DQM). |
|
+1
|
|
Comparison job queued. |
|
+1 |
cmsbuild
added
fully-signed
geometry-approved
and removed
geometry-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
+1
On 26 Feb 2017, at 08:26, cmsbuild <notifications@github.com<mailto:notifications@github.com>> wrote:
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6<https://github.com/davidlange6>, @smuzaffar<https://github.com/smuzaffar>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#17627 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEzyw4ehhEsluwnnFhrYHkVOnkFLAPSXks5rgSjcgaJpZM4MLyky>.
|
Removes the warnings in JetIdHelper class