-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
re-initialize geometry in g4e propagator as required to avoid segfaults #40543
Conversation
…propagation and for each step
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40543/33774
|
A new Pull Request was created by @bendavid (Josh Bendavid) for master. It involves the following packages:
@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild, please test |
@bendavid , thanks for the fix! Would it be possible to backport to 12_4_X? may be also it should be done for 10_6_X. |
-1 Failed Tests: RelVals-INPUT RelVals-INPUTThe relvals timed out after 4 hours. Comparison SummarySummary:
|
please test problem seems to be in timeout |
type bug-fix |
-1 Failed Tests: RelVals-INPUT RelVals-INPUTThe relvals timed out after 4 hours. Comparison SummarySummary:
|
please test
|
@mmusich I wouldn't bother so much about those RelVal-Input timeouts, unless you need anything tested exlicitely by them |
no, actually the only meaningful tests are the ones in the unit tests, so we already know that these passed (additionally I also tested this privately positively). So I guess it's up to @civanch to sign even if tests are incomplete. |
+1 this PR includes fixs of Geant4e tracking - re-initialisation of track touchables on each geometry boundary is useful, here this re-initialisation is done on each step. This was tested on Run-2 data and provide good results. |
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
please abort |
+1 |
merge
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6014d0/30043/summary.html Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
|
PR description:
Adds necessary re-initialization of the geometry navigation to the G4e propagator to avoid seg-faults/undefined behaviour.
This should fix the underlying cause of #31920
PR validation:
These initialization changes were verified to prevent segfaults in a customized version of the propagator in CMSSW_10_6_X in the context of muon calibration for mW.
For this PR directly, only checked that TrackPropagation/Geant4e/test/simpleTestPropagator_cfg.py still runs. More direct tests should probably be done reproducing the segfault and verifying this fixes it in 13_0_X with the standard g4e propagator and track refit workflow.