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: Adding time to HelicalTrackLinearizer #2179

Merged
merged 38 commits into from
Aug 25, 2023

Conversation

felix-russo
Copy link
Contributor

@felix-russo felix-russo commented Jun 6, 2023

In this PR the derivatives of/wrt time are added to the Jacobians computed in HelicalTrackLinearizer. The terms have been checked by comparing them to numerically computed derivatives (see PR #2141).

Here is a PDF where the Jacobians are derived: Track_Linearization.pdf.
Note that nobody (except me) read this yet, so it might have some mistakes in it. Any feedback is more than welcome!

Something that might need discussion: To compute the new terms, we need to have a mass and a charge hypothesis of the tracks. Right now, Pion mass and unit charge are assumed in the code, but it might be worth adding specific hypotheses to the track objects.

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #2179 (e036771) into main (ac31a9b) will increase coverage by 0.00%.
The diff coverage is 30.15%.

@@           Coverage Diff           @@
##             main    #2179   +/-   ##
=======================================
  Coverage   49.52%   49.52%           
=======================================
  Files         453      453           
  Lines       25677    25691   +14     
  Branches    11815    11819    +4     
=======================================
+ Hits        12716    12723    +7     
- Misses       4583     4585    +2     
- Partials     8378     8383    +5     
Files Changed Coverage Δ
.../include/Acts/Vertexing/HelicalTrackLinearizer.hpp 100.00% <ø> (ø)
...nclude/Acts/Vertexing/NumericalTrackLinearizer.ipp 13.55% <0.00%> (-2.81%) ⬇️
.../include/Acts/Vertexing/HelicalTrackLinearizer.ipp 26.13% <32.20%> (+6.90%) ⬆️

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

@AJPfleger AJPfleger added this to the next milestone Jun 6, 2023
Copy link
Contributor

@AJPfleger AJPfleger left a comment

Choose a reason for hiding this comment

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

Could it make sense to add the derivation on overleaf as some screenshots to the PR description?

mass hypothesis from propagator options
@felix-russo felix-russo added the Component - Core Affects the Core module label Jun 12, 2023
@felix-russo felix-russo requested a review from pbutti June 13, 2023 13:48
@pbutti
Copy link
Contributor

pbutti commented Jun 15, 2023

@andiwand what's the status of merging this in?
@felix-russo Next step is to implement a test to check the FullBilloirFit. what about porting this? https://github.com/pbutti/acts/blob/pf_4dvtx/Tests/UnitTests/Core/Vertexing/FullBilloirVertexFitterTests.cpp

@paulgessinger paulgessinger added this to the v29.0.0 milestone Aug 1, 2023
@acts-policybot acts-policybot bot dismissed pbutti’s stale review August 3, 2023 14:29

Invalidated by push of 4002e83

@paulgessinger
Copy link
Member

paulgessinger commented Aug 8, 2023

I manually launched an Athena CI run here: https://gitlab.cern.ch/acts/acts-athena-ci/-/pipelines/6020912

@felix-russo
Copy link
Contributor Author

felix-russo commented Aug 10, 2023

No large differences in athena expected, see athena-ci.zip

@felix-russo
Copy link
Contributor Author

Thanks to @andiwand's stabilization of the AMVF, these changes only have an impact on the IVF (where we don't exclude the time during the vertex fit by using head statements).

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Aug 23, 2023
@github-actions github-actions bot removed Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Aug 24, 2023
@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Aug 24, 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.

Let's gamble, try merging.

@kodiakhq kodiakhq bot merged commit 7d2a029 into acts-project:main Aug 25, 2023
56 checks passed
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Aug 25, 2023
@felix-russo felix-russo deleted the HTL_with_timing branch October 3, 2023 11:37
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 Fails Athena tests This PR causes a failure in the Athena tests Infrastructure Changes to build tools, continous integration, ... Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants