feat: Marking Edgeholes during track finding #3988
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Core
| bool inside(const Vector2& lposition, double tolR, | ||
| double tolPhiR, | ||
| double scale) const; |
There was a problem hiding this comment.
I think this can be replaced by a proper implementation of the inside check given a tolerance
There was a problem hiding this comment.
passing the "scale" parameter avoids a division. But anyway the way it is implemented duplicates code. So, as mentioned at a different place. This was the easiest way to implement it and was anyway more meant to be a proof of concept. And so far we only see that the concept does not work all that great ;-)
| static const BoundaryTolerance exclude_sensor_border | ||
| = BoundaryTolerance(BoundaryTolerance::AbsoluteCartesian{-2*UnitConstants::mm,-2*UnitConstants::mm}); | ||
| if (currentState.referenceSurface().insideBounds(currentState.parameters().template head<2>(), | ||
| exclude_sensor_border)) { | ||
| currentBranch.nHoles()++; | ||
| } | ||
| else { | ||
| if (!keepEdgeHoles) { | ||
| currentBranch.nHoles()++; | ||
| } | ||
| else { | ||
| auto typeFlags = currentState.typeFlags(); | ||
| typeFlags.reset(TrackStateFlag::HoleFlag); | ||
| typeFlags.set(TrackStateFlag::EdgeHoleFlag); | ||
| //currentBranch.nEdgeHoles()++; | ||
| } | ||
| } |
There was a problem hiding this comment.
IMO this is an experiment choice and it should not be part of the Core CKF. This can be put into the track state creator to put the user in control
There was a problem hiding this comment.
IMO this is an experiment choice and it should not be part of the Core CKF. This can be put into the track state creator to put the user in control
Not sure whether there is a universally accepted definition of what a hole is. Is it really a how if the edge is within the uncertainties of the trajectory ? It is a definition, but I guess one can argue either way, whether it should be counted as a hole or not. Sure an ad hoc 2mm tolerance is not universal.
| if (boundaryTolerance.hasAbsoluteCartesian()) { | ||
|
|
||
| const BoundaryTolerance::AbsoluteCartesian& tolerance = boundaryTolerance.asAbsoluteCartesian(); | ||
| return lposition(0,0) > (m_min.x()-tolerance.tolerance0) | ||
| && lposition(0,0) < (m_max.x()+tolerance.tolerance0) | ||
| && lposition(1,0) > (m_min.y()-tolerance.tolerance1) | ||
| && lposition(1,0) < (m_max.y()+tolerance.tolerance1); | ||
| } | ||
|
|
There was a problem hiding this comment.
@goetzgaycken Can you comment on this?
There was a problem hiding this comment.
It was just easier to implement the negative tolerance, this way then trying to add it to the more generic implementation.
Btw. the implementation was not meant for a PR, it was more a proof of concept.
There was a problem hiding this comment.
Doesn't insideAlignedBox do exactly the same thing in a more general way?
There was a problem hiding this comment.
- I have to admit that I do not understand how the method handles the case that the point is inside the polygon. Will computeClosestPointOnPolygon return the point itself if it is inside the polygon ? If not than I would not understand how the tolerance works .
- anyway currently tolerance.isTolerated() does not handle a negative tolerance, and the passed distance does not carry any information whether the point is inside or outside unless "inside" will always yield a zero distance, but then it would not be possible to handle a negative tolerance.
- far more complicated than this simple set of 4 comparisons.
Added track state type for edge holes.
Added marking of track states as edge holes in CKF Actor with 2mm tolerance (to be configured/changed)
Currently option is propagated via propagator plain options (to be changed).
Tried to add nEdgeHoles to VectorTrackContainer but fails.
--- END COMMIT MESSAGE ---
@andiwand @paulgessinger
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.