-
Notifications
You must be signed in to change notification settings - Fork 168
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: make addSeeding python function smaller #1768
refactor: make addSeeding python function smaller #1768
Conversation
📊 Physics performance monitoring for e03ec22Full report VertexingSeedingCKFAmbiguity resolutionTruth tracking (Kalman Filter)Truth tracking (GSF) |
Codecov Report
@@ Coverage Diff @@
## main #1768 +/- ##
=======================================
Coverage 49.79% 49.79%
=======================================
Files 406 406
Lines 22567 22567
Branches 10307 10307
=======================================
Hits 11238 11238
Misses 4140 4140
Partials 7189 7189 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
I tested all four seeding algorithms with the ITk full chain, and they gave identical printout (eg. performance summary) between this PR and main
.
So apart from the logLevel
comment, I'm happy with these changes.
Re-syncing. Once CI checks out can this be merged? It is needed to continue HT work. |
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.
I was told I should click approve despite me not really being the right reviewer :)
@paulgessinger @asalzburger - can you please allow this to be merged. I wait for this before proceeding with making HT example. |
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.
Let's go
This PR currently has a merge conflict. Please resolve this and then re-add the |
@tboldagh this has picked up conflicts unfortunately. Can you resolve them and then we merge ASAP? |
On it. Seems not to be a small conflict though. |
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.
Thanks, let's get it done.
It is resolved. But the CI fails on lsf checkout. Can one restart CI somehow? |
I think it's a transient failure. I retried the jobs, lets see if they succeed. |
This PR contains changes to the python code configuring seeding. Over time it grew a bit too much.
Tagging @timadye @jahreda