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: Prevent FPE in AMVF if covariance is not invertible #1142

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Jan 20, 2022

It's unclear to me how this covariance can end up not being invertible, but it has been observed when running the ACTS AMVF in an ATLAS online context.

I briefly discussed with @baschlag whether returning true, i.e. treating the vertex as merged is correct, and we think it's safer to do so, and therefore discard the vertex one level up the call chain.

I'm going to cherry pick this PR into branch release/v15.x to create a bugfix tag for the v15 release.

@paulgessinger paulgessinger added the Bug Something isn't working label Jan 20, 2022
@paulgessinger paulgessinger added this to the next milestone Jan 20, 2022
@baschlag
Copy link
Contributor

Hi, as Paul mentioned I also think that treating these vertices as merged and thus discard them is the safest way to go here.

@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #1142 (828c90d) into main (1e08892) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1142      +/-   ##
==========================================
- Coverage   48.18%   48.17%   -0.02%     
==========================================
  Files         341      341              
  Lines       17645    17649       +4     
  Branches     8324     8327       +3     
==========================================
  Hits         8502     8502              
- Misses       3368     3372       +4     
  Partials     5775     5775              
Impacted Files Coverage Δ
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp 41.46% <0.00%> (-0.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e08892...828c90d. Read the comment docs.

@kodiakhq kodiakhq bot merged commit 787658a into acts-project:main Jan 20, 2022
@paulgessinger paulgessinger deleted the fix/vtx-fpe branch January 20, 2022 17:32
paulgessinger added a commit that referenced this pull request Jan 20, 2022
It's unclear to me how this covariance can end up not being invertible, but it has been observed when running the ACTS AMVF in an ATLAS online context.

We think it's safer to do so, and therefore discard the vertex one level up the call chain.

This is the backported version of #1142 to the `v15.x` release.
@paulgessinger paulgessinger modified the milestones: next, v17.0.0 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants