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: GX2F: update track state parameters and converge #1702

Merged
merged 13 commits into from
Dec 5, 2022

Conversation

AJPfleger
Copy link
Contributor

The previous implementation could not update the track state parameters.
The updated Jacobian was overridden by stepper.transportCovarianceToBound. Therefore, every time the identity matrix was used in place of the Jacobian.
A second problem was the handling of surfaces in the update steps. Now, during the first iteration, proxies are created for each surface. During further iterations those surfaces are visited again.

@AJPfleger AJPfleger added this to the next milestone Nov 30, 2022
@AJPfleger AJPfleger added Component - Core Affects the Core module Impact - Minor Nuissance bug and/or affects only a single module labels Nov 30, 2022
@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #1702 (48f213a) into main (f30c4af) will increase coverage by 0.01%.
The diff coverage is 51.72%.

❗ Current head 48f213a differs from pull request most recent head 693359a. Consider uploading reports for the commit 693359a to get more accurate results

@@            Coverage Diff             @@
##             main    #1702      +/-   ##
==========================================
+ Coverage   49.03%   49.04%   +0.01%     
==========================================
  Files         397      397              
  Lines       21564    21581      +17     
  Branches     9817     9824       +7     
==========================================
+ Hits        10573    10585      +12     
- Misses       4183     4186       +3     
- Partials     6808     6810       +2     
Impacted Files Coverage Δ
Core/include/Acts/TrackFitting/Chi2Fitter.hpp 36.01% <51.72%> (+3.02%) ⬆️

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

@github-actions
Copy link

github-actions bot commented Nov 30, 2022

📊 Physics performance monitoring for 693359a

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: 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

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

Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Very nice work! There are a few things that I think should be cleaned up, but in general I think we are good here.
I think you could think about either fixing the parameters type directly to BoundTrackParameters and removing the related code or going back to the generic, templated case. I would not stay somewhere in the middle.

Core/include/Acts/TrackFitting/Chi2Fitter.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/Chi2Fitter.hpp Show resolved Hide resolved
Core/include/Acts/TrackFitting/Chi2Fitter.hpp Show resolved Hide resolved
Core/include/Acts/TrackFitting/Chi2Fitter.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/Chi2Fitter.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/Chi2Fitter.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/Chi2Fitter.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/Chi2Fitter.hpp Outdated Show resolved Hide resolved
Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Hmm this std::contitional is a bit suspicious to me still...

Core/include/Acts/TrackFitting/Chi2Fitter.hpp Outdated Show resolved Hide resolved
Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

let it go!

@kodiakhq kodiakhq bot merged commit 1d4f320 into acts-project:main Dec 5, 2022
@github-actions github-actions bot removed the automerge label Dec 5, 2022
pbutti pushed a commit to pbutti/acts that referenced this pull request Dec 13, 2022
)

The previous implementation could not update the track state parameters.
The updated Jacobian was overridden by `stepper.transportCovarianceToBound`. Therefore, every time the identity matrix was used in place of the Jacobian.
A second problem was the handling of surfaces in the update steps. Now, during the first iteration, proxies are created for each surface. During further iterations those surfaces are visited again.
@paulgessinger paulgessinger removed this from the next milestone Dec 21, 2022
@paulgessinger paulgessinger added this to the v22.0.0 milestone Dec 21, 2022
CarloVarni pushed a commit to CarloVarni/acts that referenced this pull request Dec 22, 2022
)

The previous implementation could not update the track state parameters.
The updated Jacobian was overridden by `stepper.transportCovarianceToBound`. Therefore, every time the identity matrix was used in place of the Jacobian.
A second problem was the handling of surfaces in the update steps. Now, during the first iteration, proxies are created for each surface. During further iterations those surfaces are visited again.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants