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!: Remove smoothing and extrapolation from core CKF #2722

Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Nov 23, 2023

After #2539 I want to finally refactor the CKF and remove the smoothing and the reference surface targeting from the core algorithm in order to make it more composable and less complicated.

Before the CKF was basically only usable with smoothing turned on otherwise we would not receive any tracks. Now it always runs without smoothing and the user can decide what and how to smooth. Afterwards the user can decide which track state they want to extrapolate to a reference surface if necessary.

IMO this makes the algorithm flow much easier to comprehend and gives the user more flexibility. This also makes an implementation of a two way finding easier as discovered in #2717

blocked by


Users can smooth and extrapolate tracks after the CKF returns them. Extrapolation can be achieved using the Propagator while smoothing can be done by the GainMatrixSmoother.

For convenience this PR also includes helper functions to smooth and extrapolate single tracks or a whole track container.

Example of smoothing and extrapolating tracks after the CKF

auto smoothingResult =
Acts::smoothTrack(ctx.geoContext, tracksTemp, track, logger());
if (!smoothingResult.ok()) {
ACTS_ERROR("Smoothing for seed "
<< iseed << " and track " << track.index()
<< " failed with error " << smoothingResult.error());
continue;
}
auto extrapolationResult = Acts::extrapolateTrackToReferenceSurface(
track, *pSurface, extrapolator, extrapolationOptions,
m_cfg.extrapolationStrategy, logger());
if (!extrapolationResult.ok()) {
ACTS_ERROR("Extrapolation for seed "
<< iseed << " and track " << track.index()
<< " failed with error " << extrapolationResult.error());
continue;
}

@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Track Finding labels Nov 23, 2023
Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Attention: Patch coverage is 9.67742% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 49.01%. Comparing base (9e72306) to head (8e11c91).

Files Patch % Lines
...de/Acts/TrackFinding/CombinatorialKalmanFilter.hpp 15.00% 12 Missing and 5 partials ⚠️
Core/src/Utilities/TrackHelpers.cpp 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2722      +/-   ##
==========================================
+ Coverage   48.99%   49.01%   +0.02%     
==========================================
  Files         490      491       +1     
  Lines       28996    28886     -110     
  Branches    13743    13685      -58     
==========================================
- Hits        14206    14159      -47     
+ Misses       4951     4938      -13     
+ Partials     9839     9789      -50     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andiwand andiwand added this to the v32.0.0 milestone Nov 23, 2023
@andiwand andiwand marked this pull request as ready for review November 27, 2023 10:34
kodiakhq bot pushed a commit that referenced this pull request Nov 27, 2023
…tion (#2723)

In #2722 I realized that the noise mode in the `MaterialInteractor` depends on the propagation direction which is unexpected IMO. Usually we do not want to remove noise in backward mode as this should still increase uncertainty while extrapolating.
@andiwand andiwand changed the title refactor!: Remove smoothing and extrapolation from CKF refactor!: Remove smoothing and extrapolation from core CKF Jan 11, 2024
@AJPfleger
Copy link
Contributor

Please add diff to description as discussed

@paulgessinger paulgessinger modified the milestones: v32.0.0, v33.0.0 Jan 19, 2024
@andiwand andiwand marked this pull request as draft January 25, 2024 09:03
@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Mar 27, 2024
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.

Let's go!

@kodiakhq kodiakhq bot merged commit d9f775f into acts-project:main Mar 28, 2024
52 checks passed
@andiwand andiwand deleted the remove-ckf-smoothing-and-extrapolation branch March 28, 2024 09:45
kodiakhq bot pushed a commit that referenced this pull request Apr 4, 2024
While working on the CKF I encountered the problem that track state components need to be created when the track state is created. Since the CKF will not smooth anymore after #2722 the user might want to add smoothed states to the existing track states without reallocating and copying all the existing states.

In case of the vector implementation the components will not be next to each other in the memory.
kodiakhq bot pushed a commit that referenced this pull request Apr 9, 2024
Since the CKF is not responsible for smoothing anymore it should not allocate these components on the track state. #2722
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Apr 19, 2024
…ct#3075)

While working on the CKF I encountered the problem that track state components need to be created when the track state is created. Since the CKF will not smooth anymore after acts-project#2722 the user might want to add smoothed states to the existing track states without reallocating and copying all the existing states.

In case of the vector implementation the components will not be next to each other in the memory.
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Apr 19, 2024
…s-project#3086)

Since the CKF is not responsible for smoothing anymore it should not allocate these components on the track state. acts-project#2722
@andiwand andiwand modified the milestones: next, v34.0.0 Apr 25, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…ject#2722)

After acts-project#2539 I want to finally refactor the CKF and remove the smoothing and the reference surface targeting from the core algorithm in order to make it more composable and less complicated.

Before the CKF was basically only usable with smoothing turned on otherwise we would not receive any tracks. Now it always runs without smoothing and the user can decide what and how to smooth. Afterwards the user can decide which track state they want to extrapolate to a reference surface if necessary.

IMO this makes the algorithm flow much easier to comprehend and gives the user more flexibility. This also makes an implementation of a two way finding easier as discovered in acts-project#2717

blocked by
- acts-project#2723

---

Users can smooth and extrapolate tracks after the CKF returns them. Extrapolation can be achieved using the `Propagator` while smoothing can be done by the `GainMatrixSmoother`.

For convenience this PR also includes helper functions to smooth and extrapolate single tracks or a whole track container.

Example of smoothing and extrapolating tracks after the CKF

https://github.com/acts-project/acts/blob/98186891d4bdc5b211f6af3e96ca59ace7c315cc/Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp#L172-L189
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…ct#3075)

While working on the CKF I encountered the problem that track state components need to be created when the track state is created. Since the CKF will not smooth anymore after acts-project#2722 the user might want to add smoothed states to the existing track states without reallocating and copying all the existing states.

In case of the vector implementation the components will not be next to each other in the memory.
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…s-project#3086)

Since the CKF is not responsible for smoothing anymore it should not allocate these components on the track state. acts-project#2722
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…ject#2722)

After acts-project#2539 I want to finally refactor the CKF and remove the smoothing and the reference surface targeting from the core algorithm in order to make it more composable and less complicated.

Before the CKF was basically only usable with smoothing turned on otherwise we would not receive any tracks. Now it always runs without smoothing and the user can decide what and how to smooth. Afterwards the user can decide which track state they want to extrapolate to a reference surface if necessary.

IMO this makes the algorithm flow much easier to comprehend and gives the user more flexibility. This also makes an implementation of a two way finding easier as discovered in acts-project#2717

blocked by
- acts-project#2723

---

Users can smooth and extrapolate tracks after the CKF returns them. Extrapolation can be achieved using the `Propagator` while smoothing can be done by the `GainMatrixSmoother`.

For convenience this PR also includes helper functions to smooth and extrapolate single tracks or a whole track container.

Example of smoothing and extrapolating tracks after the CKF

https://github.com/acts-project/acts/blob/98186891d4bdc5b211f6af3e96ca59ace7c315cc/Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp#L172-L189
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…ct#3075)

While working on the CKF I encountered the problem that track state components need to be created when the track state is created. Since the CKF will not smooth anymore after acts-project#2722 the user might want to add smoothed states to the existing track states without reallocating and copying all the existing states.

In case of the vector implementation the components will not be next to each other in the memory.
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…s-project#3086)

Since the CKF is not responsible for smoothing anymore it should not allocate these components on the track state. acts-project#2722
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module Event Data Model Infrastructure Changes to build tools, continous integration, ... Track Finding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants