fix!: Fix vertex finding for seeds with `z=0` #2917

andiwand · 2024-01-31T15:57:53Z

While working on the 4D vertexing performance I realized that our vertex finders will abort if a seed with z=0 is found. This can happen using a grid based seeder as it can round to exactly 0.

As far as I could see the vertex constraint was only used to signal the finish line for the vertex finding. I exchanged that with an optional and also removed the vector which had no use as far as I could tell

pbutti

Looks reasonable to me

Invalidated by push of 9d85a36

github-actions · 2024-02-01T10:23:03Z

📊: Physics performance monitoring for `bf674c6`

Full contents

physmon summary

felix-russo

Very good idea; I don't know why we did this but I am guessing that it is a relict from athena.

Do you know why the physmon changes for the IVF? I don't see why it should
Is this breaking because of the (very sensible) renaming?

Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp

Tests/UnitTests/Core/Vertexing/IterativeVertexFinderTests.cpp

andiwand · 2024-02-03T12:22:09Z

Very good idea; I don't know why we did this but I am guessing that it is a relict from athena.

Do you know why the physmon changes for the IVF? I don't see why it should

Is this breaking because of the (very sensible) renaming?

I am also a bit puzzled about the performance changes in the IVF. I can try to track it down

Yeah I think flagging as breaking makes a lot of sense here

codecov · 2024-02-05T18:03:57Z

Codecov Report

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

Comparison is base (8ccefe1) 48.78% compared to head (bf674c6) 48.78%.

Files	Patch %	Lines
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp	43.75%	1 Missing and 8 partials ⚠️
...e/include/Acts/Vertexing/IterativeVertexFinder.ipp	50.00%	1 Missing and 3 partials ⚠️
...Acts/Vertexing/AdaptiveGridDensityVertexFinder.ipp	0.00%	1 Missing and 1 partial ⚠️
...include/Acts/Vertexing/GridDensityVertexFinder.ipp	50.00%	0 Missing and 1 partial ⚠️
...nclude/Acts/Vertexing/TrackDensityVertexFinder.ipp	0.00%	0 Missing and 1 partial ⚠️
Core/include/Acts/Vertexing/ZScanVertexFinder.ipp	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2917   +/-   ##
=======================================
  Coverage   48.78%   48.78%           
=======================================
  Files         493      493           
  Lines       28911    28905    -6     
  Branches    13749    13747    -2     
=======================================
- Hits        14104    14102    -2     
+ Misses       4913     4911    -2     
+ Partials     9894     9892    -2

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

andiwand · 2024-02-05T19:42:47Z

the physmon regression is actually coming from an existing problem. if a single propagation during vertex fitting fails the whole event will have 0 vertices. the propagation to the perigee is not stable as the distance estimate can be quite a bit off and our overstepping tolerance will ruin the propagation. this happens more frequently than before because more vertices are reconstructed

I added a fix here #2930

The AMVF config `addSingleTrackVertices` was not correctly applied in the finding process and endet up always being `true`. This PR fixes the behavior. discovered in #2917

The AMVF config `addSingleTrackVertices` was not correctly applied in the finding process and endet up always being `true`. This PR fixes the behavior. discovered in acts-project#2917

andiwand · 2024-02-13T12:55:37Z

blocked by the vertexing refactor atm

andiwand · 2024-02-20T09:01:05Z

I decoupled this one now from #2930 and pulled the vertex refactor changes in. Ready to go from my side

paulgessinger

Seems reasonable, I only have nitpicks.

Output change is likely due to extra vertices being found?

Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp

Core/include/Acts/Vertexing/AdaptiveGridDensityVertexFinder.ipp

Core/include/Acts/Vertexing/GridDensityVertexFinder.ipp

Core/include/Acts/Vertexing/IterativeVertexFinder.ipp

Core/include/Acts/Vertexing/TrackDensityVertexFinder.ipp

Core/include/Acts/Vertexing/ZScanVertexFinder.ipp

Co-authored-by: Paul Gessinger <hello@paulgessinger.com>

acts-project-service · 2024-02-21T12:31:59Z

🔴 Athena integration test results [`123746c`]

Build job with this PR failed!

Please investigate the build job for the pipeline!

)" This reverts commit 123746c.

paulgessinger · 2024-02-21T17:19:26Z

This PR changes Athena outputs

andiwand · 2024-02-21T17:40:31Z

Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp

The code shuffle here might have unexpected effects. I will revert and see if that is it.

Otherwise I can only suspect that the number of vertices is changing. If so I would think it is a bug fix but we have to check the plots.

)" This reverts commit 123746c.

…roject#2917)"" This reverts commit dbdc5a3.

andiwand · 2024-02-29T15:26:50Z

#3002 will hopefully resolve this

(#3002) In #2917 I changed the way how "no more seeds" is communicated in our vertexing code but forgot to wire this for the `TrackDensityVertexFinder` correctly.

…ts-project#2917 (acts-project#3002) In acts-project#2917 I changed the way how "no more seeds" is communicated in our vertexing code but forgot to wire this for the `TrackDensityVertexFinder` correctly.

This fixes another case where the overstepping tolerance bites us from behind. Perigee surfaces are prone to the issue of bad path length estimation so we need a special treatment. discovered in #2917

…ject#2930) This fixes another case where the overstepping tolerance bites us from behind. Perigee surfaces are prone to the issue of bad path length estimation so we need a special treatment. discovered in acts-project#2917

The AMVF config `addSingleTrackVertices` was not correctly applied in the finding process and endet up always being `true`. This PR fixes the behavior. discovered in acts-project#2917

While working on the 4D vertexing performance I realized that our vertex finders will abort if a seed with `z=0` is found. This can happen using a grid based seeder as it can round to exactly 0. As far as I could see the vertex constraint was only used to signal the finish line for the vertex finding. I exchanged that with an optional and also removed the vector which had no use as far as I could tell

…ts-project#2917 (acts-project#3002) In acts-project#2917 I changed the way how "no more seeds" is communicated in our vertexing code but forgot to wire this for the `TrackDensityVertexFinder` correctly.

…ject#2930) This fixes another case where the overstepping tolerance bites us from behind. Perigee surfaces are prone to the issue of bad path length estimation so we need a special treatment. discovered in acts-project#2917

The AMVF config `addSingleTrackVertices` was not correctly applied in the finding process and endet up always being `true`. This PR fixes the behavior. discovered in acts-project#2917

While working on the 4D vertexing performance I realized that our vertex finders will abort if a seed with `z=0` is found. This can happen using a grid based seeder as it can round to exactly 0. As far as I could see the vertex constraint was only used to signal the finish line for the vertex finding. I exchanged that with an optional and also removed the vector which had no use as far as I could tell

…ts-project#2917 (acts-project#3002) In acts-project#2917 I changed the way how "no more seeds" is communicated in our vertexing code but forgot to wire this for the `TrackDensityVertexFinder` correctly.

…ject#2930) This fixes another case where the overstepping tolerance bites us from behind. Perigee surfaces are prone to the issue of bad path length estimation so we need a special treatment. discovered in acts-project#2917

andiwand added this to the next milestone Jan 31, 2024

andiwand requested review from pbutti and felix-russo January 31, 2024 15:57

github-actions bot added Component - Core Affects the Core module Vertexing labels Jan 31, 2024

pbutti reviewed Jan 31, 2024

View reviewed changes

pbutti previously approved these changes Jan 31, 2024

View reviewed changes

felix-russo reviewed Feb 3, 2024

View reviewed changes

Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp Outdated Show resolved Hide resolved

Tests/UnitTests/Core/Vertexing/IterativeVertexFinderTests.cpp Outdated Show resolved Hide resolved

andiwand changed the title ~~fix: Fix vertex finding for seeds with z=0~~ fix!: Fix vertex finding for seeds with z=0 Feb 3, 2024

andiwand mentioned this pull request Feb 5, 2024

fix: Robust perigee propagation in HelicalTrackLinearizer #2930

Merged

andiwand mentioned this pull request Feb 5, 2024

fix: Fix AMVF find single track vertices #2931

Merged

andiwand added the 🛑 blocked This item is blocked by another item label Feb 13, 2024

andiwand removed the 🛑 blocked This item is blocked by another item label Feb 19, 2024

fix vertex finding for z=0

afc6698

andiwand force-pushed the fix-vertex-finding-z-0 branch from cddd364 to afc6698 Compare February 19, 2024 16:15

update reference

4825aa9

github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Feb 20, 2024

paulgessinger previously approved these changes Feb 20, 2024

View reviewed changes

andiwand dismissed paulgessinger’s stale review via 6e7c074 February 20, 2024 10:08

Apply suggestions from code review

6e7c074

Co-authored-by: Paul Gessinger <hello@paulgessinger.com>

acts-project-service added the Breaks Athena build This PR breaks the Athena build label Feb 21, 2024

andiwand added a commit to andiwand/acts that referenced this pull request Feb 21, 2024

Revert "fix!: Fix vertex finding for seeds with z=0 (acts-project#2917

151f21f

)" This reverts commit 123746c.

