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

refactor: remove division of vertex covariance with weight #2522

Merged
merged 10 commits into from
Oct 17, 2023

Conversation

felix-russo
Copy link
Contributor

@felix-russo felix-russo commented Oct 9, 2023

Removes the division of the vertex covariance matrix with a weight that depends on the annealing temperature.

I believe that the reasoning behind this practice is that the vertex covariance should decrease gradually with temperature. This seems however a bit random to me because

  • the temperature is already considered in the track weights
  • the chi2 corresponding to the weight is set to the arbitrary value 1
  • there is no documentation on this that I know of. For example, https://link.springer.com/book/10.1007/978-3-030-65771-0 on page 154, the authors only mention the inflation of the track covariance matrices Vi, see
    image

Let me know if I am missing something!

@github-actions github-actions bot added Component - Core Affects the Core module Vertexing labels Oct 9, 2023
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #2522 (aad0fc0) into main (ecc68bc) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2522   +/-   ##
=======================================
  Coverage   49.76%   49.76%           
=======================================
  Files         466      466           
  Lines       26326    26323    -3     
  Branches    12097    12095    -2     
=======================================
- Hits        13101    13100    -1     
  Misses       4624     4624           
+ Partials     8601     8599    -2     
Files Coverage Δ
...clude/Acts/Vertexing/AdaptiveMultiVertexFitter.ipp 44.07% <ø> (+0.20%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@felix-russo felix-russo changed the title refactor: remove multiplication of vertex covariance with weight refactor: remove division of vertex covariance with weight Oct 10, 2023
@felix-russo felix-russo marked this pull request as ready for review October 10, 2023 11:34
@paulgessinger
Copy link
Member

Should we try to investigate where this comes from? @baschlag do you remember if this was discussed at some point?

@felix-russo
Copy link
Contributor Author

I found the same line athena, might be helpful to for tracing where this comes from: https://gitlab.cern.ch/atlas/athena/-/blame/main/Tracking/TrkVertexFitter/TrkVertexFitters/src/AdaptiveMultiVertexFitter.cxx#L137

@baschlag
Copy link
Contributor

Hi. This part was indeed just adopted from the original Athena implementation in order to guarantee numerical agreement between the ACTS and Athena version (which was needed for validating the code). So I think your changes look in fact very reasonable.

@paulgessinger paulgessinger added this to the next milestone Oct 13, 2023
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me then. Let's see what the CI says in terms of Athena outputs after we merge.

@kodiakhq kodiakhq bot merged commit 89888a4 into acts-project:main Oct 17, 2023
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Oct 17, 2023
@felix-russo
Copy link
Contributor Author

Hm.. What should we do?

@paulgessinger
Copy link
Member

If we want to tag another non-major version this week, we might have to revert this. That ok for you?

@felix-russo
Copy link
Contributor Author

Sure, as long as it gets in at some point :)
I will prepare the PRs

felix-russo added a commit to felix-russo/acts that referenced this pull request Oct 17, 2023
kodiakhq bot pushed a commit that referenced this pull request Oct 17, 2023
@felix-russo felix-russo deleted the remove-vtx-weight branch October 17, 2023 15:37
@paulgessinger paulgessinger modified the milestones: next, v30.3.0 Oct 25, 2023
kodiakhq bot pushed a commit that referenced this pull request Oct 25, 2023
Reopen #2522 which we had to revert due to athena output changes
Should go in for the next major version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Fails Athena tests This PR causes a failure in the Athena tests Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants