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: Testing HelicalTrackLinearizer by comparing its Jacobians to numerically computed derivatives #2141

Merged
merged 38 commits into from
Jun 28, 2023

Conversation

felix-russo
Copy link
Contributor

@felix-russo felix-russo commented May 23, 2023

Previously, the unit test for the HelicalTrackLinearizer only checked if its outputs were non-zero matrices. To improve the testing, a new class, NumericalTrackLinearizer, is introduced, which allows us to compute Jacobians numerically (i.e., we approximate the derivatives like f'(x) ~ (f(x + dx) - f(x)) / dx). The numerically computed Jacobians are then compared to the ones we obtain from the HelicalTrackLinearizer in the improved unit test.

Comments concerning code readability and style are highly appreciated!

@github-actions
Copy link

github-actions bot commented May 23, 2023

📊 Physics performance monitoring for ac66957

Summary
Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
AMVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

AMVF seeded

AMVF truth_smeared

AMVF truth_estimated

AMVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #2141 (30aef92) into main (91fcbb7) will decrease coverage by 0.05%.
The diff coverage is 27.69%.

❗ Current head 30aef92 differs from pull request most recent head ac66957. Consider uploading reports for the commit ac66957 to get more accurate results

@@            Coverage Diff             @@
##             main    #2141      +/-   ##
==========================================
- Coverage   49.35%   49.30%   -0.05%     
==========================================
  Files         445      447       +2     
  Lines       25264    25324      +60     
  Branches    11650    11693      +43     
==========================================
+ Hits        12468    12487      +19     
- Misses       4512     4513       +1     
- Partials     8284     8324      +40     
Impacted Files Coverage Δ
...nclude/Acts/Vertexing/NumericalTrackLinearizer.ipp 16.36% <16.36%> (ø)
...nclude/Acts/Vertexing/NumericalTrackLinearizer.hpp 90.00% <90.00%> (ø)

... and 3 files with indirect coverage changes

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

@andiwand andiwand added this to the next milestone May 24, 2023
@andiwand andiwand added the Component - Core Affects the Core module label May 24, 2023
@andiwand andiwand added the 🚧 WIP Work-in-progress label May 24, 2023
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

I wonder how much we could use from RiddersPropagator - I guess it does basically do what you did here by hand?

@beomki-yeo
Copy link
Contributor

It might not have nothing to do with this PR, but there is also a way to validate HelicalTrackLinearizer with the analytical solution of a helix model.

See
https://github.com/acts-project/detray/blob/51571670ea7b8e574db560c9dadf49401352583d/core/include/detray/intersection/detail/trajectories.hpp#L268

which is based on the helix model (Eq. 4.28 - 4.34) of https://doi.org/10.1007/978-3-030-65771-0

@felix-russo
Copy link
Contributor Author

felix-russo commented May 31, 2023

I wonder how much we could use from RiddersPropagator - I guess it does basically do what you did here by hand?

I don't know if it makes sense to unify the code here with the one in RiddersPropagator - let me know what you think!

@andiwand
Copy link
Contributor

thanks for looking into this @russofel ! I guess if it would expose the jacobian instead of (or additional to) the covariance it would fit our use case. but right now it does not really make sense as we would have to find the jacobian from the final covariance? and I don't know if that is even possible

@felix-russo
Copy link
Contributor Author

It might not have nothing to do with this PR, but there is also a way to validate HelicalTrackLinearizer with the analytical solution of a helix model.

See https://github.com/acts-project/detray/blob/51571670ea7b8e574db560c9dadf49401352583d/core/include/detray/intersection/detail/trajectories.hpp#L268

which is based on the helix model (Eq. 4.28 - 4.34) of https://doi.org/10.1007/978-3-030-65771-0

The HelicalTrackLinearizer is purely based on analytical calculations, see Sec. 5.3.3 and 5.3.4 of https://cds.cern.ch/record/1243771?ln=en.

I am not quite sure how these calculations relate to the resource you have linked in your comment though... I will need to look into this in detail!

@beomki-yeo
Copy link
Contributor

I am sorry that It is not related to your work anymore. The link that I provided is merely about the jacobian transport (free->free) which is not part of HelicalLinearizer (perigee->free). I just didn't understand this when I made a comment

@kodiakhq kodiakhq bot merged commit bd785d9 into acts-project:main Jun 28, 2023
@felix-russo felix-russo deleted the unit-test-linearizer branch July 3, 2023 14:54
@paulgessinger paulgessinger modified the milestones: next, v27.1.0 Jul 4, 2023
noemina pushed a commit to noemina/acts that referenced this pull request Jul 5, 2023
…o numerically computed derivatives (acts-project#2141)

Previously, the unit test for the HelicalTrackLinearizer only checked if its outputs were non-zero matrices. To improve the testing, a new class, NumericalTrackLinearizer, is introduced, which allows us to compute Jacobians numerically (i.e., we approximate the derivatives like f'(x) ~ (f(x + dx) - f(x)) / dx). The numerically computed Jacobians are then compared to the ones we obtain from the HelicalTrackLinearizer in the improved unit test.

Comments concerning code readability and style are highly appreciated!
kodiakhq bot pushed a commit that referenced this pull request Aug 25, 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](https://github.com/acts-project/acts/files/11664828/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.
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 Impact - Minor Nuissance bug and/or affects only a single module Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants