Erase from correct vector in PATTauHybridProducer #44067
Conversation
Erasing an element from pfChs using an iterator from pfGammas is undefined behavior. Based on the surrounding code, I guess the intention was to erase the element from pfGammas.
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44067/39019
|
|
A new Pull Request was created by @makortel for master. It involves the following packages:
@jfernan2, @mandrenguyen, @hqucms, @vlimant, @cmsbuild can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
Thanks @makortel for this fix, it was indeed the intention. |
|
-1 Failed Tests: UnitTests The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Unit TestsI found 2 errors in the following unit tests: ---> test TestL1ScoutingFormat had ERRORS ---> test TestSDSRawDataCollectionFormat had ERRORS Comparison SummarySummary:
|
|
Looks like better to wait for the next IB before restarting tests |
|
Comparisons show only the usual
The differences in 25034.999 seem to be caused by #44051 that was included in the tests (because of being merged already) |
|
enable nano |
|
@cmsbuild, please test |
|
Another thing I noticed in this code is that the loops like for (CandPtrs::iterator it = pfGammas.begin(); it != pfGammas.end();) {
if ((*it)->pt() < 0.5) {
it = pfGammas.erase(it);
} else {
++it;
}
}have pfGammas.erase(std::remove_if(pfGammas.begin(), pfGammas.end(), [](auto const& cand) {
return cand->pt() < 0.5;
}, pfGammas.end());(with C++20 this would become simpler, but we're not there yet...) |
|
@makortel will you apply this "linearization" of complexity or an action is needed from my side? |
|
@mbluj I wasn't planning to (even if I was a bit tempted), so it would be great if you (or someone else) could do that. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7797a2/37694/summary.html Comparison SummarySummary:
NANO Comparison SummarySummary:
Nano size comparison Summary:
|
|
+1 |
|
+1 |
cmsbuild
added
reconstruction-approved
fully-signed
and removed
reconstruction-pending
pending-signatures
labels
|
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. @antoniovilela, @rappoccio, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
The 14_0_X backport is in #44272 |
PR description:
Erasing an element from
pfChsusing an iterator frompfGammasis undefined behavior. Based on the surrounding code, I guess the intention was to erase the element frompfGammas.I came across this while investigating an UBSAN warning
(e.g. in https://cmssdt.cern.ch/SDT/cgi-bin/logreader/el8_amd64_gcc12/CMSSW_14_1_UBSAN_X_2024-02-21-2300/pyRelValMatrixLogs/run/11602.0_SingleElectronPt35+2021/step3_SingleElectronPt35+2021.log#/)
I'm not fully certain if this PR would fix that warning, but this line should be fixed anyway.
PR validation:
None
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:
The fix impacts physics, but I don't know by how much, nor if it should be backported. I can backport it if needed.