Dataformats and inputs only for PF HEM 15-16 mitigation #24320
Conversation
…turned on by era)
…photon producer configs
… on the 'Bc' version as suggested by Sam
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24320/6067 |
|
A new Pull Request was created by @gpetruc (Giovanni Petrucciani) for master. It involves the following packages: DataFormats/CaloRecHit @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
any chance of getting this one merged? |
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
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 will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
@fabiocos |
|
I've played a bit around this (detId and conditions) protection re-running ("critical") step3 for two wf's from runTheMatrix (2017 and 2013) : And while it's good that only good cells are accepted, but arrival of unphysical HcalDetIds in (2): As to (1) - I've added HcalTopology to EgammaHadTower constructor (+ relevant #include's ) and then used Sunanda's suggestion (should be effective only for 2017 "collapsed" depth NB: fixes the issue for this wf. (for 2017 "Plan 1" in general), so it's worth adding it. @gpetruc @bsunanda @gpetruc I saw a warning "PROPAGATION TO THE HCAL ENTRANCE HAS FAILED" before (2) |
|
Thanks Salavat. I shall investigate the issue of 2023 geometry once the PR is fixed. Please let me know how to remove the protection in testing. That will help me in debugging the case.
…________________________________
From: Salavat Abdullin [notifications@github.com]
Sent: 21 August 2018 17:02
To: cms-sw/cmssw
Cc: Sunanda Banerjee; Mention
Subject: Re: [cms-sw/cmssw] Dataformats and inputs only for PF HEM 15-16 mitigation (#24320)
I've played a bit around this (detId and conditions) protection
https://github.com/cms-sw/cmssw/pull/24320/files#diff-9c61d9003eec5179550bfe62d0ea6fbeR128
running ("critical") step3 for two wf's from runTheMatrix (2017 and 2013) :
(1) runTheMatrix.py -l 136.388 &
(2) runTheMatrix.py -l 20034.0 &
________________________________
And while it's good that only good cells are accepted, but arrival of unphysical HcalDetIds in (2):
DEBUG: hasActiveHcal called with 1 detids. First tower detid ieta 0 iphi
EgammaHadTower DetId 43f00000 hid.rawId 43f00000 sub 1 ieta 0 iphi 0 depth 15
(indeed this is HCAL HB with abnormal depth 15 and empty fields for ieta/iphi)
http://cmslxr.fnal.gov/source/DataFormats/HcalDetId/interface/HcalDetId.h#0025)
is quite worrisome. There no any such HcalDetId in the list of cells printed from CalibCalorimetry/HcalPlugins/src/HcalHardcodeCalibrations.cc
and it (naturally) doesn't appear in HCAL Reco
(in HcalDigis input dumped for the event of interest from HBHEPhase1Reconstructor).
________________________________
As to (1) - I've added HcalTopology to EgammaHadTower constructor (+ relevant #include's )
edm::ESHandle geoHandle;
es.get().get(geoHandle);
edm::ESHandle hcalTopology;
es.get().get( hcalTopology );
hcalTopology_ = hcalTopology.product();
and then used Sunanda's suggestion (should be effective only for 2017 "collapsed" depth
(and here it's used without intermediate pointer) :
int status = hcalQuality_->getValues((DetId)(hcalTopology_->idFront(HcalDetId(id))),/throwOnFail=/false)->getValue();
NB: fixes the issue for this wf. (for 2017 "Plan 1" in general), so it's worth adding it.
@gpetruc<https://github.com/gpetruc> @bsunanda<https://github.com/bsunanda>
it's painful to compile this PR + 10_3_0_pre2 (160+ packages overnight in my case) but once it's merged, we might need to investigate (using step3 of the mentioned wf's) what's going on with EgammaHadTower (id's) in 2023 case...
@gpetruc<https://github.com/gpetruc> I saw a warning "PROPAGATION TO THE HCAL ENTRANCE HAS FAILED" before (2)
crashed in EgammaHadTower. I assume this is harmless warning (?).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#24320 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEzMusWYaT0PHE7xLQJ_pSGxLXSdvHi2ks5uTCDqgaJpZM4WBJL8>.
|
|
@bsunanda |
|
+1 |
|
also @abdoulline @bsunanda regarding the HCAL DetId issues: I turned on debug output in https://github.com/cms-sw/cmssw/blob/master/Geometry/CaloTopology/src/CaloTowerConstituentsMap.cc to see if anything jumped out. In the 2023 case, these messages about a "Tower (0,0)" appear much after all the other towers are constructed, and even after other modules have run. So I think some module is making a bad call to For 2017, the merged depth DetIds are used by the CaloTower code: but the conditions correspond to the unmerged hits, so there will have to be some patch for this. |
|
If this PR gets into the next IB I shall take a look into this.
…________________________________
From: Kevin Pedro [notifications@github.com]
Sent: 21 August 2018 23:48
To: cms-sw/cmssw
Cc: Sunanda Banerjee; Mention
Subject: Re: [cms-sw/cmssw] Dataformats and inputs only for PF HEM 15-16 mitigation (#24320)
also @abdoulline<https://github.com/abdoulline> @bsunanda<https://github.com/bsunanda> regarding the HCAL DetId issues: I turned on debug output in https://github.com/cms-sw/cmssw/blob/master/Geometry/CaloTopology/src/CaloTowerConstituentsMap.cc to see if anything jumped out.
In the 2023 case, these messages about a "Tower (0,0)" appear much after all the other towers are constructed, and even after other modules have run. So I think some module is making a bad call to constituentsOf(), or some other strange issue.
Tower (0,0) Depth 0:-1 (HB 0,0,15)
Tower (0,0) Depth 1:0 (HB 0,0,0)
Tower (0,0) Depth 0:4 (HO 0,0)
Tower (0,0) Depth 0:-1 (HB 0,0,15)
Tower (0,0) Depth 1:0 (HB 0,0,0)
Tower (0,0) Depth 0:4 (HO 0,0)
Tower (0,0) Depth 0:-1 (HB 0,0,15)
Tower (0,0) Depth 1:0 (HB 0,0,0)
Tower (0,0) Depth 0:4 (HO 0,0)
Tower (0,0) Depth 0:-1 (HB 0,0,15)
Tower (0,0) Depth 1:0 (HB 0,0,0)
Tower (0,0) Depth 0:4 (HO 0,0)
For 2017, the merged depth DetIds are used by the CaloTower code:
https://github.com/cms-sw/cmssw/blob/2cf9223953b0209c6a0a326761b210637f9b4e8d/Geometry/CaloTopology/src/CaloTowerConstituentsMap.cc#L135
but the conditions correspond to the unmerged hits, so there will have to be some patch for this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#24320 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEzMui4ER_y9jUVMkJzf0lto55fZ9FBhks5uTIAugaJpZM4WBJL8>.
|
|
it's merged, but it missed the -2300 IB. So, it should show up in the -1100 IB tomorrow. |
Minimal subset of #24212 with the DataFormat changes and not touching the PF algorithm:
This should require no changes to HLT config and have no conflicts anywhere, and so integrating this in an IB should speed up the integration of the rest of the PR with the changes to the PF code.
Still based on
CMSSW_10_3_X_2018-08-15-1100