Fix collapsed status in HCAL topology #22001
Conversation
|
The code-checks are being triggered in jenkins. |
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22001/3090 |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @kpedro88 (Kevin Pedro) for master. It involves the following packages: Configuration/PyReleaseValidation @perrotta, @cmsbuild, @prebello, @Dr15Jones, @vazzolini, @kmaeshima, @civanch, @fwyzard, @ianna, @kpedro88, @fabozzi, @Martin-Grunewald, @silviodonato, @jfernan2, @mdhildreth, @slava77, @GurpreetSinghChahal, @vanbesien, @dmitrijus can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Comparison job queued. |
|
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
|
+1 |
|
+1 |
|
+1 |
|
+1 |
|
@kpedro88 : while looking at the jenkins tests outputs, I notice significant differences in the standard, "non-collapsed", 10824.0 TTbar 2018 workflow even outside the HE region, e.g.:
Can you check and comment on it? |
|
Moreover: you say in #22001 (comment) that changes are expected because previous PR (was it #21842?) the topology was incorrectly being treated as collapsed. However in #21842 there were not changes in wf 10824.0, that I would have expected if the topology was incorrectly being treated as collapsed there: am I missing something? |
|
@perrotta the previous PR had some inconsistencies, but the problem actually started when we updated the geometry in the GT to use the collapsible version. The CaloTower plot changes (outside of 28/29) look like random fluctuations to me due to the change in HcalGeometry which could affect GEN-SIM. |
|
@kpedro88, @perrotta
(edited) actually a single-pion test (one-Go from scratch) shows some small changes
(with this branch=PR=fix) coming already at SIM level, which inevitably adds rndm changes at DIGI level:
https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/Validation/SVSuite/HCAL/calo_scan_single_pi/10_0_0/10_0_0_2018C_fix_vs_10_0_0_2018UC_fix_SinglePi/
…On Wed, 31 Jan 2018, Kevin Pedro wrote:
@perrotta the previous PR had some inconsistencies, but the problem actually started when we updated the geometry in the GT
to use the collapsible version. The CaloTower plot changes (outside of 28/29) look like random fluctuations to me due to the
change in HcalGeometry which could affect GEN-SIM.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[AEx02ixvrCq1o_wlWfSlz8quHj038r7sks5tQHJvgaJpZM4RvvKq.gif]
|
|
Sorry for being too fast... Kevin, you're right, there are small SimHits changes visible in the plots linked above... |
|
Thank you @kpedro88 and @abdoulline for the explanations. I would have liked to check in the jenkins tests outputs of #21854 (the PR which supposedly exposed the bug fixed here) the change in shape witnessing the use of the collapsed scenario even for "standard" 2018 workflows. However, they already disappeared. Therefore, I checked with one of the "collapsed" workflows the same eta distribution above of the CaloTowers, and I can verify that the collapsed distribution That confirms the issue, and the efficacy of the fix applied here. |
|
+1
|
|
@prebello you had already signed it the pdmv part of the PR, so I understand that this is ok for you |
|
merge |
#21842 unintentionally introduced some discrepancies in the 2018 "collapsed" workflow. The underlying problem is that
HcalTopology::withSpecialRBXHBHE()is set by the underlying geometry, and since we use a geometry that allows collapsing for 2018, it is always true, even if the collapsing is not actually performed.Instead, from now on we will use
HcalTopology::getMergePositionFlag(), which can be set in Python. This parameter will be set by the same modifier that puts the RecHit combiner module into the HCAL local reco sequence.This change is propagated to all necessary areas of the code. @abdoulline has confirmed that it works as expected for CaloTowers (with thresholds removed, since sometimes there can be weird threshold effects for collapsed vs uncollapsed RecHits): https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/Validation/SVSuite/HCAL/calo_scan_single_pi/10_0_0/10_0_0_2018C_fixNocuts_vs_10_0_0_2018UC_fixNocuts_SinglePi/
@Martin-Grunewald for HLT, the parameter
process.HcalTopologyIdealEP.MergePositionshould be false by default, except for collapsed menus (2017 and 2018). What is the best way to specify this in the customization?This PR will be backported to 100X.
attn: @bsunanda