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.

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: Backport navigation rewrite changes #2846

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
32d17ee
backport navigation rewrite changes
andiwand Dec 20, 2023
31a37cf
sort candidates
andiwand Dec 20, 2023
f9eeffc
couple of things
andiwand Dec 20, 2023
040e1e9
use far limit again
andiwand Dec 20, 2023
ea79b4f
recover existing navigation fixes
andiwand Dec 21, 2023
c94f6b0
add todo
andiwand Dec 21, 2023
90fc498
pr feedback
andiwand Dec 21, 2023
9a57cb3
Merge branch 'main' of https://github.com/acts-project/acts into back…
andiwand Dec 22, 2023
09eed3a
fix after merging main
andiwand Dec 22, 2023
3d362c0
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Jan 9, 2024
0892b89
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Jan 22, 2024
4d6763d
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Jan 24, 2024
7b31ef5
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Feb 27, 2024
8a61d6d
Apply suggestions from code review
andiwand Feb 27, 2024
ea4c866
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Mar 15, 2024
108ae3f
pr feedback; revive nav test
andiwand Mar 15, 2024
af6a47b
Apply suggestions from code review
andiwand Mar 15, 2024
f7b77a4
Apply suggestions from code review
andiwand Mar 15, 2024
ca0ee8a
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Mar 16, 2024
87d9209
update references
andiwand Mar 18, 2024
4f7a764
Merge branch 'main' of github.com:acts-project/acts into backport-nav…
andiwand Apr 30, 2024
5f483bb
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand May 16, 2024
5661306
Merge branch 'main' of github.com:acts-project/acts into backport-nav…
andiwand May 22, 2024
0d02fa9
Merge branch 'main' of github.com:acts-project/acts into backport-nav…
andiwand May 24, 2024
7fbd681
reintroduce far limit constraint in `SteppingHelper`
andiwand May 24, 2024
d2f46cd
update references
andiwand May 24, 2024
8b80a34
Merge branch 'main' into backport-navigation-rewrite-changes
AJPfleger May 24, 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
Binary file modified CI/physmon/reference/performance_ambi_ttbar.root
Binary file not shown.
Binary file modified CI/physmon/reference/performance_amvf_gridseeder_ttbar_hist.root
Binary file not shown.
Binary file modified CI/physmon/reference/performance_amvf_ttbar_hist.root
Binary file not shown.
Binary file modified CI/physmon/reference/performance_ckf_ttbar.root
Binary file not shown.
Binary file modified CI/physmon/reference/performance_gsf.root
Binary file not shown.
Binary file modified CI/physmon/reference/performance_seeding_ttbar.root
Binary file not shown.
Binary file modified CI/physmon/reference/tracksummary_ckf_ttbar_hist.root
Binary file not shown.
2 changes: 1 addition & 1 deletion Core/include/Acts/Geometry/Layer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class VolumeBounds;
class TrackingVolume;
class ApproachDescriptor;
class IMaterialDecorator;
template <typename T>
template <typename object_t>
struct NavigationOptions;

// Simple surface intersection
Expand Down
12 changes: 1 addition & 11 deletions Core/include/Acts/Propagator/EigenStepper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,7 @@ class EigenStepper {
};

/// Constructor requires knowledge of the detector's magnetic field
EigenStepper(std::shared_ptr<const MagneticFieldProvider> bField,
double overstepLimit = 100 * UnitConstants::um);
EigenStepper(std::shared_ptr<const MagneticFieldProvider> bField);

