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

Kalman Fitter on CPU #264

Merged
merged 23 commits into from
Jan 6, 2023
Merged

Kalman Fitter on CPU #264

merged 23 commits into from
Jan 6, 2023

Conversation

beomki-yeo
Copy link
Contributor

@beomki-yeo beomki-yeo commented Nov 2, 2022

This is a (very preliminary) PR for Kalman filtering on CPU. The detray pull requests also should be merged before we accept this PR.

Let me explain one by one as the PR is pretty big...

EDM

There are two EDM classes added. track_candidate, which takes geometry link and measurement object, is an input to kalman filtering and potentially an output of combinatorial kalman filtering. track_state is the output of kalman filtering, which takes the fitted track parameters, measurement and geometry link.

Algorithm structure

There is a fitting_algorithm class which runs the fitting algorithm for track_candidate container input (Doesn't have to be for single event). kalman_fitting, which is wrapped by fitting_algorithm, runs the fitting algorithm for a single track. It runs detray::propagator inside a given geometry which triggers actor_chain for every surface intersection.

The actor chain is made of the following actors: (1) parameter transporter -> (2) material interactor -> (3) kalman actor -> (4) parameter resetter.
(1) is for calculating the jacobian from the last surface to the current surface
(2) is for calculating the effect of energy loss and multiple scattering
(3) is for kalman filtering
(4) is for resetting the track parameters for the next surface intersection

(1), (2), (4) are already defined in detray so people can check them out by themselves.

Kalman filtering (kalman actor)

The updating or filtering of the predicted track parameters is done by the kalman actor The algorithm is pretty much the same with one implemented in ACTS main repository. Techincally, eq. 7 - 8b of R.Frühwirth is implemented in gain_matrix_updater

Kalman smoothing

Given that kalman filtering is an iterative algorithm on the time series of measurements, the filtered state at the last surface contains all information from the previous surfaces while the first measurement only contains the information of its own surface. Since we are mostly interested in the fitted state of the first surface, we need to refine the fitted parameters using the smoothing algorithm. There are two ways for smoothing: (1) For a smoothed parameter of n-th surface, remove the process noise using the predicted and smoothed parameters of the n+1-th surface. Since the smoothed parameters of the last surface is equal to the filtered parameter, the algorithm should iterate backwardly (This is what implemented in ACTS) (See Eq. 3.36 - 3.37 of this book (2) Do one more kalman filtering backwardly from the last surface to the first surface and take an average of two kalman filtering result. (See Eq. 3.38 - 3.40 of this book.)

I tried to implemented (2) method at the begining but ended up with implementing (1), because the backward navigation has some issues right now.

Test Results:

Setup

  • Telescope geometry implemented in detray (9 surfaces and 5 cm gap) aligned along z axis.
  • 10000 tracks, p = 1 GeV, theta = pi/6, phi = pi/6
  • 50 um measurement resolution for local0 and local1
  • The seed was smeared by standard deviation of (x = 1 mm, y = 1mm, z = 1mm, n_x = 0.01, n_y = 0.01, n_z = 0.01, qop = 0.1)
  • Precision was double

Following figures show the pull distributions of bound track parameters at the first surface. I don't know why d0 and z0 (local0 and local1) show unreasonable results (@asalzburger @paulgessinger Do you guys have any idea ?)

One thing to note is that the fitting with a single precision makes wrong results after the smoothing where chi square gets negative.

What to do next

  1. The kalman filtering in this PR was tested in a very ideal detector setup. In real cases, lots of exceptions will occur during the fitting, which we need to handle very carefully.

  2. Haven't tested GPU yet, but I have a strong feeling that we will have a bad performance unless we achieve the thread synchronization over the actor chain triggering.

  3. As mentioned above, single precision is not working correctly for smoothing. I think it is because the residual covariance is not always positive definite. The ACTS main repository is handling this with the single value decomposition (SVD) to make the covaraince positive definite. We need to think about implementing SVD in algebra-plugins

  4. I made another (very unsatisfactory) event map class for this truth tracking validation. I need to come up with a better way to store the truth information of events.

@paulgessinger
Copy link
Member

paulgessinger commented Nov 2, 2022

First of all this looks like fantastic work! Thanks @beomki-yeo!

Are the pulls you show the smoothed ones?
To understand why the d0/z0 pulls look so narrow, it might help to look at the residual and the error estimate separately? It might be that e.g. the error is overestimated.

@asalzburger
Copy link
Contributor

This is impressive work!

Note that d0 and z0 are the only ones somehow that are estimated directly (by the measurements) and not indirectly (by the trajectory), so, Indeed it looks like the errors are not correctly defined.

It does help to print the residuals/pulls per measurement surface ...

@beomki-yeo
Copy link
Contributor Author

Yeah They are smoothed parameter. There is definitely something wrong because chi square gets bigger with smoothed parameters :/ Let me dig into it

@beomki-yeo
Copy link
Contributor Author

Nahh I was comparing with smeared local position instead of the truth local position. Hopefully that fixes the issue...

@beomki-yeo beomki-yeo marked this pull request as draft November 3, 2022 17:52
@beomki-yeo beomki-yeo marked this pull request as ready for review November 15, 2022 03:42
@beomki-yeo
Copy link
Contributor Author

PR is ready for review. Let me leave event_map as it is for now...

@beomki-yeo beomki-yeo marked this pull request as draft November 17, 2022 01:55
@beomki-yeo
Copy link
Contributor Author

Wow. I just found that KF with cmath plugin doesn't work :/ ..

@beomki-yeo beomki-yeo marked this pull request as ready for review November 21, 2022 05:50
@beomki-yeo
Copy link
Contributor Author

All bugs are fixed now but I still need to wait for acts-project/detray#360 and acts-project/algebra-plugins#80 to be merged.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I have some reservations against the current interface of the fitter code. We should really make it MT friendly from the start. Otherwise I fear that it will be even more painful to make it MT friendly later on.

core/include/traccc/edm/track_candidate.hpp Outdated Show resolved Hide resolved
core/include/traccc/fitting/fitting_algorithm.hpp Outdated Show resolved Hide resolved
core/include/traccc/fitting/kalman_filter/kalman_actor.hpp Outdated Show resolved Hide resolved
tests/io/test_event_map.cpp Show resolved Hide resolved
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Updates are still necessary to make this compatible with the main branch.

But generally I'm a lot happier about the API of the new classes now.

tests/cpu/test_kalman_fitter.cpp Outdated Show resolved Hide resolved
tests/cpu/test_kalman_fitter.cpp Outdated Show resolved Hide resolved
examples/simulation/simulate_telescope.cpp Outdated Show resolved Hide resolved
@beomki-yeo beomki-yeo mentioned this pull request Dec 9, 2022
@beomki-yeo
Copy link
Contributor Author

@krasznaa Could you check the PR?

@guilhermeAlmeida1
Copy link
Collaborator

@beomki-yeo Attila's on his Christmas break (as most already are), so he might only get around to this in January

@krasznaa
Copy link
Member

Not quite next year, but by now I'm at 1100m next to a ski slope. With only my phone for electronics.

Will have a look once I'm down from the mountain at the end of the week. 😄

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Only very technical comments on my part. As far as I'm concerned, it could go in like this. But if you think that some of the technical changes I proposed would be reasonable, please go ahead with them. 😉

core/include/traccc/fitting/fitting_algorithm.hpp Outdated Show resolved Hide resolved
core/include/traccc/fitting/fitting_algorithm.hpp Outdated Show resolved Hide resolved
performance/include/traccc/resolution/res_plot_tool.hpp Outdated Show resolved Hide resolved
performance/include/traccc/resolution/res_plot_tool.hpp Outdated Show resolved Hide resolved
performance/include/traccc/utils/helpers.hpp Show resolved Hide resolved
@beomki-yeo
Copy link
Contributor Author

@krasznaa Could you approve again if the changes are OK?

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I'm on board with the latest changes. Have to admit, didn't re-read all of the changes from scratch. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants