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: Use material before the first measurement in smoothing #956

Merged
merged 15 commits into from
Aug 25, 2021

Conversation

Corentin-Allaire
Copy link
Contributor

@Corentin-Allaire Corentin-Allaire commented Aug 23, 2021

This PR allow material situated before the first measurement (eg : beampipe) to be properly accounted for in the KF and CKF. This will fix issue #906

@Corentin-Allaire Corentin-Allaire added this to the next milestone Aug 23, 2021
@Corentin-Allaire Corentin-Allaire added Bug Something isn't working Component - Core Affects the Core module Impact - Major Significant bug and/or affects a lot of modules labels Aug 23, 2021
@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #956 (c3485ea) into main (0123060) will decrease coverage by 0.01%.
The diff coverage is 39.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #956      +/-   ##
==========================================
- Coverage   48.47%   48.46%   -0.02%     
==========================================
  Files         334      334              
  Lines       17059    17067       +8     
  Branches     8069     8074       +5     
==========================================
+ Hits         8270     8271       +1     
- Misses       3088     3094       +6     
- Partials     5701     5702       +1     
Impacted Files Coverage Δ
...de/Acts/TrackFinding/CombinatorialKalmanFilter.hpp 29.94% <29.41%> (-0.23%) ⬇️
Core/include/Acts/TrackFitting/KalmanFitter.hpp 37.83% <54.54%> (-0.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0123060...c3485ea. Read the comment docs.

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.

Seems fine to me, but let's wait for @XiaocongAi's review.

Copy link
Contributor

@XiaocongAi XiaocongAi left a comment

Choose a reason for hiding this comment

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

Hi @Corentin-Allaire , thank you for the PR. I have only one comment.
In the CKF, shall we do the following:?

  1. If we are going to run smoothing to get the fitted track parameters, we do the same as in KF, i.e. we create only material state before the first measuement and create measurement/hole/material after the first measurement;
  2. If we are don't care about the track fitting, i.e. the CKF is used just for track finding, we only create measurement/hole state after the first measurement.

Only create pre-measurement material state in fitting mode

Co-authored-by: Xiaocong Ai <xiaocong.ai@cern.ch>
@Corentin-Allaire
Copy link
Contributor Author

Hi @Corentin-Allaire , thank you for the PR. I have only one comment.
In the CKF, shall we do the following:?

1. If we are going to run smoothing to get the fitted track parameters, we do the same as in KF, i.e. we create only material state before the first measuement and create measurement/hole/material after the first measurement;

2. If we are don't care about the track fitting, i.e. the CKF is used just for track finding, we only create measurement/hole state after the first measurement.

I think this is a great idea, I merged your suggestion.

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.

Ok this seems fine now. Let's merge it.

@paulgessinger paulgessinger merged commit 72eecda into acts-project:main Aug 25, 2021
@paulgessinger paulgessinger modified the milestones: next, v12.0.0 Aug 26, 2021
asalzburger pushed a commit that referenced this pull request Sep 20, 2021
The reason for #984 is that after #956, a track might have non-zero processedStates, but zero measurementStates. So we return a failure explicity in KF if measurementStates is zero.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Component - Core Affects the Core module Impact - Major Significant bug and/or affects a lot of modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kalman Filtter doesn't account properly for the material in the smoothing
3 participants