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!: std::unique_ptr to std::optional in Acts::PropagatorResult #1622

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
8 changes: 4 additions & 4 deletions Core/include/Acts/Propagator/Propagator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@

#include <cmath>
#include <functional>
#include <memory>
#include <optional>
#include <type_traits>

#include <boost/algorithm/string.hpp>
Expand All @@ -46,11 +46,11 @@ struct PropagatorResult : private detail::Extendable<result_list...> {
/// Accessor to additional propagation quantities
using detail::Extendable<result_list...>::get;

/// Final track parameters - initialized to null pointer
std::unique_ptr<parameters_t> endParameters = nullptr;
/// Final track parameters
std::optional<parameters_t> endParameters = std::nullopt;
andiwand marked this conversation as resolved.
Show resolved Hide resolved

/// Full transport jacobian
std::unique_ptr<BoundMatrix> transportJacobian = nullptr;
std::optional<BoundMatrix> transportJacobian = std::nullopt;

/// Number of propagation steps that were carried out
unsigned int steps = 0;
Expand Down
16 changes: 4 additions & 12 deletions Core/include/Acts/Propagator/Propagator.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -160,15 +160,11 @@ auto Acts::Propagator<S, N>::propagate(
if (result.ok()) {
/// Convert into return type and fill the result object
auto curvState = m_stepper.curvilinearState(state.stepping);
auto& curvParameters = std::get<CurvilinearTrackParameters>(curvState);
// Fill the end parameters
inputResult.endParameters =
std::make_unique<CurvilinearTrackParameters>(std::move(curvParameters));
inputResult.endParameters = std::get<CurvilinearTrackParameters>(curvState);
// Only fill the transport jacobian when covariance transport was done
if (state.stepping.covTransport) {
auto& tJacobian = std::get<Jacobian>(curvState);
inputResult.transportJacobian =
std::make_unique<Jacobian>(std::move(tJacobian));
inputResult.transportJacobian = std::get<Jacobian>(curvState);
}
return Result<ResultType>::success(std::forward<ResultType>(inputResult));
} else {
Expand Down Expand Up @@ -258,15 +254,11 @@ auto Acts::Propagator<S, N>::propagate(

const auto& bs = *bsRes;

auto& boundParams = std::get<BoundTrackParameters>(bs);
// Fill the end parameters
inputResult.endParameters =
std::make_unique<BoundTrackParameters>(std::move(boundParams));
inputResult.endParameters = std::get<BoundTrackParameters>(bs);
// Only fill the transport jacobian when covariance transport was done
if (state.stepping.covTransport) {
auto& tJacobian = std::get<Jacobian>(bs);
inputResult.transportJacobian =
std::make_unique<Jacobian>(std::move(tJacobian));
inputResult.transportJacobian = std::get<Jacobian>(bs);
}
return Result<ResultType>::success(std::forward<ResultType>(inputResult));
} else {
Expand Down
2 changes: 1 addition & 1 deletion Core/include/Acts/TrackFitting/GaussianSumFitter.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ struct GaussianSumFitter {
ACTS_VERBOSE("+-----------------------------------------------+");
ACTS_VERBOSE("| Gsf: Do propagation back to reference surface |");
ACTS_VERBOSE("+-----------------------------------------------+");
auto lastResult = [&]() -> Result<std::unique_ptr<BoundTrackParameters>> {
auto lastResult = [&]() -> Result<std::optional<BoundTrackParameters>> {
const auto& [surface, lastSmoothedState] =
std::get<1>(smoothResult).front();

Expand Down
6 changes: 3 additions & 3 deletions Core/include/Acts/Vertexing/AdaptiveMultiVertexFitter.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ Acts::Result<void> Acts::
return res.error();
}
// Set ip3dParams for current trackAtVertex
currentVtxInfo.ip3dParams.emplace(trk, *(res.value()));
currentVtxInfo.ip3dParams.emplace(trk, res.value());
}
return {};
}
Expand Down Expand Up @@ -236,7 +236,7 @@ Acts::AdaptiveMultiVertexFitter<input_track_t, linearizer_t>::
return res.error();
}
// Set ip3dParams for current trackAtVertex
currentVtxInfo.ip3dParams.emplace(trk, *(res.value()));
currentVtxInfo.ip3dParams.emplace(trk, res.value());
}
// Set compatibility with current vertex
auto compRes = m_cfg.ipEst.get3dVertexCompatibility(
Expand Down Expand Up @@ -342,4 +342,4 @@ void Acts::AdaptiveMultiVertexFitter<
}
}
}
}
}
17 changes: 8 additions & 9 deletions Core/include/Acts/Vertexing/HelicalTrackLinearizer.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -29,30 +29,29 @@ Acts::Result<Acts::LinearizedTrack> Acts::
? NavigationDirection::Forward
: NavigationDirection::Backward;

