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: Propagate initial vertex time variance for AMVF w/o time #2936

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Feb 7, 2024

Logs like

21:16:17    VertexPerfor   WARNING   Nonpositive variance after vertex fit: Var(T) = 0 <= 0.

are gone after this change.

To me it seems more sensical to propagate the initial time variance instead of hard setting it to 0 which causes the cov matrix to be invalid.

@andiwand andiwand added this to the next milestone Feb 7, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Vertexing labels Feb 7, 2024
@andiwand
Copy link
Contributor Author

andiwand commented Feb 7, 2024

@felix-russo do you have an opinion on this?

@pbutti
Copy link
Contributor

pbutti commented Feb 7, 2024

@andiwand Alternatively one could make sure that a very large positive value to the initial for sigma_tt is present in the initial cov matrix (or set it to a very large value) and check/set that the other terms in the last row/column of the covariance matrix to 0 and just call the KF update on it.
Removing the lines where the time row/column are set to 0 makes sense to me.
In the case of no time information, the last row/column should not provide any weight and setting it as above should achieve that.

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (86e62ab) 48.84% compared to head (d8af9fb) 48.84%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2936   +/-   ##
=======================================
  Coverage   48.84%   48.84%           
=======================================
  Files         495      495           
  Lines       28911    28909    -2     
  Branches    13734    13732    -2     
=======================================
  Hits        14122    14122           
  Misses       4890     4890           
+ Partials     9899     9897    -2     

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

@andiwand
Copy link
Contributor Author

andiwand commented Feb 8, 2024

@andiwand Alternatively one could make sure that a very large positive value to the initial for sigma_tt is present in the initial cov matrix (or set it to a very large value) and check/set that the other terms in the last row/column of the covariance matrix to 0 and just call the KF update on it. Removing the lines where the time row/column are set to 0 makes sense to me. In the case of no time information, the last row/column should not provide any weight and setting it as above should achieve that.

@pbutti We are actually forwarding the initial covariance into the vertex fit which, like you described, has a big diagonal covariance. To me it seems good enough to use the initial cov. What do you think?

@felix-russo
Copy link
Contributor

@felix-russo do you have an opinion on this?

Looks very good to me!

@andiwand
Copy link
Contributor Author

I will go ahead and update the reference here

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Feb 13, 2024
@kodiakhq kodiakhq bot merged commit b50dd08 into acts-project:main Feb 13, 2024
54 checks passed
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Feb 13, 2024
@andiwand andiwand deleted the amvf-wo-time-proper-cov branch February 13, 2024 22:00
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Feb 14, 2024
@paulgessinger paulgessinger removed Breaks Athena build This PR breaks the Athena build Fails Athena tests This PR causes a failure in the Athena tests labels Feb 15, 2024
@paulgessinger
Copy link
Member

Athena failures unrelated.

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Feb 15, 2024
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Feb 15, 2024
@paulgessinger paulgessinger modified the milestones: next, v33.0.0 Mar 6, 2024
@paulgessinger paulgessinger removed Breaks Athena build This PR breaks the Athena build Fails Athena tests This PR causes a failure in the Athena tests labels Mar 6, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…roject#2936)

Logs like
```
21:16:17    VertexPerfor   WARNING   Nonpositive variance after vertex fit: Var(T) = 0 <= 0.
```
are gone after this change.

To me it seems more sensical to propagate the initial time variance instead of hard setting it to `0` which causes the cov matrix to be invalid.
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…roject#2936)

Logs like
```
21:16:17    VertexPerfor   WARNING   Nonpositive variance after vertex fit: Var(T) = 0 <= 0.
```
are gone after this change.

To me it seems more sensical to propagate the initial time variance instead of hard setting it to `0` which causes the cov matrix to be invalid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Core Affects the Core module Infrastructure Changes to build tools, continous integration, ... Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants