Project

General

Profile

Bug #25559

geo::WireGeo wire angle calculation

Added by Vyacheslav Galymov about 2 months ago. Updated about 2 months ago.

Status:
Closed
Priority:
Normal
Category:
Geometry
Target version:
-
Start date:
02/25/2021
Due date:
% Done:

100%

Estimated time:
Occurs In:
Experiment:
DUNE
Co-Assignees:
Duration:

Description

I have encountered an issue with the wire angle returned by WireGeo::ThetaZ() for horizontal wires (i.e., ThetaZ=0) while implementing a sorting wire algorithm for an instance of GeoObjectSorter. The value of ThetaZ appears to be NaN for some wires. In my sortWire method I put a print of the type:

if( w1.ThetaZ()!=w1.ThetaZ() ){
        cout<<"w1 " << w1.WireInfo("  ", 3)<<" half length " << w1.HalfL()<<endl;
      }

and this is an example for a problematic wire of the above print:
w1 wire from (325.03,-500.827,148.996) to (325.03,-500.827,297.892) (148.896 cm long), theta(z)=nan rad
    center at (325.03,-500.827,223.444) cm half length 74.448

From a quick look at the constructor of geo::WireGeo, the wire angle is calculated as

fThetaZ = std::acos((end.Z() - fCenter.Z())/fHalfL);

A likely culprit is the rounding errors in the argument of acos that causing it to go just above 1. Even if this is in a some insignificant decimal place it can still cause a NaN for the return.

Could one add something like an std::clamp function to limit range of the argument to +/-1 or some similar fix to avoid such a rounding issue popping up?

History

#1 Updated by Gianluca Petrillo about 2 months ago

I feel your analysis is right on the spot... although in ICARUS none of the 8416 horizontal wires ever showed that feat (it probably depends also on the amount of rotations needed to get there...).
Anyway, I have put in a larcorealg pull request #15 on the topic.
You can pull in feature/gp_issue25559 from my fork of larcorealg to see if that is solving the issue.

#2 Updated by Vyacheslav Galymov about 2 months ago

Yes, I can confirm this solved the issue.

#3 Updated by Kyle Knoepfel about 2 months ago

  • Assignee set to Gianluca Petrillo
  • Status changed from New to Assigned

Thanks, Gianluca. We will look at the PR and approve as appropriate.

#4 Updated by Lynn Garren about 2 months ago

larcorealg PR 15 is included in larsoft v09_17_02.

#5 Updated by Kyle Knoepfel about 2 months ago

  • % Done changed from 0 to 100
  • Status changed from Assigned to Closed

Also available in: Atom PDF