const BoundTrackParameters* endParams = nullptr;
// Do the propagation to linPointPos
auto result = m_cfg.propagator->propagate(params, *perigeeSurface, pOptions);
if (result.ok()) {
endParams = (*result).endParameters.get();
} else {
if (not result.ok()) {
return result.error();
}

BoundVector paramsAtPCA = endParams->parameters();
const auto& endParams = *result->endParameters;

BoundVector paramsAtPCA = endParams.parameters();
Vector4 positionAtPCA = Vector4::Zero();
{
auto pos = endParams->position(gctx);
auto pos = endParams.position(gctx);
positionAtPCA[ePos0] = pos[ePos0];
positionAtPCA[ePos1] = pos[ePos1];
positionAtPCA[ePos2] = pos[ePos2];
positionAtPCA[eTime] = endParams->time();
positionAtPCA[eTime] = endParams.time();
}
BoundSymMatrix parCovarianceAtPCA = endParams->covariance().value();
BoundSymMatrix parCovarianceAtPCA = endParams.covariance().value();

if (parCovarianceAtPCA.determinant() <= 0) {
// Use the original parameters
paramsAtPCA = params.parameters();
auto pos = endParams->position(gctx);
auto pos = endParams.position(gctx);
positionAtPCA[ePos0] = pos[ePos0];
positionAtPCA[ePos1] = pos[ePos1];
positionAtPCA[ePos2] = pos[ePos2];
Expand Down
9 changes: 4 additions & 5 deletions Core/include/Acts/Vertexing/ImpactPointEstimator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,10 @@ class ImpactPointEstimator {
/// @param state The state object
///
/// @return New track params
Result<std::unique_ptr<const BoundTrackParameters>>
estimate3DImpactParameters(const GeometryContext& gctx,
const Acts::MagneticFieldContext& mctx,
const BoundTrackParameters& trkParams,
const Vector3& vtxPos, State& state) const;
Result<BoundTrackParameters> estimate3DImpactParameters(
const GeometryContext& gctx, const Acts::MagneticFieldContext& mctx,
const BoundTrackParameters& trkParams, const Vector3& vtxPos,
State& state) const;

/// @brief Estimates the compatibility of a
/// track to a vertex position based on the 3d
Expand Down
4 changes: 2 additions & 2 deletions Core/include/Acts/Vertexing/ImpactPointEstimator.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ Acts::ImpactPointEstimator<input_track_t, propagator_t, propagator_options_t>::

template <typename input_track_t, typename propagator_t,
typename propagator_options_t>
Acts::Result<std::unique_ptr<const Acts::BoundTrackParameters>>
Acts::Result<Acts::BoundTrackParameters>
Acts::ImpactPointEstimator<input_track_t, propagator_t, propagator_options_t>::
estimate3DImpactParameters(const GeometryContext& gctx,
const Acts::MagneticFieldContext& mctx,
Expand Down Expand Up @@ -79,7 +79,7 @@ Acts::ImpactPointEstimator<input_track_t, propagator_t, propagator_options_t>::
// Do the propagation to linPointPos
auto result = m_cfg.propagator->propagate(trkParams, *planeSurface, pOptions);
if (result.ok()) {
return std::move((*result).endParameters);
return *result->endParameters;
} else {
return result.error();
}
Expand Down
6 changes: 3 additions & 3 deletions Tests/UnitTests/Core/Propagator/ExtrapolatorTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ BOOST_DATA_TEST_CASE(
options.maxStepSize = 10_cm;
options.pathLimit = 25_cm;

BOOST_CHECK(epropagator.propagate(start, options).value().endParameters !=
nullptr);
BOOST_CHECK(
epropagator.propagate(start, options).value().endParameters.has_value());
}

// This test case checks that no segmentation fault appears
Expand Down Expand Up @@ -176,7 +176,7 @@ BOOST_DATA_TEST_CASE(
const auto& cresult = epropagator.propagate(start, *csurface, optionsEmpty)
.value()
.endParameters;
BOOST_CHECK(cresult != nullptr);
BOOST_CHECK(cresult.has_value());
}
}

Expand Down
38 changes: 18 additions & 20 deletions Tests/UnitTests/Core/Propagator/MaterialCollectionTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta,
std::cout << ">>> Backward Propagation : start." << std::endl;
}
const auto& bwdResult =
prop.propagate(*fwdResult.endParameters.get(), startSurface, bwdOptions)
prop.propagate(*fwdResult.endParameters, startSurface, bwdOptions)
.value();

if (debugModeBwd) {
Expand Down Expand Up @@ -241,43 +241,42 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta,
}

// move forward step by step through the surfaces
const BoundTrackParameters* sParameters = &start;
std::vector<std::unique_ptr<const BoundTrackParameters>> stepParameters;
BoundTrackParameters sParameters = start;
std::vector<BoundTrackParameters> stepParameters;
for (auto& fwdSteps : fwdMaterial.materialInteractions) {
if (debugModeFwdStep) {
std::cout << ">>> Forward step : "
<< sParameters->referenceSurface().geometryId() << " --> "
<< sParameters.referenceSurface().geometryId() << " --> "
<< fwdSteps.surface->geometryId() << std::endl;
}

// make a forward step
const auto& fwdStep =
prop.propagate(*sParameters, (*fwdSteps.surface), fwdStepOptions)
prop.propagate(sParameters, (*fwdSteps.surface), fwdStepOptions)
.value();

auto& fwdStepMaterial =
fwdStep.template get<typename MaterialInteractor::result_type>();
fwdStepStepMaterialInX0 += fwdStepMaterial.materialInX0;
fwdStepStepMaterialInL0 += fwdStepMaterial.materialInL0;

if (fwdStep.endParameters != nullptr) {
if (fwdStep.endParameters.has_value()) {
// make sure the parameters do not run out of scope
stepParameters.push_back(std::make_unique<BoundTrackParameters>(
(*fwdStep.endParameters.get())));
sParameters = stepParameters.back().get();
stepParameters.push_back(*fwdStep.endParameters);
sParameters = stepParameters.back();
}
}
// final destination surface
const Surface& dSurface = fwdResult.endParameters->referenceSurface();

if (debugModeFwdStep) {
std::cout << ">>> Forward step : "
<< sParameters->referenceSurface().geometryId() << " --> "
<< sParameters.referenceSurface().geometryId() << " --> "
<< dSurface.geometryId() << std::endl;
}

const auto& fwdStepFinal =
prop.propagate(*sParameters, dSurface, fwdStepOptions).value();
prop.propagate(sParameters, dSurface, fwdStepOptions).value();

auto& fwdStepMaterial =
fwdStepFinal.template get<typename MaterialInteractor::result_type>();
Expand Down Expand Up @@ -317,41 +316,40 @@ void runTest(const propagator_t& prop, double pT, double phi, double theta,
}

// move forward step by step through the surfaces
sParameters = fwdResult.endParameters.get();
sParameters = *fwdResult.endParameters;
for (auto& bwdSteps : bwdMaterial.materialInteractions) {
if (debugModeBwdStep) {
std::cout << ">>> Backward step : "
<< sParameters->referenceSurface().geometryId() << " --> "
<< sParameters.referenceSurface().geometryId() << " --> "
<< bwdSteps.surface->geometryId() << std::endl;
}
// make a forward step
const auto& bwdStep =
prop.propagate(*sParameters, (*bwdSteps.surface), bwdStepOptions)
prop.propagate(sParameters, (*bwdSteps.surface), bwdStepOptions)
.value();

auto& bwdStepMaterial =
bwdStep.template get<typename MaterialInteractor::result_type>();
bwdStepStepMaterialInX0 += bwdStepMaterial.materialInX0;
bwdStepStepMaterialInL0 += bwdStepMaterial.materialInL0;

if (bwdStep.endParameters != nullptr) {
if (bwdStep.endParameters.has_value()) {
// make sure the parameters do not run out of scope
stepParameters.push_back(std::make_unique<BoundTrackParameters>(
*(bwdStep.endParameters.get())));
sParameters = stepParameters.back().get();
stepParameters.push_back(*bwdStep.endParameters);
sParameters = stepParameters.back();
}
}
// final destination surface
const Surface& dbSurface = start.referenceSurface();

if (debugModeBwdStep) {
std::cout << ">>> Backward step : "
<< sParameters->referenceSurface().geometryId() << " --> "
<< sParameters.referenceSurface().geometryId() << " --> "
<< dSurface.geometryId() << std::endl;
}

const auto& bwdStepFinal =
prop.propagate(*sParameters, dbSurface, bwdStepOptions).value();
prop.propagate(sParameters, dbSurface, bwdStepOptions).value();

auto& bwdStepMaterial =
bwdStepFinal.template get<typename MaterialInteractor::result_type>();
Expand Down
4 changes: 2 additions & 2 deletions Tests/UnitTests/Core/Vertexing/ImpactPointEstimatorTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ BOOST_DATA_TEST_CASE(SingleTrackDistanceParametersCompatibility3d, tracks, d0,
// estimate parameters at the closest point in 3d
auto res = ipEstimator.estimate3DImpactParameters(
geoContext, magFieldContext, myTrack, refPosition, state);
BoundTrackParameters trackAtIP3d = **res;
BoundTrackParameters trackAtIP3d = *res;
const auto& atPerigee = myTrack.parameters();
const auto& atIp3d = trackAtIP3d.parameters();

Expand Down Expand Up @@ -190,7 +190,7 @@ BOOST_AUTO_TEST_CASE(SingleTrackDistanceParametersAthenaRegression) {
auto res2 = ipEstimator.estimate3DImpactParameters(
geoContext, magFieldContext, params1, vtxPos, state);
BOOST_CHECK(res2.ok());
BoundTrackParameters endParams = **res2;
BoundTrackParameters endParams = *res2;
Vector3 surfaceCenter = endParams.referenceSurface().center(geoContext);

BOOST_CHECK_EQUAL(surfaceCenter, vtxPos);
Expand Down