State makeState(std::reference_wrapper<const GeometryContext> gctx,
std::reference_wrapper<const MagneticFieldContext> mctx,
Expand Down Expand Up @@ -317,12 +316,6 @@ class EigenStepper {
return state.stepSize.toString();
}

/// Overstep limit
double overstepLimit(const State& /*state*/) const {
// A dynamic overstep limit could sit here
return -m_overstepLimit;
}

/// Create and return the bound state at the current position
///
/// @brief This transports (if necessary) the covariance
Expand Down Expand Up @@ -435,9 +428,6 @@ class EigenStepper {
protected:
/// Magnetic field inside of the detector
std::shared_ptr<const MagneticFieldProvider> m_bField;

/// Overstep limit
double m_overstepLimit{};
};

template <typename navigator_t>
Expand Down
4 changes: 2 additions & 2 deletions Core/include/Acts/Propagator/EigenStepper.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@

template <typename E, typename A>
Acts::EigenStepper<E, A>::EigenStepper(
std::shared_ptr<const MagneticFieldProvider> bField, double overstepLimit)
: m_bField(std::move(bField)), m_overstepLimit(overstepLimit) {}
std::shared_ptr<const MagneticFieldProvider> bField)
: m_bField(std::move(bField)) {}

template <typename E, typename A>
auto Acts::EigenStepper<E, A>::makeState(
Expand Down
8 changes: 0 additions & 8 deletions Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -726,14 +726,6 @@ class MultiEigenStepperLoop
return ss.str();
}

/// Overstep limit
///
/// @param state [in] The stepping state (thread-local cache)
double overstepLimit(const State& state) const {
// A dynamic overstep limit could sit here
return SingleStepper::overstepLimit(state.components.front().state);
}

/// Create and return the bound state at the current position
///
/// @brief This transports (if necessary) the covariance
Expand Down
70 changes: 43 additions & 27 deletions Core/include/Acts/Propagator/Navigator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#pragma once

#include "Acts/Definitions/Units.hpp"
#include "Acts/Geometry/BoundarySurfaceT.hpp"
#include "Acts/Geometry/GeometryIdentifier.hpp"
#include "Acts/Geometry/Layer.hpp"
#include "Acts/Geometry/TrackingGeometry.hpp"
Expand All @@ -19,8 +18,6 @@
#include "Acts/Utilities/Logger.hpp"
#include "Acts/Utilities/StringHelpers.hpp"

#include <iomanip>
#include <iterator>
#include <sstream>
#include <string>

Expand All @@ -31,7 +28,6 @@ namespace Acts {
/// @brief struct for the Navigation options that are forwarded to
/// the geometry
///
/// @tparam propagator_state_t Type of the object for navigation state
/// @tparam object_t Type of the object for navigation to check against
template <typename object_t>
struct NavigationOptions {
Expand All @@ -46,9 +42,9 @@ struct NavigationOptions {
/// always look for passive
bool resolvePassive = false;

/// object to check against: at start
/// Hint for start object
const object_t* startObject = nullptr;
/// object to check against: at end
/// Hint for end object
const object_t* endObject = nullptr;

/// External surface identifier for which the boundary check is ignored
Expand Down Expand Up @@ -856,7 +852,13 @@ class Navigator {
state.geoContext, stepper.position(state.stepping),
state.options.direction * stepper.direction(state.stepping),
navOpts, logger());
// The number of boundary candidates
std::sort(state.navigation.navBoundaries.begin(),
state.navigation.navBoundaries.end(),
[](const auto& a, const auto& b) {
return SurfaceIntersection::pathLengthOrder(a.first, b.first);
});

// Print boundary information
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navBoundaries.size();
Expand All @@ -866,6 +868,7 @@ class Navigator {
}
logger().log(Logging::VERBOSE, os.str());
}

// Set the begin index
state.navigation.navBoundaryIndex = 0;
if (!state.navigation.navBoundaries.empty()) {
Expand Down Expand Up @@ -1049,18 +1052,23 @@ class Navigator {
state.navigation.navSurfaces = currentLayer->compatibleSurfaces(
state.geoContext, stepper.position(state.stepping),
state.options.direction * stepper.direction(state.stepping), navOpts);
// the number of layer candidates
if (!state.navigation.navSurfaces.empty()) {
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navSurfaces.size();
os << " surface candidates found at path(s): ";
for (auto& sfc : state.navigation.navSurfaces) {
os << sfc.pathLength() << " ";
}
logger().log(Logging::VERBOSE, os.str());
std::sort(state.navigation.navSurfaces.begin(),
state.navigation.navSurfaces.end(),
SurfaceIntersection::pathLengthOrder);

// Print surface information
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navSurfaces.size();
os << " surface candidates found at path(s): ";
for (auto& sfc : state.navigation.navSurfaces) {
os << sfc.pathLength() << " ";
}
logger().log(Logging::VERBOSE, os.str());
}

// Surface candidates have been found
if (!state.navigation.navSurfaces.empty()) {
// set the index
state.navigation.navSurfaceIndex = 0;
// The stepper updates the step size ( single / multi component)
Expand All @@ -1070,6 +1078,7 @@ class Navigator {
<< stepper.outputStepSize(state.stepping));
return true;
}

state.navigation.navSurfaceIndex = state.navigation.navSurfaces.size();
ACTS_VERBOSE(volInfo(state) << "No surface candidates found.");
return false;
Expand Down Expand Up @@ -1104,25 +1113,32 @@ class Navigator {
navOpts.nearLimit = state.options.surfaceTolerance;
navOpts.farLimit =
stepper.getStepSize(state.stepping, ConstrainedStep::aborter);

// Request the compatible layers
state.navigation.navLayers =
state.navigation.currentVolume->compatibleLayers(
state.geoContext, stepper.position(state.stepping),
state.options.direction * stepper.direction(state.stepping),
navOpts);
std::sort(state.navigation.navLayers.begin(),
state.navigation.navLayers.end(),
[](const auto& a, const auto& b) {
return SurfaceIntersection::pathLengthOrder(a.first, b.first);
});

// Print layer information
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navLayers.size();
os << " layer candidates found at path(s): ";
for (auto& lc : state.navigation.navLayers) {
os << lc.first.pathLength() << " ";
}
logger().log(Logging::VERBOSE, os.str());
}

// Layer candidates have been found
if (!state.navigation.navLayers.empty()) {
// Screen output where they are
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navLayers.size();
os << " layer candidates found at path(s): ";
for (auto& lc : state.navigation.navLayers) {
os << lc.first.pathLength() << " ";
}
logger().log(Logging::VERBOSE, os.str());
}
// Set the index to the first
state.navigation.navLayerIndex = 0;
// Setting the step size towards first
Expand Down
15 changes: 6 additions & 9 deletions Core/include/Acts/Propagator/StandardAborters.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@ struct SurfaceReached {
/// Distance limit to discard intersections "behind us"
/// @note this is only necessary because some surfaces have more than one
/// intersection
std::optional<double> overrideNearLimit;
double nearLimit = -100 * UnitConstants::um;

SurfaceReached() = default;
SurfaceReached(double nearLimit) : overrideNearLimit(nearLimit) {}
SurfaceReached(double nLimit) : nearLimit(nLimit) {}

/// boolean operator for abort condition without using the result
///
Expand All @@ -101,14 +101,11 @@ struct SurfaceReached {
return true;
}

// not blindly using the stepper overstep limit here because it does not
// always work for perigee surfaces.
// not using the stepper overstep limit here because it does not always work
// for perigee surfaces
// note: the near limit is necessary for surfaces with more than one
// intersections in order to discard the ones which are behind us.
const double nearLimit =
overrideNearLimit.value_or(stepper.overstepLimit(state.stepping));
const double farLimit =
state.stepping.stepSize.value(ConstrainedStep::aborter);
// intersection in order to discard the ones which are behind us
andiwand marked this conversation as resolved.
Show resolved Hide resolved
const double farLimit = std::numeric_limits<double>::max();
const double tolerance = state.options.surfaceTolerance;

const auto sIntersection = surface->intersect(
Expand Down
3 changes: 0 additions & 3 deletions Core/include/Acts/Propagator/StepperConcept.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ METHOD_TRAIT(absolute_momentum_t, absoluteMomentum);
METHOD_TRAIT(momentum_t, momentum);
METHOD_TRAIT(charge_t, charge);
METHOD_TRAIT(time_t, time);
METHOD_TRAIT(overstep_t, overstepLimit);
METHOD_TRAIT(bound_state_method_t, boundState);
METHOD_TRAIT(curvilinear_state_method_t, curvilinearState);
METHOD_TRAIT(update_t, update);
Expand Down Expand Up @@ -114,8 +113,6 @@ constexpr bool MultiStepperStateConcept= require<
static_assert(charge_exists, "charge method not found");
constexpr static bool time_exists = has_method<const S, double, time_t, const state&>;
static_assert(time_exists, "time method not found");
constexpr static bool overstep_exists = has_method<const S, double, overstep_t, const state&>;
static_assert(overstep_exists, "overstepLimit method not found");
constexpr static bool bound_state_method_exists= has_method<const S, Result<typename S::BoundState>, bound_state_method_t, state&, const Surface&, bool, const FreeToBoundCorrection&>;
static_assert(bound_state_method_exists, "boundState method not found");
constexpr static bool curvilinear_state_method_exists = has_method<const S, typename S::CurvilinearState, curvilinear_state_method_t, state&, bool>;
Expand Down
8 changes: 0 additions & 8 deletions Core/include/Acts/Propagator/StraightLineStepper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -218,11 +218,6 @@ class StraightLineStepper {
/// @param state [in] The stepping state (thread-local cache)
double time(const State& state) const { return state.pars[eFreeTime]; }

/// Overstep limit
double overstepLimit(const State& /*state*/) const {
return -m_overstepLimit;
}

/// Update surface status
///
/// This method intersects the provided surface and update the navigation
Expand Down Expand Up @@ -451,9 +446,6 @@ class StraightLineStepper {
// return h
return h;
}

private:
double m_overstepLimit = s_onSurfaceTolerance;
};

template <typename navigator_t>
Expand Down
5 changes: 3 additions & 2 deletions Core/include/Acts/Propagator/detail/SteppingHelper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
#include "Acts/Utilities/Intersection.hpp"
#include "Acts/Utilities/Logger.hpp"

#include <limits>

namespace Acts::detail {

/// Update surface status - Single component
Expand Down Expand Up @@ -50,8 +52,7 @@ Acts::Intersection3D::Status updateSingleSurfaceStatus(
return Intersection3D::Status::onSurface;
}

// Path and overstep limit checking
const double nearLimit = stepper.overstepLimit(state);
const double nearLimit = std::numeric_limits<double>::lowest();
const double farLimit = state.stepSize.value(ConstrainedStep::aborter);

if (sIntersection && detail::checkIntersection(sIntersection.intersection(),
Expand Down
12 changes: 8 additions & 4 deletions Core/include/Acts/Utilities/Intersection.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,10 @@ class ObjectMultiIntersection {
return {m_intersections[index], m_object, index};
}

constexpr const MultiIntersection3D& intersections() const {
return m_intersections;
}

constexpr std::size_t size() const { return m_intersections.size(); }

constexpr const object_t* object() const { return m_object; }
Expand Down Expand Up @@ -274,7 +278,7 @@ bool checkIntersection(const intersection_t& intersection, double nearLimit,
<< nearLimit << ", " << farLimit << ", " << distance);

const bool coCriterion = distance > nearLimit;
const bool cpCriterion = std::abs(distance) < std::abs(farLimit) + tolerance;
const bool cpCriterion = distance < farLimit + tolerance;

const bool accept = coCriterion && cpCriterion;

Expand All @@ -288,9 +292,9 @@ bool checkIntersection(const intersection_t& intersection, double nearLimit,
}
if (!cpCriterion) {
ACTS_VERBOSE("- intersection path length "
<< std::abs(distance) << " is over the far limit "
<< (std::abs(farLimit) + tolerance)
<< " (including tolerance of " << tolerance << ")");
<< distance << " is over the far limit "
<< (farLimit + tolerance) << " (including tolerance of "
<< tolerance << ")");
}
}

Expand Down
Loading
Loading