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.

Sign up for GitHub

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

feat: Marking Edgeholes during track finding #3988

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

feat: Marking Edgeholes during track finding #3988

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4f0fe37
Add boundary tolerance for intersection tests and inverse tolerance f…
Nov 25, 2024
ad0e897
Restore no check on boundaries in navigator
pbutti Dec 3, 2024
e9a20e6
Merge branch 'acts-project:main' into fix-edgeholes
pbutti Dec 5, 2024
5cb0ada
Merge branch 'acts-project:main' into fix-edgeholes
pbutti Dec 5, 2024
29ed187
Merge branch 'acts-project:main' into fix-edgeholes
pbutti Dec 9, 2024
fbc3b2f
Support for skipping edge holes
pbutti Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Core/include/Acts/Propagator/Navigator.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ namespace Acts {
template <typename object_t>
struct NavigationOptions {
/// The boundary check directive
BoundaryTolerance boundaryTolerance = BoundaryTolerance::None();
BoundaryTolerance boundaryTolerance = BoundaryTolerance::AbsoluteCartesian{2*UnitConstants::mm,2*UnitConstants::mm};
pbutti marked this conversation as resolved.
Show resolved Hide resolved

// How to resolve the geometry
/// Always look for sensitive
Expand Down
7 changes: 6 additions & 1 deletion Core/include/Acts/Surfaces/AnnulusBounds.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ class AnnulusBounds : public DiscBounds {
/// @return boolean indicator for the success of this operation
virtual bool inside(const Vector2& lposition, double tolR,
double tolPhi) const final;

/// Transform the strip cartesian
/// into the module polar system
///
Expand All @@ -209,6 +209,11 @@ class AnnulusBounds : public DiscBounds {

/// Private helper method
double squaredNorm(const Vector2& v, const SquareMatrix2& weight) const;

private:
bool inside(const Vector2& lposition, double tolR,
double tolPhiR,
double scale) const;
Comment on lines +214 to +216
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be replaced by a proper implementation of the inside check given a tolerance

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ;-)

};

inline SurfaceBounds::BoundsType AnnulusBounds::type() const {
Expand Down
7 changes: 6 additions & 1 deletion Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,12 @@ class CombinatorialKalmanFilter {
currentBranch.tipIndex() = currentTip;
auto currentState = currentBranch.outermostTrackState();
if (expectMeasurements) {
currentBranch.nHoles()++;
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()++;
}
}

BranchStopperResult branchStopperResult =
Expand Down
44 changes: 44 additions & 0 deletions Core/src/Surfaces/AnnulusBounds.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,44 @@ bool Acts::AnnulusBounds::inside(const Vector2& lposition, double tolR,
return true;
}

bool Acts::AnnulusBounds::inside(const Vector2& lposition, double tolR,
double tolPhiR,
double scale) const {
// locpo is PC in STRIP SYSTEM
// need to perform internal rotation induced by average phi
Vector2 locpo_rotated = m_rotationStripPC * lposition;
double phiLoc = locpo_rotated[eBoundLoc1];
double phiLocR = phiLoc*scale;
double rLoc = locpo_rotated[eBoundLoc0];

if (phiLocR < (get(eMinPhiRel)*scale - tolPhiR) ||
phiLocR > (get(eMaxPhiRel)*scale + tolPhiR)) {
return false;
}

// calculate R in MODULE SYSTEM to evaluate R-bounds
if (tolR == 0.) {
// don't need R, can use R^2
double r_mod2 =
m_shiftPC[eBoundLoc0] * m_shiftPC[eBoundLoc0] + rLoc * rLoc +
2 * m_shiftPC[eBoundLoc0] * rLoc * cos(phiLoc - m_shiftPC[eBoundLoc1]);

if (r_mod2 < get(eMinR) * get(eMinR) || r_mod2 > get(eMaxR) * get(eMaxR)) {
return false;
}
} else {
// use R
double r_mod = sqrt(
m_shiftPC[eBoundLoc0] * m_shiftPC[eBoundLoc0] + rLoc * rLoc +
2 * m_shiftPC[eBoundLoc0] * rLoc * cos(phiLoc - m_shiftPC[eBoundLoc1]));

if (r_mod < (get(eMinR) - tolR) || r_mod > (get(eMaxR) + tolR)) {
return false;
}
}
return true;
}

bool Acts::AnnulusBounds::inside(
const Vector2& lposition,
const BoundaryTolerance& boundaryTolerance) const {
Expand All @@ -181,6 +219,12 @@ bool Acts::AnnulusBounds::inside(
if (boundaryTolerance.isNone()) {
return inside(lposition, 0., 0.);
}
if (boundaryTolerance.hasAbsoluteCartesian()) {
const double r = lposition(0,0);
const BoundaryTolerance::AbsoluteCartesian& tolerance = boundaryTolerance.asAbsoluteCartesian();
return inside(lposition, tolerance.tolerance0,
tolerance.tolerance1,r);
}

if (auto absoluteBound = boundaryTolerance.asAbsoluteBoundOpt();
absoluteBound.has_value()) {
Expand Down
9 changes: 9 additions & 0 deletions Core/src/Surfaces/RectangleBounds.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@
bool Acts::RectangleBounds::inside(
const Acts::Vector2& lposition,
const Acts::BoundaryTolerance& boundaryTolerance) const {
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);
}

Comment on lines +19 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goetzgaycken Can you comment on this?

Copy link
Contributor

@goetzgaycken goetzgaycken Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@paulgessinger paulgessinger Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't insideAlignedBox do exactly the same thing in a more general way?

Copy link
Contributor

@goetzgaycken goetzgaycken Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  • 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.

return detail::insideAlignedBox(m_min, m_max, boundaryTolerance, lposition,
std::nullopt);
}
Expand Down