acts icon indicating copy to clipboard operation
acts copied to clipboard

fix: protect TrackSelector against eta=nan

Open timadye opened this issue 1 year ago • 5 comments

Protect against η=nan in Acts::TrackSelector::isValidTrack(), which would throw std::invalid_argument{"Eta is outside the abs eta bin edges"} exception. This can come about if somehow the track θ<0 or θ>π. We should try to prevent these out-of-range values, but having them shouldn't raise an exception.

timadye avatar Oct 25 '24 16:10 timadye

📊: Physics performance monitoring for 42b5486ad419242793efbb83e53fecec00d52cff

Full contents

physmon summary

github-actions[bot] avatar Oct 25 '24 17:10 github-actions[bot]

I wonder if the nan is bound to an earlier FPE and we should rather fix the root cause. Something like that might help https://github.com/acts-project/acts/pull/3756/files#diff-c0c115f048fe0bf709beaf5d8268193cb06dcadcda1819c7722696393e4e2df2R49

I have this helper https://github.com/acts-project/acts/blob/645a8d1fb9599c780c927aab6f231477c3236e8f/Core/include/Acts/Utilities/AngleHelpers.hpp#L15-L23 and would like to make it FPE safe. In that case it could also fix the problem you see at its root.

andiwand avatar Oct 25 '24 19:10 andiwand

I wonder if the nan is bound to an earlier FPE and we should rather fix the root cause.

I don't know if FPEs come from std::log(-ve) giving nan. In the Athena job, I am seeing ~1 FPE (WARNING FPE INVALID in [Execute] of [ActsToTrkConvertorAlg] on event ...) an event, but not for the event that throwed the exception.

#3756 sounds like an excellent idea. I will take a look in detail. Even so, I think we should be protected against nans in this code, as users can pass what they like.

timadye avatar Oct 25 '24 21:10 timadye

I think nan inputs we see as a user responsibility but let's also pull in @paulgessinger for this. I would hope that an FPE pops up if a nan is produced in some way

andiwand avatar Oct 26 '24 07:10 andiwand

:white_check_mark: Athena integration test results

:white_check_mark: All tests successful

status job report
:green_circle: report_pull_request
:green_circle: run_art_test: test_run4_acts_vertex_PU200
:green_circle: run_art_test: test_run4_ttbar_PU200
:green_circle: run_art_test: test_data18_13TeV_1000evt
:green_circle: run_art_test: test_ttbarPU40_reco

acts-project-service avatar Oct 30 '24 12:10 acts-project-service

Somehow this PR failed physmon on GitLab after having been merged. But is was green here, so it might be a fluke?

paulgessinger avatar Oct 30 '24 12:10 paulgessinger

Somehow #3794's changes were included here and removed from that PR. No harm done.

timadye avatar Nov 01 '24 18:11 timadye