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: Add nonlinear correction in KF #1233

Merged
merged 24 commits into from
May 9, 2022

Conversation

XiaocongAi
Copy link
Contributor

This PR refactors a few methods of the stepper related with transforming from free to bound parameters. It adds the option for correction of non-linear effects during the transformation using a few sigma points (https://doi.org/10.1117/12.280797).

@XiaocongAi XiaocongAi added Component - Core Affects the Core module 🚧 WIP Work-in-progress labels Apr 21, 2022
@XiaocongAi XiaocongAi added this to the next milestone Apr 21, 2022
@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #1233 (595c3bf) into main (0b8796e) will decrease coverage by 0.08%.
The diff coverage is 37.69%.

@@            Coverage Diff             @@
##             main    #1233      +/-   ##
==========================================
- Coverage   47.98%   47.89%   -0.09%     
==========================================
  Files         373      375       +2     
  Lines       19484    19588     +104     
  Branches     9148     9214      +66     
==========================================
+ Hits         9349     9382      +33     
- Misses       3804     3822      +18     
- Partials     6331     6384      +53     
Impacted Files Coverage Δ
Core/include/Acts/Propagator/EigenStepper.hpp 68.42% <ø> (ø)
...re/include/Acts/Propagator/StraightLineStepper.hpp 67.85% <ø> (ø)
Core/src/Propagator/detail/CovarianceEngine.cpp 47.44% <15.00%> (-4.63%) ⬇️
...c/EventData/CorrectedTransformationFreeToBound.cpp 32.46% <32.46%> (ø)
Core/include/Acts/TrackFitting/KalmanFitter.hpp 45.07% <44.44%> (+0.41%) ⬆️
...e/Acts/TrackFitting/detail/KalmanUpdateHelpers.hpp 22.03% <50.00%> (ø)
.../include/Acts/Propagator/MultiEigenStepperLoop.hpp 70.37% <60.00%> (+0.18%) ⬆️
Core/src/Propagator/StraightLineStepper.cpp 68.42% <60.00%> (-1.03%) ⬇️
Core/include/Acts/Propagator/AtlasStepper.hpp 68.51% <66.66%> (ø)
...Data/detail/CorrectedTransformationFreeToBound.hpp 100.00% <100.00%> (ø)
... and 5 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@XiaocongAi
Copy link
Contributor Author

Hi @asalzburger , it's the PR for the non-linear KF. Could you help have a look?

@XiaocongAi XiaocongAi removed the 🚧 WIP Work-in-progress label Apr 26, 2022
Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

Very nicely integrated, please check if the introduction of a small struct that carries a bool plus the unable parameters wouldn't make it even a step clearer - I suppose so!

Core/include/Acts/Propagator/AtlasStepper.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFitting/KalmanFitter.hpp Outdated Show resolved Hide resolved
Core/src/EventData/CorrectedTransformationFreeToBound.cpp Outdated Show resolved Hide resolved
Core/src/EventData/CorrectedTransformationFreeToBound.cpp Outdated Show resolved Hide resolved
Core/src/EventData/CorrectedTransformationFreeToBound.cpp Outdated Show resolved Hide resolved
Core/src/Propagator/detail/CovarianceEngine.cpp Outdated Show resolved Hide resolved
@XiaocongAi
Copy link
Contributor Author

Hi @asalzburger , thank you a lot for the suggestions. I should have resolved all the comments.

Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

A few more things: Paul's suggestion of not auto-constructing from pool & the moving out of the detail namespace.

@XiaocongAi
Copy link
Contributor Author

Hi @asalzburger @paulgessinger , I have resolved your comments. Could you have another look?

Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

Comments are addressed, there is still a failure in the CI to check.

@asalzburger
Copy link
Contributor

Hi @XiaocongAi - there are build failures, due to a missing test update & a mismatch in the concept, please resolve these ones and I will reapprove.

@paulgessinger
Copy link
Member

Note there are also some CI issues that should be fixed with #1243.

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.

I would strongly prefer we make the FreeToBoundCorrection constructor from bool explicit like

explicit FreeToBoundCorrection(bool apply_);

See comment in the changeset.

@asalzburger
Copy link
Contributor

From my side this is ok, let's see if the CI runs through and @paulgessinger is happy as well.

@XiaocongAi
Copy link
Contributor Author

Hi @paulgessinger , thank you for your further comments. I have made the change. Do you think you can approve and get it in?

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.

LGTM as far as I can tell at this point!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Component - Core Affects the Core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants