From 3ce296b0a59d8dc388af4817eca9fd6503c747f4 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 25 Nov 2024 10:16:44 +0100 Subject: [PATCH 01/10] change measurement columns to not allocate --- .../Acts/EventData/VectorMultiTrajectory.hpp | 58 ++++++++++++++----- .../detail/MultiTrajectoryTestsCommon.hpp | 24 +++++++- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp index 3859239ae69..39c1152b6c8 100644 --- a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp +++ b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp @@ -47,6 +47,32 @@ using MultiTrajectoryTraits::IndexType; constexpr auto kInvalid = MultiTrajectoryTraits::kInvalid; constexpr auto MeasurementSizeMax = MultiTrajectoryTraits::MeasurementSizeMax; +template +struct NonInitializingAllocator { + using value_type = T; + + NonInitializingAllocator() noexcept = default; + + template + NonInitializingAllocator( + const NonInitializingAllocator& /*other*/) noexcept {} + + template + bool operator==(const NonInitializingAllocator& /*other*/) const noexcept { + return true; + } + + T* allocate(std::size_t n) const { return std::allocator{}.allocate(n); } + + void deallocate(T* const p, std::size_t n) const noexcept { + std::allocator{}.deallocate(p, n); + } + + /// This construct function intentionally does not initialize the object! + /// Be very careful when using this allocator. + void construct(T* /*p*/) {} +}; + class VectorMultiTrajectoryBase { public: struct Statistics { @@ -320,9 +346,9 @@ class VectorMultiTrajectoryBase { m_params; std::vector::Covariance> m_cov; - std::vector m_meas; + std::vector> m_meas; std::vector m_measOffset; - std::vector m_measCov; + std::vector> m_measCov; std::vector m_measCovOffset; std::vector::Covariance> m_jac; @@ -465,22 +491,26 @@ class VectorMultiTrajectory final } void allocateCalibrated_impl(IndexType istate, std::size_t measdim) { - throw_assert(measdim > 0 && measdim <= eBoundSize, - "Invalid measurement dimension detected"); + if (m_measOffset[istate] == kInvalid || + m_measCovOffset[istate] == kInvalid || + m_index[istate].measdim != measdim) { + m_index[istate].measdim = measdim; - if (m_measOffset[istate] != kInvalid && - m_measCovOffset[istate] != kInvalid && - m_index[istate].measdim == measdim) { - return; - } + m_measOffset[istate] = static_cast(m_meas.size()); + m_meas.resize(m_meas.size() + measdim); - m_index[istate].measdim = measdim; + m_measCovOffset[istate] = static_cast(m_measCov.size()); + m_measCov.resize(m_measCov.size() + measdim * measdim); + } - m_measOffset[istate] = static_cast(m_meas.size()); - m_meas.resize(m_meas.size() + measdim); + // Always initialize to zero + for (std::size_t i = 0; i < measdim; i++) { + m_meas[m_measOffset[istate] + i] = 0; + } - m_measCovOffset[istate] = static_cast(m_measCov.size()); - m_measCov.resize(m_measCov.size() + measdim * measdim); + for (std::size_t i = 0; i < measdim * measdim; i++) { + m_measCov[m_measCovOffset[istate] + i] = 0; + } } void setUncalibratedSourceLink_impl(IndexType istate, diff --git a/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp b/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp index a2e374c0cc2..1c66756cab1 100644 --- a/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp +++ b/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp @@ -10,6 +10,7 @@ #include +#include "Acts/Definitions/Algebra.hpp" #include "Acts/Definitions/TrackParametrization.hpp" #include "Acts/EventData/TrackStatePropMask.hpp" #include "Acts/EventData/VectorMultiTrajectory.hpp" @@ -196,6 +197,9 @@ class MultiTrajectoryTestsCommon { alwaysPresent(ts); ts.allocateCalibrated(5); BOOST_CHECK(ts.hasCalibrated()); + BOOST_CHECK_EQUAL(ts.template calibrated<5>(), ActsVector<5>::Zero()); + BOOST_CHECK_EQUAL(ts.template calibratedCovariance<5>(), + ActsSquareMatrix<5>::Zero()); ts = t.getTrackState(t.addTrackState(PM::None)); BOOST_CHECK(!ts.hasPredicted()); @@ -242,6 +246,9 @@ class MultiTrajectoryTestsCommon { BOOST_CHECK(!ts.hasJacobian()); ts.allocateCalibrated(5); BOOST_CHECK(ts.hasCalibrated()); + BOOST_CHECK_EQUAL(ts.template calibrated<5>(), ActsVector<5>::Zero()); + BOOST_CHECK_EQUAL(ts.template calibratedCovariance<5>(), + ActsSquareMatrix<5>::Zero()); ts = t.getTrackState(t.addTrackState(PM::Jacobian)); BOOST_CHECK(!ts.hasPredicted()); @@ -300,6 +307,9 @@ class MultiTrajectoryTestsCommon { BOOST_CHECK(ts.hasSmoothed()); BOOST_CHECK(ts.hasCalibrated()); BOOST_CHECK(!ts.hasJacobian()); + BOOST_CHECK_EQUAL(ts.template calibrated<5>(), ActsVector<5>::Zero()); + BOOST_CHECK_EQUAL(ts.template calibratedCovariance<5>(), + ActsSquareMatrix<5>::Zero()); ts.addComponents(PM::Jacobian); BOOST_CHECK(ts.hasPredicted()); @@ -379,6 +389,10 @@ class MultiTrajectoryTestsCommon { // reset measurements w/ full parameters auto [measPar, measCov] = generateBoundParametersCovariance(rng, {}); tsb.allocateCalibrated(eBoundSize); + BOOST_CHECK_EQUAL(tsb.template calibrated(), + BoundVector::Zero()); + BOOST_CHECK_EQUAL(tsb.template calibratedCovariance(), + BoundMatrix::Zero()); tsb.template calibrated() = measPar; tsb.template calibratedCovariance() = measCov; BOOST_CHECK_EQUAL(tsa.template calibrated(), measPar); @@ -394,7 +408,15 @@ class MultiTrajectoryTestsCommon { std::size_t nMeasurements = tsb.effectiveCalibrated().rows(); auto effPar = measPar.head(nMeasurements); auto effCov = measCov.topLeftCorner(nMeasurements, nMeasurements); - tsb.allocateCalibrated(eBoundSize); + tsb.allocateCalibrated( + eBoundSize); // no allocation, but we expect it to be reset to zero + // with this overload + BOOST_CHECK_EQUAL(tsa.effectiveCalibrated(), BoundVector::Zero()); + BOOST_CHECK_EQUAL(tsa.effectiveCalibratedCovariance(), + BoundMatrix::Zero()); + BOOST_CHECK_EQUAL(tsa.effectiveCalibrated(), BoundVector::Zero()); + BOOST_CHECK_EQUAL(tsa.effectiveCalibratedCovariance(), + BoundMatrix::Zero()); tsb.effectiveCalibrated() = effPar; tsb.effectiveCalibratedCovariance() = effCov; BOOST_CHECK_EQUAL(tsa.effectiveCalibrated(), effPar); From 976c2f347a6b7b5dba44586a433d3196fcc8e2c9 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 25 Nov 2024 10:17:02 +0100 Subject: [PATCH 02/10] add a size() method to the vector track container --- Core/include/Acts/EventData/VectorTrackContainer.hpp | 1 + Core/src/EventData/VectorTrackContainer.cpp | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/Core/include/Acts/EventData/VectorTrackContainer.hpp b/Core/include/Acts/EventData/VectorTrackContainer.hpp index 8734f69eb4c..d0ce9cf402d 100644 --- a/Core/include/Acts/EventData/VectorTrackContainer.hpp +++ b/Core/include/Acts/EventData/VectorTrackContainer.hpp @@ -253,6 +253,7 @@ class VectorTrackContainer final : public detail_vtc::VectorTrackContainerBase { void reserve(IndexType size); void clear(); + std::size_t size() const; void setReferenceSurface_impl(IndexType itrack, std::shared_ptr surface) { diff --git a/Core/src/EventData/VectorTrackContainer.cpp b/Core/src/EventData/VectorTrackContainer.cpp index dc7cca761d1..5be40fae6c8 100644 --- a/Core/src/EventData/VectorTrackContainer.cpp +++ b/Core/src/EventData/VectorTrackContainer.cpp @@ -167,4 +167,8 @@ void VectorTrackContainer::clear() { } } +std::size_t VectorTrackContainer::size() const { + return m_tipIndex.size(); +} + } // namespace Acts From a479b2717f6519acec88bb8bd5d476d4ae18e425 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 25 Nov 2024 15:53:45 +0100 Subject: [PATCH 03/10] add overload allocateCalibrated that immediately sets the value, use from other overload --- .../Acts/EventData/MultiTrajectory.hpp | 18 +++++-- .../MultiTrajectoryBackendConcept.hpp | 10 +++- .../Acts/EventData/TrackStateProxy.hpp | 23 +++++++++ .../Acts/EventData/TrackStateProxyConcept.hpp | 7 +++ .../Acts/EventData/VectorMultiTrajectory.hpp | 45 ++++++++++++++---- .../detail/MultiTrajectoryTestsCommon.hpp | 47 +++++++++++++++++++ Core/src/EventData/VectorMultiTrajectory.cpp | 1 + .../Podio/PodioTrackStateContainer.hpp | 37 ++++++++++++--- .../Core/EventData/MultiTrajectoryTests.cpp | 5 ++ .../Podio/PodioTrackStateContainerTest.cpp | 5 ++ 10 files changed, 178 insertions(+), 20 deletions(-) diff --git a/Core/include/Acts/EventData/MultiTrajectory.hpp b/Core/include/Acts/EventData/MultiTrajectory.hpp index 1c065fd7e12..e293c6c9c5f 100644 --- a/Core/include/Acts/EventData/MultiTrajectory.hpp +++ b/Core/include/Acts/EventData/MultiTrajectory.hpp @@ -8,6 +8,7 @@ #pragma once +#include "Acts/Definitions/Algebra.hpp" #include "Acts/Definitions/TrackParametrization.hpp" #include "Acts/EventData/MeasurementHelpers.hpp" #include "Acts/EventData/SourceLink.hpp" @@ -687,13 +688,24 @@ class MultiTrajectory { /// Allocate storage for a calibrated measurement of specified dimension /// @param istate The track state to store for /// @param measdim the dimension of the measurement to store - /// @note Is a noop if the track state already has an allocation - /// an the dimension is the same. + /// @note In case an allocation is already present, no additional allocation + /// will be performed, but the existing allocation will be zeroed. void allocateCalibrated(IndexType istate, std::size_t measdim) { throw_assert(measdim > 0 && measdim <= eBoundSize, "Invalid measurement dimension detected"); - self().allocateCalibrated_impl(istate, measdim); + visit_measurement(measdim, [this, istate]( + std::integral_constant) { + self().template allocateCalibrated_impl( + istate, ActsVector{ActsVector::Zero()}, + ActsSquareMatrix{ActsSquareMatrix::Zero()}); + }); + } + + template + void allocateCalibrated(IndexType istate, const Eigen::DenseBase& val, + const Eigen::DenseBase& cov) { + self().template allocateCalibrated_impl(istate, val, cov); } void setUncalibratedSourceLink(IndexType istate, SourceLink&& sourceLink) diff --git a/Core/include/Acts/EventData/MultiTrajectoryBackendConcept.hpp b/Core/include/Acts/EventData/MultiTrajectoryBackendConcept.hpp index 92ae47270bc..83f2593d647 100644 --- a/Core/include/Acts/EventData/MultiTrajectoryBackendConcept.hpp +++ b/Core/include/Acts/EventData/MultiTrajectoryBackendConcept.hpp @@ -130,7 +130,15 @@ concept MutableMultiTrajectoryBackend = { v.template addColumn_impl(col) }; { v.template addColumn_impl(col) }; - { v.allocateCalibrated_impl(istate, dim) }; + { + v.template allocateCalibrated_impl(istate, ActsVector<1>{}, + ActsSquareMatrix<1>{}) + }; + // Assuming intermediate values also work + { + v.template allocateCalibrated_impl(istate, ActsVector{}, + ActsSquareMatrix{}) + }; { v.setUncalibratedSourceLink_impl(istate, std::move(sl)) }; diff --git a/Core/include/Acts/EventData/TrackStateProxy.hpp b/Core/include/Acts/EventData/TrackStateProxy.hpp index 741bfa5b511..c43ef4c38d3 100644 --- a/Core/include/Acts/EventData/TrackStateProxy.hpp +++ b/Core/include/Acts/EventData/TrackStateProxy.hpp @@ -871,10 +871,33 @@ class TrackStateProxy { /// Allocate storage to be able to store a measurement of size @p measdim. /// This must be called **before** setting the measurement content. + /// @note This does not allocate if an allocation of the same size already exists + /// @note This will zero-initialize the allocated storage + /// @note This is an error if an existing allocation has different size void allocateCalibrated(std::size_t measdim) { m_traj->allocateCalibrated(m_istate, measdim); } + /// Allocate storage and assign the given vector and covariance to it. + /// The dimension is inferred from the given vector and matrix. + /// @tparam val_t Type of the vector + /// @tparam cov_t Type of the covariance matrix + /// @param val The measurement vector + /// @param cov The covariance matrix + /// @note This does not allocate if an allocation of the same size already exists + /// @note This is an error if an existing allocation has different size + template + void allocateCalibrated(const Eigen::DenseBase& val, + const Eigen::DenseBase& cov) + requires(Eigen::PlainObjectBase::RowsAtCompileTime == + Eigen::PlainObjectBase::RowsAtCompileTime && + Eigen::PlainObjectBase::RowsAtCompileTime == + Eigen::PlainObjectBase::ColsAtCompileTime) + { + m_traj->template allocateCalibrated< + Eigen::PlainObjectBase::RowsAtCompileTime>(m_istate, val, cov); + } + /// @} /// @anchor track_state_share_copy diff --git a/Core/include/Acts/EventData/TrackStateProxyConcept.hpp b/Core/include/Acts/EventData/TrackStateProxyConcept.hpp index d148d3c4940..61e98adbc48 100644 --- a/Core/include/Acts/EventData/TrackStateProxyConcept.hpp +++ b/Core/include/Acts/EventData/TrackStateProxyConcept.hpp @@ -248,6 +248,13 @@ concept MutableTrackStateProxyConcept = { v.allocateCalibrated(measdim) }; + { v.allocateCalibrated(ActsVector<1>{}, ActsSquareMatrix<1>{}) }; + // Assuming intermediate values are also allowed + { + v.allocateCalibrated(ActsVector{}, + ActsSquareMatrix{}) + }; + { v.chi2() } -> std::same_as; { v.pathLength() } -> std::same_as; diff --git a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp index 39c1152b6c8..ea99eff7a89 100644 --- a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp +++ b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp @@ -490,10 +490,28 @@ class VectorMultiTrajectory final return detail_vmt::VectorMultiTrajectoryBase::hasColumn_impl(*this, key); } - void allocateCalibrated_impl(IndexType istate, std::size_t measdim) { - if (m_measOffset[istate] == kInvalid || - m_measCovOffset[istate] == kInvalid || + template + void allocateCalibrated_impl(IndexType istate, + const Eigen::DenseBase& val, + const Eigen::DenseBase& cov) + + requires(Eigen::PlainObjectBase::RowsAtCompileTime > 0 && + Eigen::PlainObjectBase::RowsAtCompileTime <= eBoundSize && + Eigen::PlainObjectBase::RowsAtCompileTime == + Eigen::PlainObjectBase::RowsAtCompileTime && + Eigen::PlainObjectBase::RowsAtCompileTime == + Eigen::PlainObjectBase::ColsAtCompileTime) + { + constexpr std::size_t measdim = val_t::RowsAtCompileTime; + + if (m_index[istate].measdim != kInvalid && m_index[istate].measdim != measdim) { + throw std::invalid_argument{ + "Measurement dimension does not match the allocated dimension"}; + } + + if (m_measOffset[istate] == kInvalid || + m_measCovOffset[istate] == kInvalid) { m_index[istate].measdim = measdim; m_measOffset[istate] = static_cast(m_meas.size()); @@ -503,14 +521,21 @@ class VectorMultiTrajectory final m_measCov.resize(m_measCov.size() + measdim * measdim); } - // Always initialize to zero - for (std::size_t i = 0; i < measdim; i++) { - m_meas[m_measOffset[istate] + i] = 0; - } + m_index[istate].measdim = measdim; - for (std::size_t i = 0; i < measdim * measdim; i++) { - m_measCov[m_measCovOffset[istate] + i] = 0; - } + m_measOffset[istate] = static_cast(m_meas.size()); + m_meas.resize(m_meas.size() + measdim); + + double* measPtr = &m_meas[m_measOffset[istate]]; + Eigen::Map> valMap(measPtr); + valMap = val; + + m_measCovOffset[istate] = static_cast(m_measCov.size()); + m_measCov.resize(m_measCov.size() + measdim * measdim); + + double* covPtr = &m_measCov[m_measCovOffset[istate]]; + Eigen::Map> covMap(covPtr); + covMap = cov; } void setUncalibratedSourceLink_impl(IndexType istate, diff --git a/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp b/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp index 1c66756cab1..422dc58e053 100644 --- a/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp +++ b/Core/include/Acts/EventData/detail/MultiTrajectoryTestsCommon.hpp @@ -21,6 +21,7 @@ #include "Acts/Utilities/HashedString.hpp" #include +#include namespace Acts::detail::Test { @@ -388,6 +389,8 @@ class MultiTrajectoryTestsCommon { { // reset measurements w/ full parameters auto [measPar, measCov] = generateBoundParametersCovariance(rng, {}); + // Explicitly unset to avoid error below + tsb.unset(TrackStatePropMask::Calibrated); tsb.allocateCalibrated(eBoundSize); BOOST_CHECK_EQUAL(tsb.template calibrated(), BoundVector::Zero()); @@ -466,6 +469,8 @@ class MultiTrajectoryTestsCommon { BOOST_CHECK_EQUAL( ts.getUncalibratedSourceLink().template get().sourceId, pc.sourceLink.sourceId); + // Explicitly unset to avoid error below + ts.unset(TrackStatePropMask::Calibrated); testSourceLinkCalibratorReturn( gctx, cctx, SourceLink{ttsb.sourceLink}, ts); BOOST_CHECK_EQUAL( @@ -784,6 +789,8 @@ class MultiTrajectoryTestsCommon { BOOST_CHECK_NE(ts1.pathLength(), ts2.pathLength()); BOOST_CHECK_NE(&ts1.referenceSurface(), &ts2.referenceSurface()); + // Explicitly unset to avoid error below + ts1.unset(TrackStatePropMask::Calibrated); ts1.copyFrom(ts2); BOOST_CHECK_EQUAL(ts1.predicted(), ts2.predicted()); @@ -817,6 +824,8 @@ class MultiTrajectoryTestsCommon { ts2 = mkts(PM::Predicted | PM::Jacobian | PM::Calibrated); ts2.copyFrom(ots2, PM::Predicted | PM::Jacobian | PM::Calibrated); // copy into empty ts, only copy some + // explicitly unset to avoid error below + ts1.unset(TrackStatePropMask::Calibrated); ts1.copyFrom(ots1); // reset to original // is different again BOOST_CHECK_NE(ts1.predicted(), ts2.predicted()); @@ -838,6 +847,8 @@ class MultiTrajectoryTestsCommon { BOOST_CHECK_NE(ts1.pathLength(), ts2.pathLength()); BOOST_CHECK_NE(&ts1.referenceSurface(), &ts2.referenceSurface()); + // Explicitly unset to avoid error below + ts1.unset(TrackStatePropMask::Calibrated); ts1.copyFrom(ts2); // some components are same now @@ -1215,5 +1226,41 @@ class MultiTrajectoryTestsCommon { // runTest([](const std::string& c) { return c; }); // runTest([](std::string_view c) { return c; }); } + + void testMultiTrajectoryAllocateCalibratedInit( + std::default_random_engine& rng) { + trajectory_t traj = m_factory.create(); + auto ts = traj.makeTrackState(TrackStatePropMask::All); + + BOOST_CHECK_EQUAL(ts.calibratedSize(), MultiTrajectoryTraits::kInvalid); + + auto [par, cov] = generateBoundParametersCovariance(rng, {}); + + ts.template allocateCalibrated(par.head<3>(), cov.topLeftCorner<3, 3>()); + + BOOST_CHECK_EQUAL(ts.calibratedSize(), 3); + BOOST_CHECK_EQUAL(ts.template calibrated<3>(), par.head<3>()); + BOOST_CHECK_EQUAL(ts.template calibratedCovariance<3>(), + (cov.topLeftCorner<3, 3>())); + + auto [par2, cov2] = generateBoundParametersCovariance(rng, {}); + + ts.allocateCalibrated(3); + BOOST_CHECK_EQUAL(ts.template calibrated<3>(), ActsVector<3>::Zero()); + BOOST_CHECK_EQUAL(ts.template calibratedCovariance<3>(), + ActsSquareMatrix<3>::Zero()); + + ts.template allocateCalibrated(par2.head<3>(), cov2.topLeftCorner<3, 3>()); + BOOST_CHECK_EQUAL(ts.calibratedSize(), 3); + // The values are re-assigned + BOOST_CHECK_EQUAL(ts.template calibrated<3>(), par2.head<3>()); + BOOST_CHECK_EQUAL(ts.template calibratedCovariance<3>(), + (cov2.topLeftCorner<3, 3>())); + + // Re-allocation with a different measurement dimension is an error + BOOST_CHECK_THROW(ts.template allocateCalibrated( + par2.head<4>(), cov2.topLeftCorner<4, 4>()), + std::invalid_argument); + } }; } // namespace Acts::detail::Test diff --git a/Core/src/EventData/VectorMultiTrajectory.cpp b/Core/src/EventData/VectorMultiTrajectory.cpp index 66629d65f12..f25d64db4c5 100644 --- a/Core/src/EventData/VectorMultiTrajectory.cpp +++ b/Core/src/EventData/VectorMultiTrajectory.cpp @@ -212,6 +212,7 @@ void VectorMultiTrajectory::unset_impl(TrackStatePropMask target, case PM::Calibrated: m_measOffset[istate] = kInvalid; m_measCovOffset[istate] = kInvalid; + m_index[istate].measdim = kInvalid; break; default: throw std::domain_error{"Unable to unset this component"}; diff --git a/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp b/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp index e503f3ca2f3..53eebf0e9b4 100644 --- a/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp +++ b/Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp @@ -70,9 +70,9 @@ class PodioTrackStateContainerBase { case "smoothed"_hash: return data.ismoothed != kInvalid; case "calibrated"_hash: - return data.measdim != 0; + return data.measdim != kInvalid; case "calibratedCov"_hash: - return data.measdim != 0; + return data.measdim != kInvalid; case "jacobian"_hash: return data.ijacobian != kInvalid; case "projector"_hash: @@ -472,7 +472,7 @@ class MutablePodioTrackStateContainer final m_jacs->create(); data.ijacobian = m_jacs->size() - 1; } - data.measdim = 0; + data.measdim = kInvalid; data.hasProjector = false; if (ACTS_CHECK_BIT(mask, TrackStatePropMask::Calibrated)) { data.hasProjector = true; @@ -592,7 +592,7 @@ class MutablePodioTrackStateContainer final data.ijacobian = kInvalid; break; case TrackStatePropMask::Calibrated: - data.measdim = 0; + data.measdim = kInvalid; break; default: throw std::domain_error{"Unable to unset this component"}; @@ -615,10 +615,35 @@ class MutablePodioTrackStateContainer final {hashedKey, std::make_unique>(key)}); } - void allocateCalibrated_impl(IndexType istate, std::size_t measdim) { - assert(measdim > 0 && "Zero measdim not supported"); + template + void allocateCalibrated_impl(IndexType istate, + const Eigen::DenseBase& val, + const Eigen::DenseBase& cov) + + requires(Eigen::PlainObjectBase::RowsAtCompileTime > 0 && + Eigen::PlainObjectBase::RowsAtCompileTime <= eBoundSize && + Eigen::PlainObjectBase::RowsAtCompileTime == + Eigen::PlainObjectBase::RowsAtCompileTime && + Eigen::PlainObjectBase::RowsAtCompileTime == + Eigen::PlainObjectBase::ColsAtCompileTime) + { + constexpr std::size_t measdim = val_t::RowsAtCompileTime; + auto& data = PodioUtil::getDataMutable(m_collection->at(istate)); + + if (data.measdim != kInvalid && data.measdim != measdim) { + throw std::invalid_argument{ + "Measurement dimension does not match the allocated dimension"}; + } + data.measdim = measdim; + + Eigen::Map> valMap(data.measurement.data()); + valMap = val; + + Eigen::Map> covMap( + data.measurementCovariance.data()); + covMap = cov; } void setUncalibratedSourceLink_impl(IndexType istate, diff --git a/Tests/UnitTests/Core/EventData/MultiTrajectoryTests.cpp b/Tests/UnitTests/Core/EventData/MultiTrajectoryTests.cpp index 6a3c1aafc3e..421fcc3c62f 100644 --- a/Tests/UnitTests/Core/EventData/MultiTrajectoryTests.cpp +++ b/Tests/UnitTests/Core/EventData/MultiTrajectoryTests.cpp @@ -203,6 +203,11 @@ BOOST_AUTO_TEST_CASE(MultiTrajectoryExtraColumnsRuntime) { ct.testMultiTrajectoryExtraColumnsRuntime(); } +BOOST_AUTO_TEST_CASE(MultiTrajectoryAllocateCalibratedInit) { + CommonTests ct; + ct.testMultiTrajectoryAllocateCalibratedInit(rng); +} + BOOST_AUTO_TEST_CASE(MemoryStats) { using namespace boost::histogram; using cat = axis::category; diff --git a/Tests/UnitTests/Plugins/Podio/PodioTrackStateContainerTest.cpp b/Tests/UnitTests/Plugins/Podio/PodioTrackStateContainerTest.cpp index 2be4cc1411a..9169568fd60 100644 --- a/Tests/UnitTests/Plugins/Podio/PodioTrackStateContainerTest.cpp +++ b/Tests/UnitTests/Plugins/Podio/PodioTrackStateContainerTest.cpp @@ -202,6 +202,11 @@ BOOST_AUTO_TEST_CASE(MultiTrajectoryExtraColumnsRuntime) { ct.testMultiTrajectoryExtraColumnsRuntime(); } +BOOST_AUTO_TEST_CASE(MultiTrajectoryAllocateCalibratedInit) { + CommonTests ct; + ct.testMultiTrajectoryAllocateCalibratedInit(rng); +} + BOOST_AUTO_TEST_CASE(WriteToPodioFrame) { using namespace HashedStringLiteral; From 6d1f6f06046d72e6afa8f8ea23af1d3c5ac0995d Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 25 Nov 2024 16:00:16 +0100 Subject: [PATCH 04/10] fix benchmark --- Tests/Benchmarks/TrackEdmBenchmark.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Tests/Benchmarks/TrackEdmBenchmark.cpp b/Tests/Benchmarks/TrackEdmBenchmark.cpp index b063254e154..9e0842e0381 100644 --- a/Tests/Benchmarks/TrackEdmBenchmark.cpp +++ b/Tests/Benchmarks/TrackEdmBenchmark.cpp @@ -133,17 +133,17 @@ int main(int /*argc*/, char** /*argv[]*/) { trackState.typeFlags().set(TrackStateFlag::MaterialFlag); } else { BenchmarkSourceLink bsl{gid, 123}; - trackState.allocateCalibrated(measDimDist(rng)); + std::size_t measdim = measDimDist(rng); const auto& [predicted, covariance] = parameters(); trackState.predicted() = predicted; trackState.predictedCovariance() = covariance; visit_measurement( - trackState.calibratedSize(), + measdim, [&](std::integral_constant /*d*/) { - trackState.calibrated().setOnes(); - trackState.calibratedCovariance().setIdentity(); + trackState.allocateCalibrated(ActsVector::Ones(), + ActsSquareMatrix::Identity()); std::array indices{0}; std::iota(indices.begin(), indices.end(), 0); From 3d5f3a0f35ab1c6de1674afc6c8df6ccd6c6883a Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 25 Nov 2024 17:04:02 +0100 Subject: [PATCH 05/10] migrate edm clients to new overload --- Core/include/Acts/EventData/TrackProxy.hpp | 3 --- .../include/Acts/EventData/TrackStateProxy.hpp | 18 ++++++------------ .../TrackFinding/CombinatorialKalmanFilter.hpp | 1 - .../TrackFitting/src/RefittingCalibrator.cpp | 8 +++----- Examples/Framework/ML/src/NeuralCalibrator.cpp | 4 +--- .../src/EventData/MeasurementCalibration.cpp | 6 ++---- .../src/EventData/ScalingCalibrator.cpp | 4 +--- .../Core/EventData/TrackTestsExtra.cpp | 2 -- 8 files changed, 13 insertions(+), 33 deletions(-) diff --git a/Core/include/Acts/EventData/TrackProxy.hpp b/Core/include/Acts/EventData/TrackProxy.hpp index 6ccb0f402b2..4c8cd472d25 100644 --- a/Core/include/Acts/EventData/TrackProxy.hpp +++ b/Core/include/Acts/EventData/TrackProxy.hpp @@ -577,9 +577,6 @@ class TrackProxy { // append track states (cheap), but they're in the wrong order for (const auto& srcTrackState : other.trackStatesReversed()) { auto destTrackState = appendTrackState(srcTrackState.getMask()); - if (srcTrackState.hasCalibrated()) { - destTrackState.allocateCalibrated(srcTrackState.calibratedSize()); - } destTrackState.copyFrom(srcTrackState, Acts::TrackStatePropMask::All, true); } diff --git a/Core/include/Acts/EventData/TrackStateProxy.hpp b/Core/include/Acts/EventData/TrackStateProxy.hpp index f50e40d3ee7..822775ac74b 100644 --- a/Core/include/Acts/EventData/TrackStateProxy.hpp +++ b/Core/include/Acts/EventData/TrackStateProxy.hpp @@ -1016,17 +1016,14 @@ class TrackStateProxy { } if (ACTS_CHECK_BIT(src, PM::Calibrated)) { - allocateCalibrated(other.calibratedSize()); - // workaround for gcc8 bug: // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86594 auto* self = this; visit_measurement(other.calibratedSize(), [&](auto N) { constexpr int measdim = decltype(N)::value; - self->template calibrated() = - other.template calibrated(); - self->template calibratedCovariance() = - other.template calibratedCovariance(); + self->allocateCalibrated( + other.template calibrated().eval(), + other.template calibratedCovariance().eval()); }); setBoundSubspaceIndices(other.boundSubspaceIndices()); @@ -1064,17 +1061,14 @@ class TrackStateProxy { // may be not yet allocated if (ACTS_CHECK_BIT(mask, PM::Calibrated) && other.template has()) { - allocateCalibrated(other.calibratedSize()); - // workaround for gcc8 bug: // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86594 auto* self = this; visit_measurement(other.calibratedSize(), [&](auto N) { constexpr int measdim = decltype(N)::value; - self->template calibrated() = - other.template calibrated(); - self->template calibratedCovariance() = - other.template calibratedCovariance(); + self->allocateCalibrated( + other.template calibrated().eval(), + other.template calibratedCovariance().eval()); }); setBoundSubspaceIndices(other.boundSubspaceIndices()); diff --git a/Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp b/Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp index 315dd2379b0..0bc2e5680cc 100644 --- a/Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp +++ b/Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp @@ -464,7 +464,6 @@ class CombinatorialKalmanFilter { } // either copy ALL or everything except for predicted and jacobian - trackState.allocateCalibrated(candidateTrackState.calibratedSize()); trackState.copyFrom(candidateTrackState, mask, false); auto typeFlags = trackState.typeFlags(); diff --git a/Examples/Algorithms/TrackFitting/src/RefittingCalibrator.cpp b/Examples/Algorithms/TrackFitting/src/RefittingCalibrator.cpp index b54efd0dcb4..eaf2f74bffa 100644 --- a/Examples/Algorithms/TrackFitting/src/RefittingCalibrator.cpp +++ b/Examples/Algorithms/TrackFitting/src/RefittingCalibrator.cpp @@ -30,11 +30,9 @@ void RefittingCalibrator::calibrate(const Acts::GeometryContext& /*gctx*/, using namespace Acts; constexpr int Size = decltype(N)::value; - trackState.allocateCalibrated(Size); - trackState.template calibrated() = - sl.state.template calibrated(); - trackState.template calibratedCovariance() = - sl.state.template calibratedCovariance(); + trackState.allocateCalibrated( + sl.state.template calibrated().eval(), + sl.state.template calibratedCovariance().eval()); }); trackState.setBoundSubspaceIndices(sl.state.boundSubspaceIndices()); diff --git a/Examples/Framework/ML/src/NeuralCalibrator.cpp b/Examples/Framework/ML/src/NeuralCalibrator.cpp index 67c6e76db57..83e4d11354e 100644 --- a/Examples/Framework/ML/src/NeuralCalibrator.cpp +++ b/Examples/Framework/ML/src/NeuralCalibrator.cpp @@ -189,9 +189,7 @@ void ActsExamples::NeuralCalibrator::calibrate( calibratedCovariance(boundLoc0, boundLoc0) = output[iVar0]; calibratedCovariance(boundLoc1, boundLoc1) = output[iVar0 + 1]; - trackState.allocateCalibrated(kMeasurementSize); - trackState.calibrated() = calibratedParameters; - trackState.calibratedCovariance() = calibratedCovariance; + trackState.allocateCalibrated(calibratedParameters, calibratedCovariance); trackState.setSubspaceIndices(fixedMeasurement.subspaceIndices()); }); } diff --git a/Examples/Framework/src/EventData/MeasurementCalibration.cpp b/Examples/Framework/src/EventData/MeasurementCalibration.cpp index b4764844770..190d37153bb 100644 --- a/Examples/Framework/src/EventData/MeasurementCalibration.cpp +++ b/Examples/Framework/src/EventData/MeasurementCalibration.cpp @@ -41,10 +41,8 @@ void ActsExamples::PassThroughCalibrator::calibrate( static_cast>( measurement); - trackState.allocateCalibrated(kMeasurementSize); - trackState.calibrated() = fixedMeasurement.parameters(); - trackState.calibratedCovariance() = - fixedMeasurement.covariance(); + trackState.allocateCalibrated(fixedMeasurement.parameters().eval(), + fixedMeasurement.covariance().eval()); trackState.setSubspaceIndices(fixedMeasurement.subspaceIndices()); }); } diff --git a/Examples/Framework/src/EventData/ScalingCalibrator.cpp b/Examples/Framework/src/EventData/ScalingCalibrator.cpp index ae5c1eca4fe..2a956902513 100644 --- a/Examples/Framework/src/EventData/ScalingCalibrator.cpp +++ b/Examples/Framework/src/EventData/ScalingCalibrator.cpp @@ -178,9 +178,7 @@ void ActsExamples::ScalingCalibrator::calibrate( calibratedCovariance(boundLoc0, boundLoc0) *= ct.x_scale; calibratedCovariance(boundLoc1, boundLoc1) *= ct.y_scale; - trackState.allocateCalibrated(kMeasurementSize); - trackState.calibrated() = calibratedParameters; - trackState.calibratedCovariance() = calibratedCovariance; + trackState.allocateCalibrated(calibratedParameters, calibratedCovariance); trackState.setSubspaceIndices(fixedMeasurement.subspaceIndices()); }); } diff --git a/Tests/UnitTests/Core/EventData/TrackTestsExtra.cpp b/Tests/UnitTests/Core/EventData/TrackTestsExtra.cpp index ca444466216..f2f27ed8522 100644 --- a/Tests/UnitTests/Core/EventData/TrackTestsExtra.cpp +++ b/Tests/UnitTests/Core/EventData/TrackTestsExtra.cpp @@ -457,8 +457,6 @@ BOOST_AUTO_TEST_CASE(CopyTrackProxyCalibrated) { auto track1 = tc.makeTrack(); auto ts = track1.appendTrackState(TrackStatePropMask::Calibrated); ts.allocateCalibrated(kMeasurementSize); - ts.calibrated() = Vector3::Ones(); - ts.calibratedCovariance() = SquareMatrix3::Identity(); ts.setSubspaceIndices(BoundSubspaceIndices{}); auto tsCopy = track1.appendTrackState(TrackStatePropMask::Calibrated); From bb3193698b145bbcf32b375a9a0ce8e13b895b19 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 25 Nov 2024 17:06:00 +0100 Subject: [PATCH 06/10] add clearing of the output collection as well --- Tests/Benchmarks/TrackEdmBenchmark.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tests/Benchmarks/TrackEdmBenchmark.cpp b/Tests/Benchmarks/TrackEdmBenchmark.cpp index 9e0842e0381..a85b0482be2 100644 --- a/Tests/Benchmarks/TrackEdmBenchmark.cpp +++ b/Tests/Benchmarks/TrackEdmBenchmark.cpp @@ -111,6 +111,8 @@ int main(int /*argc*/, char** /*argv[]*/) { << std::endl; for (std::size_t r = 0; r < runs; ++r) { tc.clear(); + output.clear(); + for (std::size_t i = 0; i < nTracks; ++i) { auto track = tc.makeTrack(); From d9a548907967f1d351193416efda0d54025da4ab Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 26 Nov 2024 12:00:00 +0100 Subject: [PATCH 07/10] avoid double allocation --- Core/include/Acts/EventData/VectorMultiTrajectory.hpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp index ea99eff7a89..533f5f7044e 100644 --- a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp +++ b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp @@ -512,8 +512,6 @@ class VectorMultiTrajectory final if (m_measOffset[istate] == kInvalid || m_measCovOffset[istate] == kInvalid) { - m_index[istate].measdim = measdim; - m_measOffset[istate] = static_cast(m_meas.size()); m_meas.resize(m_meas.size() + measdim); @@ -523,16 +521,10 @@ class VectorMultiTrajectory final m_index[istate].measdim = measdim; - m_measOffset[istate] = static_cast(m_meas.size()); - m_meas.resize(m_meas.size() + measdim); - double* measPtr = &m_meas[m_measOffset[istate]]; Eigen::Map> valMap(measPtr); valMap = val; - m_measCovOffset[istate] = static_cast(m_measCov.size()); - m_measCov.resize(m_measCov.size() + measdim * measdim); - double* covPtr = &m_measCov[m_measCovOffset[istate]]; Eigen::Map> covMap(covPtr); covMap = cov; From 8c598005eb5f972dcc99b895287ac40da7caefae Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 26 Nov 2024 12:02:50 +0100 Subject: [PATCH 08/10] check lower bound on rows at compile time. this will also exclude dynamic eigen matrices --- Core/include/Acts/EventData/TrackStateProxy.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Core/include/Acts/EventData/TrackStateProxy.hpp b/Core/include/Acts/EventData/TrackStateProxy.hpp index 822775ac74b..901e608b9f0 100644 --- a/Core/include/Acts/EventData/TrackStateProxy.hpp +++ b/Core/include/Acts/EventData/TrackStateProxy.hpp @@ -883,7 +883,9 @@ class TrackStateProxy { template void allocateCalibrated(const Eigen::DenseBase& val, const Eigen::DenseBase& cov) - requires(Eigen::PlainObjectBase::RowsAtCompileTime == + requires(Eigen::PlainObjectBase::RowsAtCompileTime > 0 && + Eigen::PlainObjectBase::RowsAtCompileTime <= eBoundSize && + Eigen::PlainObjectBase::RowsAtCompileTime == Eigen::PlainObjectBase::RowsAtCompileTime && Eigen::PlainObjectBase::RowsAtCompileTime == Eigen::PlainObjectBase::ColsAtCompileTime) From ed1b576201755e38c98514316b83db3e946b3cd8 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 26 Nov 2024 12:05:55 +0100 Subject: [PATCH 09/10] sonar fixes --- Core/include/Acts/EventData/VectorMultiTrajectory.hpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp index 533f5f7044e..e13498cea2c 100644 --- a/Core/include/Acts/EventData/VectorMultiTrajectory.hpp +++ b/Core/include/Acts/EventData/VectorMultiTrajectory.hpp @@ -54,7 +54,7 @@ struct NonInitializingAllocator { NonInitializingAllocator() noexcept = default; template - NonInitializingAllocator( + explicit NonInitializingAllocator( const NonInitializingAllocator& /*other*/) noexcept {} template @@ -68,9 +68,10 @@ struct NonInitializingAllocator { std::allocator{}.deallocate(p, n); } - /// This construct function intentionally does not initialize the object! - /// Be very careful when using this allocator. - void construct(T* /*p*/) {} + void construct(T* /*p*/) const { + // This construct function intentionally does not initialize the object! + // Be very careful when using this allocator. + } }; class VectorMultiTrajectoryBase { From 59270a30fb5c8cd8799a16bfaa064744e29993f8 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 26 Nov 2024 12:14:06 +0100 Subject: [PATCH 10/10] clarify error type --- Core/include/Acts/EventData/TrackStateProxy.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/include/Acts/EventData/TrackStateProxy.hpp b/Core/include/Acts/EventData/TrackStateProxy.hpp index 901e608b9f0..3f7ddf1a028 100644 --- a/Core/include/Acts/EventData/TrackStateProxy.hpp +++ b/Core/include/Acts/EventData/TrackStateProxy.hpp @@ -879,7 +879,7 @@ class TrackStateProxy { /// @param val The measurement vector /// @param cov The covariance matrix /// @note This does not allocate if an allocation of the same size already exists - /// @note This is an error if an existing allocation has different size + /// @note This throws an exception if an existing allocation has different size template void allocateCalibrated(const Eigen::DenseBase& val, const Eigen::DenseBase& cov)