Adding "safe-guards" to avoid segfault in the GaussHitFinderSBN module#659
Adding "safe-guards" to avoid segfault in the GaussHitFinderSBN module#659mattiasotgia wants to merge 5 commits into
Conversation
|
Adding @linyan-w as reviewer for SBND, adding this to SBND 2025 Gen2 Production |
cerati
left a comment
There was a problem hiding this comment.
Please remove the " // THIS IS NOT USED" lines? or if there is a specific need for them, please explain. Everything else seems fine so I will approve in the mean time
|
Hi @cerati I addressed your comment. However I wanted to highlight that the comment was present since even if the hit filtering is enabled, filtered hits are not saved (since the lines 645--649 are commented out), so maybe we might need to check how this might be enabled correctly, to align it to the |
PetrilloAtWork
left a comment
There was a problem hiding this comment.
A couple of comments to help documentation and future interpretation.
Technically it looks good to me.
|
I remember the gotchas addressed by points 2 and 3.. The changes look good to me, approved! Thanks @mattiasotgia :) |
|
Addressed @PetrilloAtWork's comments (thank you 🙏🏾). I've added a couple of lines regarding this issue (and a reference to the presentation where the issue is presented, SBN DocDB-47092 |
SFBayLaser
left a comment
There was a problem hiding this comment.
I'm happy to approve, have seen multiple presentations on this, the code changes are consistent with those presentations and, finally, it looks like Mattia has addressed other reviewer requests.
PetrilloAtWork
left a comment
There was a problem hiding this comment.
Thank you, @mattiasotgia.
The issue
The introduction of the
NextADCThresholdparameter in LArSoft/larreco#92 highlighted some minor bugs in theGaussHitFinderSBNmodule that made the code segfault for some edge case.This PR aims at adding some safeguards to the code preventing the segfault to happen.
Description
The changes happens in three points of the code
peakFitterToolreturns the peak parameters for each "candidate hit". Here we added a check for all "not finite" bounds, whereas the old version of the code was only checking for the positive side. Since the