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: Refactor and fix component merging for GSF #1364

Merged
merged 32 commits into from
Oct 5, 2022

Conversation

benjaminhuth
Copy link
Member

@benjaminhuth benjaminhuth commented Aug 3, 2022

Fixes a bug in the component merging where the covariance was computed without respecting cyclic coordinates
This did blow up the variance in some cases and did lead to change of direction for some components.

Additional to this, now the surface-type dependent behavior (with the AngleDescription) should be applied in all places where the component merging is used.

This also completely redid the unit test for the component merging, that should do a better job in testing the functionality.

@benjaminhuth benjaminhuth added Bug Something isn't working Component - Core Affects the Core module 🚧 WIP Work-in-progress labels Aug 3, 2022
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #1364 (a0b2ca9) into main (504882e) will increase coverage by 0.10%.
The diff coverage is 60.29%.

@@            Coverage Diff             @@
##             main    #1364      +/-   ##
==========================================
+ Coverage   48.47%   48.58%   +0.10%     
==========================================
  Files         381      381              
  Lines       20699    20727      +28     
  Branches     9503     9497       -6     
==========================================
+ Hits        10034    10070      +36     
+ Misses       4112     4110       -2     
+ Partials     6553     6547       -6     
Impacted Files Coverage Δ
Core/include/Acts/TrackFitting/detail/GsfActor.hpp 43.52% <0.00%> (+0.94%) ⬆️
.../include/Acts/TrackFitting/detail/GsfSmoothing.hpp 35.57% <27.27%> (+0.52%) ⬆️
.../include/Acts/Propagator/MultiEigenStepperLoop.ipp 43.68% <60.00%> (-0.43%) ⬇️
...de/Acts/TrackFitting/detail/KLMixtureReduction.hpp 59.74% <66.66%> (ø)
...Acts/Utilities/detail/gaussian_mixture_helpers.hpp 69.64% <68.75%> (+51.78%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@benjaminhuth benjaminhuth added this to the WIP milestone Aug 4, 2022
@benjaminhuth benjaminhuth removed the 🚧 WIP Work-in-progress label Aug 11, 2022
@benjaminhuth benjaminhuth modified the milestones: WIP, next Aug 11, 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.

Makes sense to me.

@kodiakhq kodiakhq bot merged commit c27fa44 into acts-project:main Oct 5, 2022
@github-actions github-actions bot removed the automerge label Oct 5, 2022
@paulgessinger paulgessinger modified the milestones: next, v20.3.0 Oct 11, 2022
@paulgessinger paulgessinger added the backport develop/v19.x Backport this PR to the v19.x series label Oct 11, 2022
acts-project-service pushed a commit that referenced this pull request Oct 11, 2022
Fixes a bug in the component merging where the covariance was computed without respecting cyclic coordinates
This did blow up the variance in some cases and did lead to change of direction for some components.

Additional to this, now the surface-type dependent behavior (with the `AngleDescription`) should be applied in all places where the component merging is used.

This also completely redid the unit test for the component merging, that should do a better job in testing the functionality.

(cherry picked from commit c27fa44)
paulgessinger pushed a commit that referenced this pull request Oct 13, 2022
…velop/v19.x] (#1592)

Fixes a bug in the component merging where the covariance was computed
without respecting cyclic coordinates
This did blow up the variance in some cases and did lead to change of
direction for some components.

Additional to this, now the surface-type dependent behavior (with the
`AngleDescription`) should be applied in all places where the component
merging is used.

This also completely redid the unit test for the component merging, that
should do a better job in testing the functionality.

Co-authored-by: Benjamin Huth <37871400+benjaminhuth@users.noreply.github.com>
@benjaminhuth benjaminhuth deleted the fix/direction-change-in-gsf branch February 22, 2023 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport develop/v19.x Backport this PR to the v19.x series Bug Something isn't working Component - Core Affects the Core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants