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 to KineParticleFilter::acceptParticle #33088
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33088/21411
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages: FastSimulation/Event @ssekmen, @lveldere, @civanch, @mdhildreth, @cmsbuild, @sbein 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-fce5a2/13314/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
The very few differences in the comparisons don't seem (to me) related to the update in this PR. |
A kind ping for the review of this PR. This fix is needed for Run-3 JME studies. |
@sbein, it seems the PR is fine. |
ping @cms-sw/fastsim-l2 |
+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 now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
@missirol thank you very much for tracking this down. To me the change correctly fixes the issue and as you point out fixes a long-standing problem seen in JME. What I'm trying to understand is why this did not mess anything up for normal physics - why only JME... anyways, it is accepted so thanks again. |
Thanks @sbein , and others, for having a look.
That'd be interesting for know, in case you find more information. My rough understanding is that |
ok I will keep an eye out if there are other examples. TY |
PR description:
This PR is meant to fix a problem in a Jet/MET analysis workflow.
The current procedure to derive the so-called PF-Hadron Calibrations (for Offline and HLT) relies on the
PFSimParticleProducer
, which in turn makes use ofPFSimBaseEvent
and related utilities.For a non-negligible amount of events,
PFSimParticleProducer
crashes with the following error messageThis error was already reported in previous years, apparently without a clear solution:
https://indico.cern.ch/event/673246/contributions/2757878/attachments/1541980/2418599/JMEHLTX-POG.pdf
https://hypernews.cern.ch/HyperNews/CMS/get/eflow/1011/1/1.html
https://hypernews.cern.ch/HyperNews/CMS/get/famos/687.html
Some debugging [*] led to the update in this PR, which fixes the issue.
FYI: @lathomas @kirschen @hatakeyamak @bendavid @pallabidas
[*] More debugging information.
The crash occurs in
FBaseSimEvent
, and was caused by instances whereKineParticleFilter::acceptVertex
would fail (returnfalse
)cmssw/FastSimulation/Event/src/KineParticleFilter.cc
Line 59 in 03dd22b
and this would cause the vertex-id in
cmssw/FastSimulation/Event/src/FBaseSimEvent.cc
Line 178 in 03dd22b
to be
-1
cmssw/FastSimulation/Event/src/FBaseSimEvent.cc
Line 529 in 03dd22b
in the ensuing call to
FBaseSimEvent::addSimTrack
cmssw/FastSimulation/Event/src/FBaseSimEvent.cc
Line 199 in 03dd22b
KineParticleFilter::acceptParticle
would returntrue
cmssw/FastSimulation/Event/src/FBaseSimEvent.cc
Line 487 in 03dd22b
because of
cmssw/FastSimulation/Event/src/KineParticleFilter.cc
Line 37 in 03dd22b
and, ultimately, the crash would occur here, because of
iv
being-1
cmssw/FastSimulation/Event/src/FBaseSimEvent.cc
Line 498 in 03dd22b
In summary, it seems that, when
KineParticleFilter::acceptVertex
returnsfalse
, thenKineParticleFilter::acceptParticle
should also returnfalse
for particles associated to that vertex. When this is not the case (i.e. without the suggested fix), the logic inFBaseSimEvent
starts to break.PR validation:
Private tests on the affected JME workflow.
if this PR is a backport please specify the original PR and why you need to backport that PR:
N/A (no backport planned)