Segmentation fault in PulseCombiner::sumPulses caused by a hit with an exceptionally large time - #2789
Segmentation fault in PulseCombiner::sumPulses caused by a hit with an exceptionally large time#2789mhkim-anl wants to merge 9 commits into
Conversation
for more information, see https://pre-commit.ci
Capybara summary for PR 2789
Last updated 2026-08-10T20:34-04:00 f3416a7 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…n exceptionally large time (fix: iwyu) (#2793) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/30235625195. Please merge this PR into the branch `2788-segmentation-fault-in-pulsecombiner` to resolve failures in PR #2789. Auto-generated by [create-pull-request][1] [1]: https://github.com/peter-evans/create-pull-request Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
No Clang-Tidy warnings found so I assume my comments were addressed
ruse-traveler
left a comment
ruse-traveler
left a comment
There was a problem hiding this comment.
Thanks for the fix, @mhkim-anl! I think @ssedd1123 and @wdconinc's comments are good to consider, but otherwise think this is good to go!
…std::size_t for sizes
|
Sorry for the late updates to the remaining comments, and it caused the previous approval to be dismissed. I think I've now addressed all the remaining comments. If everything looks good, I would appreciate it if you could approve this PR. Thanks! |
wdconinc
left a comment
wdconinc
left a comment
There was a problem hiding this comment.
I looked again and the duplication of the loop in sumPulses struck me. Can we not avoid that explicit loop and at least make the first one more computationally efficient and avoid function calls?
Then I looked a bit further outside the immediate context and noticed a lot of pulse copying. Some of this is maybe inevitable (but I'd think you can avoid all copies here). Can we at least get the functions changes to call-by-reference? Each time we copy a pulse, it's not just a single number but a varying sequence of numbers. You don't seem to modify any of these pulses until the very end when you sum them.
| std::map<uint64_t, std::vector<PulseType>> cell_pulses; | ||
| for (const PulseType& pulse : *inPulses) { | ||
| uint64_t shiftedCellID = pulse.getCellID() & m_detector_bitmask; | ||
| cell_pulses[shiftedCellID].push_back(pulse); |
There was a problem hiding this comment.
We make a copy of all pulses.
| outPulses->push_back(pulses.at(0).clone()); | ||
| debug("CellID {} has only one pulse, no combination needed", cellID); | ||
| } else { | ||
| std::vector<std::vector<PulseType>> clusters = clusterPulses(pulses); |
There was a problem hiding this comment.
We make a copy of all pulses (call by value, return by value (but probably tail call eliminated)).
| sum_pulse.setInterval(cluster[0].getInterval()); | ||
| sum_pulse.setTime(cluster[0].getTime()); | ||
|
|
||
| auto newPulse = sumPulses(cluster); |
There was a problem hiding this comment.
We make a copy of all pulses (call by value, return by value (but probably tail call eliminated)).
| PulseCombiner::clusterPulses(const std::vector<PulseType> pulses) const { | ||
|
|
||
| // Clone the pulses array of pointers so they aren't const | ||
| std::vector<PulseType> ordered_pulses{pulses}; |
There was a problem hiding this comment.
We make a copy of all pulses.
| std::size_t maxStep = 0; | ||
| for (const auto& pulse : pulses) { | ||
| auto startStep = static_cast<std::size_t>( | ||
| std::round((pulse.getTime() - pulses[0].getTime()) / pulses[0].getInterval())); |
There was a problem hiding this comment.
There is an implicit assumption for the correct operation of this function: that pulses is time-ordered (and has at least one entry). If that assumption is not guaranteed (in this case two frames away) this will start to return negative time differences. Can you make this a bit clearer in the name of this function, its documentation, and the calling context?
Also, we calculate (pulse.getTime() - pulses[0].getTime()) / pulses[0].getInterval() in two identical loops. In the first loop, it seems that all you calculate is actually:
std::ranges::max(pulses, [&int=pulses[0].getInterval()](auto a, auto b) {
return a.getTime() + a.getAmplitude().size()*int < b.getTime() + b.getAmplitude().size()*int;
});
(where the std::ranges::max does not do worse than what you do, but is arguably clearer).
6 participants
Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.
In rare cases, a BIC hit has an exceptionally large time value (> 10^10). In
PulseCombiner::sumPulses,maxStepis calculated usingfloatprecision, and this large time value causesmaxStepto be calculated as 0, resulting in a segmentation fault. This PR is to address the above problem.Actually, it is unusual for some BIC hits to have such large time values. These are likely hits that should be discarded. I will investigate why they are produced, and, if necessary, introduce a time threshold in the
SimCalorimeterHitProcessoralgorithm.What is the urgency of this PR?
What kind of change does this PR introduce?
Please check if any of the following apply