Skip to content
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

fix: add safeDivide to cylinderSurface pathCorrection #3254

Closed
wants to merge 3 commits into from

Conversation

AJPfleger
Copy link
Contributor

@AJPfleger AJPfleger commented Jun 5, 2024

I encountered some FPEs in during the navigation. This would avoid them and not crash the programme by giving a good estimate, in case we end up with 2 perpendicular vectors in double cosAlpha = normalT.dot(direction);

FPE

In gx2f physmon I got the following output:

15:26:16    Sequencer      INFO      -----------------------------------
15:26:16    Sequencer      INFO      FPE summary for Algorithm: TrackFittingAlgorithm
15:26:16    Sequencer      INFO      -----------------------------------
15:26:16    Sequencer      INFO      - FLTDIV: (3 times) 
 0# Acts::CylinderSurface::pathCorrection(Acts::ContextType const&, Eigen::Matrix<double, 3, 1, 0, 3, 1> const&, Eigen::Matrix<double, 3, 1, 0, 3, 1> const&) const at /builds/acts/ci-bridge/src/Core/src/Surfaces/CylinderSurface.cpp:174
 1# Acts::Layer::compatibleSurfaces(Acts::ContextType const&, Eigen::Matrix<double, 3, 1, 0, 3, 1> const&, Eigen::Matrix<double, 3, 1, 0, 3, 1> const&, Acts::NavigationOptions<Acts::Surface> const&) const at /builds/acts/ci-bridge/src/Core/src/Geometry/Layer.cpp:151
 2# bool Acts::Navigator::resolveSurfaces<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>::State<Acts::PropagatorOptions<Acts::ActionList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Actor<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> > >, Acts::AbortList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Aborter<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> >, Acts::PathLimitReached> >, Acts::Experimental::Gx2FitterResult<Acts::VectorMultiTrajectory> >, Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer> >(Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>::State<Acts::PropagatorOptions<Acts::ActionList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Actor<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> > >, Acts::AbortList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Aborter<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> >, Acts::PathLimitReached> >, Acts::Experimental::Gx2FitterResult<Acts::VectorMultiTrajectory> >&, Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer> const&) const [clone .isra.0] at /builds/acts/ci-bridge/src/Core/include/Acts/Propagator/Navigator.hpp:1055
 3# void Acts::Navigator::preStep<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>::State<Acts::PropagatorOptions<Acts::ActionList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Actor<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> > >, Acts::AbortList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Aborter<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> >, Acts::PathLimitReached> >, Acts::Experimental::Gx2FitterResult<Acts::VectorMultiTrajectory> >, Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer> >(Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>::State<Acts::PropagatorOptions<Acts::ActionList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Actor<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> > >, Acts::AbortList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Aborter<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> >, Acts::PathLimitReached> >, Acts::Experimental::Gx2FitterResult<Acts::VectorMultiTrajectory> >&, Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer> const&) const at /builds/acts/ci-bridge/src/Core/include/Acts/Propagator/Navigator.hpp:356
 4# Acts::Result<void, std::error_code> Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>::propagate<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>::State<Acts::PropagatorOptions<Acts::ActionList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Actor<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> > >, Acts::AbortList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Aborter<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> >, Acts::PathLimitReached> >, Acts::Experimental::Gx2FitterResult<Acts::VectorMultiTrajectory> > >(Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>::State<Acts::PropagatorOptions<Acts::ActionList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Actor<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> > >, Acts::AbortList<Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Aborter<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> >, Acts::PathLimitReached> >, Acts::Experimental::Gx2FitterResult<Acts::VectorMultiTrajectory> >&) const at /builds/acts/ci-bridge/src/Core/include/Acts/Propagator/Propagator.ipp:48
 5# std::enable_if<!(false), Acts::Result<Acts::TrackContainer<Acts::VectorTrackContainer, Acts::VectorMultiTrajectory, std::shared_ptr>::TrackProxy, std::error_code> >::type Acts::Experimental::Gx2Fitter<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::DefaultExtension>, Acts::detail::VoidAuctioneer>, Acts::Navigator>, Acts::VectorMultiTrajectory>::fit<__gnu_cxx::__normal_iterator<Acts::SourceLink const*, std::vector<Acts::SourceLink, std::allocator<Acts::SourceLink> > >, Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis>, Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis>, Acts::VectorTrackContainer, std::shared_ptr, false>(__gnu_cxx::__normal_iterator<Acts::SourceLink const*, std::vector<Acts::SourceLink, std::allocator<Acts::SourceLink> > >, __gnu_cxx::__normal_iterator<Acts::SourceLink const*, std::vector<Acts::SourceLink, std::allocator<Acts::SourceLink> > >, Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> const&, Acts::Experimental::Gx2FitterOptions<Acts::VectorMultiTrajectory> const&, Acts::TrackContainer<Acts::VectorTrackContainer, Acts::VectorMultiTrajectory, std::shared_ptr>&) const at /builds/acts/ci-bridge/src/Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpp:789
 6# (anonymous namespace)::GlobalChiSquareFitterFunctionImpl::operator()(std::vector<Acts::SourceLink, std::allocator<Acts::SourceLink> > const&, Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> const&, ActsExamples::TrackFitterFunction::GeneralFitterOptions const&, ActsExamples::MeasurementCalibratorAdapter const&, Acts::TrackContainer<Acts::VectorTrackContainer, Acts::VectorMultiTrajectory, std::shared_ptr>&) const at /builds/acts/ci-bridge/src/Examples/Algorithms/TrackFitting/src/GlobalChiSquareFitterFunction.cpp:110
 7# ActsExamples::TrackFittingAlgorithm::execute(ActsExamples::AlgorithmContext const&) const at /builds/acts/ci-bridge/src/Examples/Algorithms/TrackFitting/src/TrackFittingAlgorithm.cpp:148

