-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HCAL: switch off auxiliary M3 reconstruction fit for Run3 #35806
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35806/26169
|
A new Pull Request was created by @abdoulline (Salavat Abdullin) for master. It involves the following packages:
@jpata, @missirol, @cmsbuild, @Martin-Grunewald, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-986f2b/19870/summary.html Comparison SummarySummary:
|
@@ -2654,3 +2654,6 @@ | |||
PixelCPE = cms.string('PixelCPEGeneric'), | |||
StripCPE = cms.string('StripCPEfromTrackAngle') | |||
) | |||
|
|||
from Configuration.Eras.Modifier_run3_common_cff import run3_common | |||
run3_common.toModify(hltHbhereco, algorithm = dict(useM3 = False)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HLT does not use Eras and anway has useM3 = cms.bool( False )
so this is not needed for HLT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Martin-Grunewald thank you for calrification, I (naively) thought this might be a kind of standalone config used for some purposes. I'll remove this modification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I see that that file (PhaseII) actually sets useM3 = True which seems nonsensical but then it has to be switched to false after all. Apologies for the confusion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Martin-Grunewald just to avoid any (more) confusion: do you suggest to bring the (just removed) customization back ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but also I want Andrea Bocci @fwyzard to clarify why in that Phase-II file useM3 = True is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It (useM3=True) should in principle be harmless for HLT, it just takes unnecessary additional CPU time (~10% of MAHI in CPU case), as useMahi =True is set anyway and it means HCAL RecHits energy comes from MAHI, and M3 "just" fills an auxiliary energy word...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded Phase2 copy seems a copy of the offline Run2 sequence.
processQIE11 = cms.bool(True),
processQIE8 = cms.bool(True)
we will not have QIE8 in the Phase2
And these are very expensive CPU wise, in fact were mostly calculated offline in Run2 not at HLT.
setPulseShapeFlagsQIE8 = cms.bool(True),
setLegacyFlagsQIE8 = cms.bool(True)
setNegativeFlagsQIE8 = cms.bool(True),
setNoiseFlagsQIE8 = cms.bool(True),
setPulseShapeFlagsQIE8 = cms.bool(True),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mariadalfonso thanks for spotting these obsolete settings, I must admit I didn't even look at them, as I've been focused just on M3...
I suppose you mean that even if there is no QIE8 Digi collection in Phase2, setting all those modules (and then calling them for nothing) may take some CPU time?
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35806/26176
|
-1 Failed Tests: RelVals RelVals
|
...So, waiting for a new IB to come out for test(s)... |
@cmsbuild please test |
urgent
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-986f2b/19928/summary.html Comparison SummarySummary:
|
Worth noting that in the current HLT Phase-2 menu there are a couple of cases where |
+reconstruction
|
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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
M3 signal fit is an auxiliary one (MAHI is default), but it stays activated in HCAL Offline reconstruction (while swithed off since long time for HLT) and it wasn't explcitly looked at in the context of Run3 preparations and it's not used by any downstream consumers.
HCAL is now discussing to possibly use corresponding HBHERecHit member (auxEnergy) for saving next-to-trigger BX (SOI+1 in HCAL notations) energy from MAHI for LLP studies.
NB: this PR (once backported) serves as a workaround for recently reported issue #35785
while (in parallel) HCAL DPG figures out what went wrong with M3 as reported here:
https://hypernews.cern.ch/HyperNews/CMS/get/tier0-Ops/2294.html
PR validation:
runTheMatrix.py -l limited