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

Backward filter for Two-filter Smoothing #788

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

beomki-yeo
Copy link
Contributor

@beomki-yeo beomki-yeo commented Nov 30, 2024

This PR implements the two-filters smoother where a smoothed state is calculated by taking a weighted average between filtered state from the forward filter and predicted state from the backward filter (See Eq (3.38) of Pattern Recognition, Tracking and Vertex Reconstruction in Particle Detectors)

Following changes are made to implement the two-filters method:

  1. Backward propagator, whose detray actor sequence is reversed, is implemented to run the backward filter
  2. Initial covariance is always inflated for both forward and backward filter
  3. Test for the uniform distribution of p-values is also added

However, the default smoothing algorithm will still be the RTS smoother (i.e. smoothing algorithm which is already implemented) until we improve the two-filters method with the DirectNavigator of ACTS. The DirectNavigator is required to make sure that forward and backward filter propagate through the same set of surfaces.

@beomki-yeo beomki-yeo marked this pull request as ready for review December 12, 2024 15:45
stephenswat added a commit to stephenswat/vecmem that referenced this pull request Dec 12, 2024
As @beomki-yeo discovered in
acts-project/traccc#788, constant reverse
iterators cannot be currently compared to non-constant reverse iterators
as is required by the C++ standard in section
[container.requirements.general]. This commit adds the necessary
comparison code, and adds some tests for the new behaviour.
stephenswat added a commit to stephenswat/vecmem that referenced this pull request Dec 13, 2024
As @beomki-yeo discovered in
acts-project/traccc#788, constant reverse
iterators cannot be currently compared to non-constant reverse iterators
as is required by the C++ standard in section
[container.requirements.general]. This commit adds the necessary
comparison code, and adds some tests for the new behaviour.
stephenswat added a commit to stephenswat/vecmem that referenced this pull request Dec 13, 2024
As @beomki-yeo discovered in
acts-project/traccc#788, constant reverse
iterators cannot be currently compared to non-constant reverse iterators
as is required by the C++ standard in section
[container.requirements.general]. This commit adds the necessary
comparison code, and adds some tests for the new behaviour.
@beomki-yeo beomki-yeo force-pushed the backward-filter branch 5 times, most recently from 1e15615 to bfcf283 Compare December 17, 2024 14:08
@@ -123,9 +126,10 @@ TEST_P(CkfSparseTrackTelescopeTests, Run) {
// Finding algorithm configuration
typename traccc::finding_config cfg;
cfg.ptc_hypothesis = ptc;
cfg.chi2_max = 30.f;
cfg.chi2_max = 200.f;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After I set the B field to z-axis and Telescope planes set along the x-axis, chi2 cut value of 30 for the CKF selection did not work for a few measurements :/

I believe the cut value of 30 should be OK in most cases from the distribution of chi2 of the Kalman Fitter (See the plot below). The change to 200 is just to cover the outliers which happen very rarely

@andiwand
Copy link

If I read the ACTS implementation correctly it just rewinds the direction and uses the usual KF formalism but then sets smoothed instead of filtered https://github.com/acts-project/acts/blob/2bb167c0ba1be5c66b55ca76308f8b84e609f489/Core/include/Acts/TrackFitting/KalmanFitter.hpp#L696

That does not seem correct to me 🤔 since we would use measurements twice without accounting this in the covariance.

Have you looked at this @beomki-yeo? I suppose you use the formalism provided in the reference?

cc @asalzburger @pbutti

@beomki-yeo
Copy link
Contributor Author

it just rewinds the direction and uses the usual KF formalism but then sets smoothed instead of filtered

If I read the code correctly, apply_backwards function sets the backward-filtered state to the smoothed state? In this PR, we use the forward-fileted state and backward-predicted state to calculate the smoothed state as stated in the reference.

And It seems the ACTS backward filter does not inflate covariance. I inflated the covariance of both forward and backward propagator. In the various references, I found that there are two boundary conditions:

  1. smoothed-state of the last measurement = forward-filtered state of the last measurement
  2. inverse of initial covariance of backward propagator = zero matrix, which means a very big initial covariance matrix

The second condition sounds a bit weird because there is no inverse matrix for a zero matrix but I interpreted this as a big diagonal initial covariance matrix.

@beomki-yeo
Copy link
Contributor Author

beomki-yeo commented Dec 17, 2024

That does not seem correct to me 🤔 since we would use measurements twice without accounting this in the covariance.

Yeah it seems some measurements are used twice;

Copy link
Contributor

@niermann999 niermann999 left a comment

Choose a reason for hiding this comment

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

OK, lets get this in an fix stuff later

@beomki-yeo beomki-yeo merged commit 8620769 into acts-project:main Dec 19, 2024
29 checks passed
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.

4 participants