-
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
fix GEM inputs for L1REPACK:FullMC
#40461
fix GEM inputs for L1REPACK:FullMC
#40461
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40461/33624
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages:
@cmsbuild, @perrotta, @rappoccio, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
assign l1 |
New categories assigned: l1 @epalencia,@rekovic,@cecilecaillol you have been requested to review this Pull request/Issue and eventually sign? Thanks |
please test @cms-sw/l1-l2 , is there any wf testing |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1dd99c/29854/summary.html Comparison SummarySummary:
|
@cms-sw/l1-l2 Could you please have a look, and check if this fix is correct? If so, it would be useful to have this in |
+l1 |
type bugfix |
@@ -60,6 +67,8 @@ def _print(ignored): | |||
# CSC TPs | |||
simCscTriggerPrimitiveDigis.CSCComparatorDigiProducer = 'unpackCSC:MuonCSCComparatorDigi' | |||
simCscTriggerPrimitiveDigis.CSCWireDigiProducer = 'unpackCSC:MuonCSCWireDigi' | |||
# GEM | |||
run3_GEM.toModify(simMuonGEMPadDigis, InputCollection = 'unpackGEM') |
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.
This is the same as what currently in SimL1EmulatorRepack_Full_cff.py, therefore I imagine it may work and my question is probably silly...
But what happens if run3_GEM
and not stage2L1Trigger
? 'unpackGEM' is feed as InputCollection to simMuonGEMPadDigis
[1], but the SimL1EmulatorTask
is not updates with the unpackGEM
module in it [2]
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.
Well, I can only guess as a non-L1T expert.
- This file only really works for
stage2L1Trigger
, according to the comments up top ("only supports Stage 2 eras for now."), modulo the fact that the comments are mistakenly swapped (the first comment, "use a legacy version", should show for~stage2L1Trigger
, not forstage2L1Trigger
) - In practise, I think
stage2L1Trigger
applies to nearly anything after 2015, so "run3_GEM
and notstage2L1Trigger
" should never happen (in principle..).
Let me fix the comments, since I would like to backport this..
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.
Thank you Marino, this is also my understanding.
However, even if so, the clean way to implement I imagine it should be
run3_GEM.toModify(simMuonGEMPadDigis, InputCollection = 'unpackGEM') | |
(stage2L1Trigger & run3_GEM).toModify(simMuonGEMPadDigis, InputCollection = 'unpackGEM') |
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.
Following what I see elsewhere, e.g.
(stage2L1Trigger & run3_GEM).toReplaceWith( SimL1TMuonTask, cms.Task(simMuonGEMPadTask,_run3_SimL1TMuonTask) ) |
I can change
run3_GEM
to (stage2L1Trigger & run3_GEM)
also above (and in "Full").
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.
However, even if so, the clean way to implement I imagine it should be
I cross-posted. Yes, will do (soon).
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40461/33642
|
7cb44b3
to
633c942
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40461/33643
|
Pull request #40461 was updated. @perrotta, @rappoccio, @epalencia, @cecilecaillol, @cmsbuild, @rekovic, @fabiocos, @davidlange6 can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1dd99c/29892/summary.html Comparison SummarySummary:
|
@cms-sw/l1-l2, could you please check again, and re-sign? Thanks! The diff of this PR since your previous signature is below: |
+l1 |
+1 |
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 be automatically merged. |
PR description:
It looks like
L1REPACK:FullMC
does not work as expected on EDM inputs without the DIGI data tier, as [1] returns [2].(This would suggest that, up to now, users were simply reading the GEM digis from the EDM inputs when running
L1REPACK:FullMC
.)This PR is a tentative fix (I'm not a domain expert). It is based on what is currently done for
L1REPACK:Full
(which is for real data, as opposed toL1REPACK:FullMC
which is for MC), e.g.cmssw/Configuration/StandardSequences/python/SimL1EmulatorRepack_Full_cff.py
Lines 168 to 170 in a8e4cca
(
L1REPACK:Full
was fixed in #36133, based on #36133 (comment))[1]
[2]
PR validation:
Nothing beyond the use case described in the PR description.
If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:
Plan to backport to (at least)12_6_X
to aid HLT studies.CMSSW_12_4_X
CMSSW_12_5_X
CMSSW_12_6_X