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

Implement Combinatorial Kalman Filter #102

Merged
merged 59 commits into from
Apr 23, 2020

Conversation

XiaocongAi
Copy link
Contributor

This MR includes the following:

  • Implement the Combinatorial Kalman Filter for track finding&fitting
  • Implement the source link selector to select the measurements/outlier
  • Implement the branch stopper plugin to allow stopping bad branch

@XiaocongAi XiaocongAi added the Feature Development to integrate a new feature label Apr 13, 2020
@acts-issue-bot acts-issue-bot bot removed the Triage label Apr 13, 2020
@XiaocongAi XiaocongAi changed the title Kf with track finding Implement Combinatorial Kalman Filter Apr 13, 2020
@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #102 into master will decrease coverage by 1.52%.
The diff coverage is 29.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
- Coverage   44.50%   42.98%   -1.53%     
==========================================
  Files         357      354       -3     
  Lines       17592    17147     -445     
  Branches     8260     8130     -130     
==========================================
- Hits         7830     7371     -459     
- Misses       4807     4824      +17     
+ Partials     4955     4952       -3     
Impacted Files Coverage Δ
...cts/TrackFinder/CombinatorialKalmanFilterError.hpp 0.00% <0.00%> (ø)
...ude/Acts/TrackFinder/CombinatorialKalmanFilter.hpp 27.32% <27.32%> (ø)
...include/Acts/TrackFinder/CKFSourceLinkSelector.hpp 41.57% <41.57%> (ø)
...s/TrackFinder/detail/VoidTrackFinderComponents.hpp 100.00% <100.00%> (ø)
Core/src/Geometry/BinUtility.cpp 0.00% <0.00%> (-100.00%) ⬇️
Core/include/Acts/Propagator/AtlasStepper.hpp 18.37% <0.00%> (-50.68%) ⬇️
...Propagator/detail/PointwiseMaterialInteraction.cpp 87.50% <0.00%> (-12.50%) ⬇️
Core/include/Acts/Utilities/BinUtility.hpp 39.65% <0.00%> (-5.18%) ⬇️
Core/src/Propagator/detail/CovarianceEngine.cpp 45.23% <0.00%> (-2.27%) ⬇️
Core/include/Acts/Propagator/EigenStepper.ipp 35.77% <0.00%> (-2.19%) ⬇️
... and 45 more

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 5770f23...5770f23. Read the comment docs.

@paulgessinger
Copy link
Member

This looks like a ton of work! Had a quick look and I think it looks really good. I'll try to have a closer look as soon as possible, but if anyone else wants to have a look, go ahead.

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.

Sorry for the tons of comments, but it's a huge amount of code. Looks like awesome work! Will approve once we resolve the discussion items.

@paulgessinger paulgessinger modified the milestones: v0.22.00, v0.23.00 Apr 17, 2020
@XiaocongAi XiaocongAi force-pushed the kf_with_track_finding branch 2 times, most recently from 7aac56a to 8e28e01 Compare April 20, 2020 08:25
@asalzburger
Copy link
Contributor

I think the unordered_map misses an #include statement, as the macOS build fails.

@XiaocongAi XiaocongAi force-pushed the kf_with_track_finding branch from 8858c14 to e504bb0 Compare April 21, 2020 09:39
@XiaocongAi
Copy link
Contributor Author

@paulgessinger Thank you for your good suggestions for this PR! I have updated/optimized the code as much as possible. Will rebase it tomorrow.

@XiaocongAi XiaocongAi force-pushed the kf_with_track_finding branch from e504bb0 to 7eb0aa8 Compare April 22, 2020 07:45
@paulgessinger
Copy link
Member

Approval is out!
The build failure in the coverage job is "No space left on device", which is hopefully transient. I hit retry, let's see.

@paulgessinger
Copy link
Member

The CI fails because it points a commit that was most likely force pushed away. @XiaocongAi can you rebase and/or push an empty commit to trigger a complete pipeline?

@asalzburger
Copy link
Contributor

@XiaocongAi - if you rebase & push, I will merge in later.

@XiaocongAi
Copy link
Contributor Author

XiaocongAi commented Apr 22, 2020

@asalzburger . it's rebased. But the CI fails due to "No space left on device". How could I deal with this?

@paulgessinger
Copy link
Member

@asalzburger . it's rebased. But the CI fails due to "No space left on device". How could I deal with this?

Hm, I saw this message before, but if I check the log now I still see

2020-04-22T20:14:16.2364578Z ##[error]fatal: reference is not a tree: 0b4df649b842be6dff35af7e70483db3c65ff084

That is not the tip of the branch however, I believe. Not sure why that job tries to check that ref out.

@paulgessinger paulgessinger merged commit 2a9b326 into acts-project:master Apr 23, 2020
@paulgessinger
Copy link
Member

I override the coverage failure, because I think it's a technical problem, which I don't want to hold this PR back for. (See #124)

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

Successfully merging this pull request may close these issues.

3 participants