-
Notifications
You must be signed in to change notification settings - Fork 35
Fixes two segfaults in CAF processing of SBND v10_06_00_09. #609
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
Fixes two segfaults in CAF processing of SBND v10_06_00_09. #609
Conversation
First fix: Added guard in G4 weight calculator against trajectory points outside world volume. Second fix: Added guard in RecoUtils.cc for when a trajectory does not intersect detector walls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @kjplows !
The updates look good to me.
I cannot see any issue with updates inGetWallCross to FillTrue.cxx's FillTrueG4Particle.
PetrilloAtWork
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend to remove the redundant duplication in the second change.
I also mention for your consideration the possibility to fix the first issue by computing in a loop intersection_i, something like:
if (intersections.empty()) return caf::WallNone; // no intersection
// get the intersection point closest to p0
TVector3 closestIntersection;
double minDistance2 = std::numeric_limits<double>::max();
for (auto const& point: intersections) {
double const d2 = (point - p0).Mag2();
if (d2 > minDistance2) continue;
minDistance2 = d2;
closestIntersection = point;
}This saves you for having to take decisions.
I save my usual rant against TVector3 for better times.
PetrilloAtWork
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes!
|
trigger build ci_ref=v10_06_00_06 SBNSoftware/sbndcode@v10_06_00_09 SBNSoftware/sbn*@release/SBN2025A LArSoft/larsoft@LARSOFT_SUITE_v10_06_00_02 LArSoft/larwirecell@LARSOFT_SUITE_v10_06_00_02 LArSoft/lar*@LARSOFT_SUITE_v10_06_00 |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
🚨 For more details about the warning phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
Description
During reprocessing of the SBND Spring BNB+cosmics (prodgenie) MC, a portion of cafmaker jobs failed, reproducibly.
These were tracked down to the following two (as of the writing of this PR) unrelated bugs:
Geant4WeightCalc::GetWeight-- there are particles in the MC whose coordinates exit out of the world volume. As an example, tracked down with gdb, a neutron exited out of bounds at negative Y (see below output). This causes the geometry service to return a null pointer for the material associated with that trajectory point, and segfaults immediately after.Fix: Added guard in G4 weight calculator against trajectory points outside world volume.
This does not affect calculations, as points that do not correspond to
LArmaterial do not currently get weighted.Summoning @jzennamo for review.
sbn::GetWallCross-- the calculation assumes there are always two intersections of a line connecting two points and a detector bounding box. This is untrue if one or both of the points are outside the bounding box.There is an
assert( intersections.size() == 2 );statement that does not fire, leading to a segfault on the call forintersections[0].See the attached image, from an example tracked down with gdb --
p0, p1have coordinates(198.71, 178.94, 518.58)and(197.87, 180.60, 518.98)cm respectively, compared to the detector bounding box defined by[-201.30, 201.30] x [-203.73, 203.73] x [0, 509.40]cm^3.(The line misses the detector by about 7 cm..)
Fix: Added switch statement to explicitly return
caf::kWallNoneif zero points. As a bonus, if one intersection, pad the vector with a "second point" same as the first, to make the calculation work. And if somehow the points both lie on an edge of the detector, just use the first two points.Summoning @sungbinoh for review. In the upcoming develop PR, this fix will be located in
FillTrue.cxx.As this PR does affect CAFs (positively, I should hope!), I am summoning @PetrilloAtWork for review.
Here are some turkey emojis, to help my case for this PR: 🦃🦃🦃
See also #610.