andiwand mentioned this pull request Feb 21, 2024

Revert "fix!: Fix vertex finding for seeds with z=0 (#2917)" #2978

Closed

paulgessinger pushed a commit to andiwand/acts that referenced this pull request Feb 21, 2024

Revert "fix!: Fix vertex finding for seeds with z=0 (acts-project#2917

dbdc5a3

)" This reverts commit 123746c.

paulgessinger added Fails Athena tests This PR causes a failure in the Athena tests and removed Breaks Athena build This PR breaks the Athena build labels Feb 21, 2024

andiwand commented Feb 21, 2024

View reviewed changes

andiwand added a commit to andiwand/acts that referenced this pull request Feb 22, 2024

Revert "fix!: Fix vertex finding for seeds with z=0 (acts-project#2917

5d1c74c

)" This reverts commit 123746c.

andiwand added a commit to andiwand/acts that referenced this pull request Feb 22, 2024

Revert "Revert "fix!: Fix vertex finding for seeds with z=0 (acts-p…

931eadd

…roject#2917)"" This reverts commit dbdc5a3.

This was referenced Feb 28, 2024

refactor: AMVF error handling and zt splitting #2828

Merged

fix: Wire optional seed result in TrackDensityVertexFinder after #2917 #3002

Merged

paulgessinger modified the milestones: next, v33.0.0 Mar 6, 2024

paulgessinger added Fails Athena tests This PR causes a failure in the Athena tests and removed Fails Athena tests This PR causes a failure in the Athena tests labels Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: Fix vertex finding for seeds with `z=0` #2917

fix!: Fix vertex finding for seeds with `z=0` #2917

andiwand commented Jan 31, 2024 •

edited

Loading

pbutti left a comment

github-actions bot commented Feb 1, 2024 •

edited

Loading

felix-russo left a comment

andiwand commented Feb 3, 2024

codecov bot commented Feb 5, 2024 •

edited

Loading

andiwand commented Feb 5, 2024 •

edited

Loading

andiwand commented Feb 13, 2024

andiwand commented Feb 20, 2024

paulgessinger left a comment

acts-project-service commented Feb 21, 2024

paulgessinger commented Feb 21, 2024

andiwand Feb 21, 2024

andiwand commented Feb 29, 2024

fix!: Fix vertex finding for seeds with z=0 #2917

fix!: Fix vertex finding for seeds with z=0 #2917

Conversation

andiwand commented Jan 31, 2024 • edited Loading

pbutti left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 1, 2024 • edited Loading

📊: Physics performance monitoring for bf674c6

physmon summary

felix-russo left a comment

Choose a reason for hiding this comment

andiwand commented Feb 3, 2024

codecov bot commented Feb 5, 2024 • edited Loading

Codecov Report

andiwand commented Feb 5, 2024 • edited Loading

andiwand commented Feb 13, 2024

andiwand commented Feb 20, 2024

paulgessinger left a comment

Choose a reason for hiding this comment

acts-project-service commented Feb 21, 2024

🔴 Athena integration test results [123746c]

Build job with this PR failed!

paulgessinger commented Feb 21, 2024

andiwand Feb 21, 2024

Choose a reason for hiding this comment

andiwand commented Feb 29, 2024

fix!: Fix vertex finding for seeds with `z=0` #2917

fix!: Fix vertex finding for seeds with `z=0` #2917

andiwand commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading

📊: Physics performance monitoring for `bf674c6`

codecov bot commented Feb 5, 2024 •

edited

Loading

andiwand commented Feb 5, 2024 •

edited

Loading

🔴 Athena integration test results [`123746c`]