-
Notifications
You must be signed in to change notification settings - Fork 177
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: and fix truth seeding #1897
refactor: and fix truth seeding #1897
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1897 +/- ##
=======================================
Coverage 49.44% 49.44%
=======================================
Files 408 408
Lines 22704 22704
Branches 10371 10371
=======================================
Hits 11226 11226
Misses 4266 4266
Partials 7212 7212 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
📊 Physics performance monitoring for d74905fFull report VertexingSeedingCKFAmbiguity resolutionTruth tracking (Kalman Filter)Truth tracking (GSF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice changes, I also always had the impression that thisTrackParamEstimationAlgorithm
served way to many purposes! I just have one minor comment about naming.
Examples/Algorithms/TruthTracking/ActsExamples/TruthTracking/TruthSeedingAlgorithm.hpp
Show resolved
Hide resolved
I think we should try to make this refactoring in a way that the hashes stay the same, right? Currently quite a lot of hash checks still fail. |
will try to do that. physmon looks also kind of sketchy. but I wonder if we also did 3 hit fitting in the tests |
If we do 3 hit fitting, maybe change that in a follow-up PR? that way we can be sure that we have no change in behaviour in this refactoring... |
almost done - I may remove the proto tracks from the seeds in a follow up PR now it should be down to physmon which is picking on the truth estimated seeding efficiency. I believe we measured this in a wrong way which resulted in a 100% efficiency but it is actually a little lower than that because we apply some cuts in the truth seeding |
this should be ready now @benjaminhuth @asalzburger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice refactoring, let's get it in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, makes perfect sense.
truth seeding seems to be quite broken at the moment
with this PR I tried to improve things by splitting the truth seeding out of
TrackParamsEstimationAlgorithm
and restore its original purpose. I addedTruthSeedingAlgorithm
which now also doesTruthTrackFinder
in order to keep seeded particles and track params aligned