fix: Fix trapezoid accelerated inside check

[andiwand]

This bug flagged some points to be inside while they are not. I fixed this by checking against two rectangles instead of just one which is too big.

discovered in #2694

[codecov]

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (fd1e0b4) 49.63% compared to head (a6e2432) 49.64%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
...e/include/Acts/TrackFitting/BetheHeitlerApprox.hpp	57.14%	4 Missing and 2 partials ⚠️
Core/include/Acts/TrackFitting/detail/GsfActor.hpp	16.66%	2 Missing and 3 partials ⚠️
...re/include/Acts/TrackFitting/GaussianSumFitter.hpp	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2696      +/-   ##
==========================================
+ Coverage   49.63%   49.64%   +0.01%     
==========================================
  Files         474      474              
  Lines       26941    26951      +10     
  Branches    12415    12416       +1     
==========================================
+ Hits        13372    13381       +9     
- Misses       4746     4747       +1     
  Partials     8823     8823              

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

[github-actions]

📊: Physics performance monitoring for a6e2432

Full contents

physmon summary

• ✅ CKF truth_smeared
• ✅ IVF truth_smeared
• ✅ AMVF truth_smeared
• ✅ Track Summary CKF truth_smeared
• ✅ Seeding truth_estimated
• ✅ CKF truth_estimated
• ✅ IVF truth_estimated
• ✅ AMVF truth_estimated
• ✅ Track Summary CKF truth_estimated
• ✅ Seeding seeded
• ✅ CKF seeded
• ✅ IVF seeded
• ✅ AMVF seeded
• ✅ AMVF (+grid seeder) seeded
• ✅ Track Summary CKF seeded
• ✅ Seeding orthogonal
• ✅ CKF orthogonal
• ✅ IVF orthogonal
• ✅ AMVF orthogonal
• ✅ Track Summary CKF orthogonal
• ✅ Ambisolver seeded
• ✅ Ambisolver orthogonal
• ✅ Seeding ttbar
• ✅ CKF ttbar
• ✅ Ambisolver
• ✅ Track Summary CKF ttbar
• ✅ AMVF ttbar
• ✅ AMVF (+grid seeder) ttbar
• ✅ Truth tracking (GSF)
• ✅ Truth tracking
• ✅ Particles inital fatras
• ✅ Particles final fatras
• ✅ Particles inital geant4
• ✅ Particles final geant4

[paulgessinger]

This surprises me. The way I read the new code it seems to me like it's the same shape but with extra redundant checks. Can you draw a sketch of which points it was previously missing?

[paulgessinger]

Here are some MC tests from the current main bounds implementation for a trap like this:

Acts::TrapezoidBounds bounds{halfXnegY: 20, halfXposY: 30, halfY: 20};

This looks correct to me? Am I missing something?

[andiwand]

@paulgessinger this test https://github.com/acts-project/acts/blob/main/Tests/UnitTests/Core/Surfaces/TrapezoidBoundsTests.cpp#L203 failed after I changed the RNG in #2694

The reason is that we previously extended the inner rectangle in X and Y direction using tolerance without special treatment of the corners. Not sure what we expect at the corners but the behavior is different in the polygon check and in the quick bounds check

looking at your MC generated image the problem are the corners where we either just inflate the trapezoid or like in your picture we have rectangles overshooting the inflated trapezoid

[paulgessinger]

Hm, I think that's what we want. By convention, the tolerances are aligned with the local axes not with the slanted edge of the trap.

I need to double check how the polygon code uses the tolerances.

[andiwand]

the polygon code will make the trapezoid bigger in x and y so there are no additional edges which seems "more correct" to me

[paulgessinger]

@andiwand Hm I see. I think the polygon code calculates the minimum distance first and then determines if they're inside tolerance in x and y.

Ultimately it's a choice. I think the current behavior is least surprising given "dumb" tolerances but that's just me.

Btw, with this PR included, the covered shape by the trap with tolerances seems to be exactly unchanged compared to before:

If this is correct, maybe the unit test failure is because of a floating point edge case and not because of a geometric difference?

[andiwand]

hmm I see. now I am confused. I don't think the numbers where small enough for a FP issue but let me check again

this is the error I see in #2694 (reverted the fix there)

/home/andreas/cern/source/acts/acts/git2/Tests/UnitTests/Core/Surfaces/TrapezoidBoundsTests.cpp(204): error: in "Surfaces/TrapezoidInsideCheck/_3459": check bc.isInside({x, y}, vertices) == trapezoidBoundsObject.inside({x, y}, bc) has failed [false != true]
Failure occurred in a following context:
    x = -1.2921219286961447; y = -2.2747078076516623; index = 864; tol = 0.29999999999999999; 

*** 1 failure is detected in the test module "ActsUnitTestTrapezoidBounds"

[paulgessinger]

@andiwand Hm, the way the test is written (full polygon vs trap inside method) I think your diagnosis should be correct, in that it's the ears of the trap that are just different in the two. 😵‍💫
Especially since the changes in this PR seem to have fixed the unit test failure.

[andiwand]

running on higher stats shows that this happens very rarely and only in one of the corners. happened 52 / 1000000 failures.txt

my code is basically delegating the corners to the polygon code this is why the edge case disappears. maybe there is a problem in the polygon code after all?

[paulgessinger]

If it's only in one of the corners maybe it's just < vs <= or something like that?

[andiwand]

it seems to be a rare numerical problem with the polygon method. I moved not the seed in #2694 so it does not fail

not sure if it is worth trying to make the polygon method more numerical stable

closing for now