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

Remove TrackState #289

Merged
merged 11 commits into from
Jul 2, 2020
Merged

Conversation

FabianKlimpel
Copy link
Contributor

@FabianKlimpel FabianKlimpel commented Jun 26, 2020

The TrackState is not in use anymore and became superseded by the MultiTrajectory. Herewith I propose with the PR to remove it from the code base.

@FabianKlimpel FabianKlimpel self-assigned this Jun 26, 2020
@FabianKlimpel FabianKlimpel requested a review from XiaocongAi June 26, 2020 09:16
@FabianKlimpel FabianKlimpel added this to the v0.28.00 milestone Jun 26, 2020
@FabianKlimpel FabianKlimpel added Component - Core Affects the Core module Impact - Minor Nuissance bug and/or affects only a single module Needs Decision Needs decision or further information and removed Triage labels Jun 26, 2020
@FabianKlimpel FabianKlimpel added Improvement Changes to an existing feature and removed Triage labels Jun 26, 2020
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 I think generally this is fine, definitely want @XiaocongAi's opinion on this though.

Copy link
Contributor

@msmk0 msmk0 left a comment

Choose a reason for hiding this comment

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

Good idea. I really like removing unused code. Most comments are clarifications or style changes only.

@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #289 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
- Coverage   48.33%   48.26%   -0.08%     
==========================================
  Files         325      323       -2     
  Lines       16444    16367      -77     
  Branches     7643     7606      -37     
==========================================
- Hits         7949     7900      -49     
+ Misses       3183     3181       -2     
+ Partials     5312     5286      -26     
Impacted Files Coverage Δ
Core/include/Acts/EventData/MultiTrajectory.hpp 71.42% <ø> (+0.84%) ⬆️
Core/include/Acts/EventData/MultiTrajectory.ipp 70.17% <ø> (+3.50%) ⬆️
Core/include/Acts/Fitter/KalmanFitter.hpp 36.23% <ø> (ø)
...ude/Acts/TrackFinder/CombinatorialKalmanFilter.hpp 26.26% <ø> (ø)
Core/include/Acts/EventData/Measurement.hpp 53.19% <0.00%> (-10.64%) ⬇️
...lude/Acts/EventData/SingleBoundTrackParameters.hpp 72.09% <0.00%> (-2.33%) ⬇️

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 17f1fa9...6a615e8. Read the comment docs.

@FabianKlimpel FabianKlimpel force-pushed the RemoveTrackState branch 2 times, most recently from 9169b8a to 47b5023 Compare June 30, 2020 10:12
@paulgessinger
Copy link
Member

Let's hold this for @XiaocongAi to have a look.

@msmk0 msmk0 removed the Needs Decision Needs decision or further information label Jun 30, 2020
@msmk0
Copy link
Contributor

msmk0 commented Jul 1, 2020

@XiaocongAi Could you have a look at this and let us know if you see any blockers or whether we can merge it?

@XiaocongAi
Copy link
Contributor

This looks good to me. I will approve.

@msmk0 msmk0 merged commit c8dfa61 into acts-project:master Jul 2, 2020
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 13, 2020
The TrackState is not in use anymore and became superseded by the MultiTrajectory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Impact - Minor Nuissance bug and/or affects only a single module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants