-
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
fix: Align globalToLocal
and intersection
in LineSurface
#2287
fix: Align globalToLocal
and intersection
in LineSurface
#2287
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2287 +/- ##
==========================================
+ Coverage 49.47% 49.49% +0.01%
==========================================
Files 451 451
Lines 25484 25469 -15
Branches 11720 11710 -10
==========================================
- Hits 12609 12605 -4
+ Misses 4582 4581 -1
+ Partials 8293 8283 -10
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
📊 Physics performance monitoring for 5d5c343Summary 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.
thanks for the review @felix-russo !
Co-authored-by: felix-russo <[email protected]>
should be ready now @paulgessinger |
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.
Sorry maybe I'm dumb but from the diff it's not at all clear to me what's actually happening here. You're changing both intersect
and globalToLocal
, and they're not doing the same thing afterwards (and neither did they before I don't think).
What do we want globalToLocal
to do (after this)? Check if we're on the line surface, or check if we're on the PCA?
Co-authored-by: Paul Gessinger <[email protected]>
In particular `LineSurface` after acts-project#2287
after #2321 the conflicts should disappear 🤞 |
This PR currently has a merge conflict. Please resolve this and then re-add the |
Build failure is likely due to #2269, and not this PR. I'll update this PR once it's resolved. |
In particular `LineSurface` after #2287.
This is supposed to be a proper fix after the quick fix in #2239
In
globalToLocal
we did not respect the direction which is important for the line surface.I tried to make the code a little more clear and move away from constructor initialization in the mathy part. I also added a unit test to check the intersection against the propagation.
I will let the CI judge how much this breaks