One case had the following values before:
position:
164.63
-48.368
26.7465

normalT:
0.959448
-0.281884
0

direction:
-0.0299944
-0.102092
0.994323

cosAlpha: 0

blocked by:

@AJPfleger AJPfleger added the 🛑 blocked This item is blocked by another item label Jun 5, 2024
@AJPfleger AJPfleger added this to the next milestone Jun 5, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Jun 5, 2024
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 47.65%. Comparing base (122de33) to head (8fa208c).
Report is 286 commits behind head on main.

Files with missing lines Patch % Lines
Core/include/Acts/Utilities/AlgebraHelpers.hpp 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3254   +/-   ##
=======================================
  Coverage   47.65%   47.65%           
=======================================
  Files         507      507           
  Lines       29205    29212    +7     
  Branches    14010    14013    +3     
=======================================
+ Hits        13917    13922    +5     
- Misses       5264     5265    +1     
- Partials    10024    10025    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andiwand
Copy link
Contributor

andiwand commented Jun 6, 2024

I believe I saw similar problems in the past and they seemed rather related to navigation issues since a 90° incline should be almost impossible.

But apart from potential navigation problems - which should be ruled out first - we could have a bound on the path correction either where we apply it or directly in the surface function because the surface will never be infinitely thick.

@paulgessinger
Copy link
Member

paulgessinger commented Jun 6, 2024

I guess constraining cosAlpha would do the trick here, at least to first order.

@AJPfleger
Copy link
Contributor Author

Yes, that is what safeDivide does 😊

@andiwand
Copy link
Contributor

andiwand commented Jun 6, 2024

Indirectly, yes. But it is hiding the cut and the threshold is not tunable which is not applicable in all situations.

@AJPfleger
Copy link
Contributor Author

Closed in favour of a possible better solution and tracked here:

@AJPfleger AJPfleger closed this Jun 7, 2024
kodiakhq bot pushed a commit that referenced this pull request Jun 7, 2024
Have *almost* same settings for GX2F and KF makes it easier to understand in the physmon output and check if our results are good.

## Why *almost*?
Had also 3 FPEs which fixed here, but did not choose this approach:
- #3254

This issue only occurred for `2.5 < |eta| < 3.0`. To get better physmon until the problem is fixed (maybe in a later stage of the GX2F-development), the eta-range is reduced. The full eta-range could be used after solving:
- #3267
@andiwand andiwand modified the milestones: next, v35.2.0 Jun 16, 2024
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
…ct#3248)

Have *almost* same settings for GX2F and KF makes it easier to understand in the physmon output and check if our results are good.

## Why *almost*?
Had also 3 FPEs which fixed here, but did not choose this approach:
- acts-project#3254

This issue only occurred for `2.5 < |eta| < 3.0`. To get better physmon until the problem is fixed (maybe in a later stage of the GX2F-development), the eta-range is reduced. The full eta-range could be used after solving:
- acts-project#3267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module 🛑 blocked This item is blocked by another item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants