Skip to content

Deprecating BoundIndices: full-tree survey of the 1554 remaining call sites, and where the accessor design does not yet reach #5737

Description

@paulgessinger

Context

BoundIndices (and the eBoundLoc0 … eBoundTime enumerators) have been marked [[deprecated]], with eBoundSize replaced by a plain kBoundVectorSize constant. The end goal is to route component lookup through BoundTrackParameters with a surface argument where needed, so that at least the direction parameters (today phi & theta) can become surface-dependent.

The first step — adding the accessors and deprecating the enum — is done. This issue records the result of a full-tree survey of the resulting deprecation warnings, so we can plan the migration rather than grind through it file by file.

Survey method: full build (Core + Examples + Plugins + Tests + Python bindings, clang, all plugins except GNN/Mille/GBL/FPEMon), warnings deduplicated by file:line:col.

Result: 1554 unique deprecation sites across 120 files.

Tree Sites Files
Tests/ 674 40
Examples/ 522 40
Core/ 323 34
Plugins/ 28 3
Fatras/ 5 2
Python/ 2 1

Split by which parameter is being named:

  • 1006 sites name loc0 / loc1 / phi / theta — i.e. the parameters that are slated to become surface-dependent.
  • 343 sites name qOverP / time — surface-independent, mechanical to migrate.
  • 174 sites use BoundIndices as a type.

Blocking issue: main does not currently compile

Before any survey was possible, five hard errors had to be fixed. They fall into two root causes, both worth calling out because they generalise:

(a) kBoundVectorSize is no longer an enum, so toUnderlying() no longer applies:

// Core/include/Acts/EventData/VectorMultiTrajectory.hpp:520
// Plugins/EDM4hep/include/ActsPlugins/EDM4hep/PodioTrackStateContainer.hpp:988
requires(... Eigen::PlainObjectBase<val_t>::RowsAtCompileTime <=
             toUnderlying(kBoundVectorSize))   // error: no matching function

(b) kBoundVectorSize was being used as a "no such parameter" sentinel, and unsigned int no longer converts to BoundIndices:

// Examples/Algorithms/Digitization/include/ActsExamples/Digitization/SmearingConfig.hpp:21
Acts::BoundIndices index = Acts::kBoundVectorSize;   // error: cannot initialize

// Core/include/Acts/EventData/detail/TestSourceLink.hpp:42
std::array<BoundIndices, 2> indices = {kBoundVectorSize, kBoundVectorSize};  // error

This sentinel pattern is a design signal, not just a compile fix: whatever replaces BoundIndices needs an explicit "invalid/unused" value. Casting back to the deprecated enum at each site is a workaround, not a solution.

Plus one plain arity bug: TrackProxyCommon.hpp:213 calls BoundTrackParameters::qOverP(params, surface) but the static takes only (const BoundVector&)qOverP is not surface-dependent.


Cluster 1 — Scalar parameter access · 706 sites (45%), 76 files

v[eBoundPhi], v(eBoundLoc0), p.get<eBoundTheta>().

// Examples/Io/Root/src/RootTrackStatesWriter.cpp:458
truthParams[Acts::eBoundLoc0]  = truthLocal[Acts::ePos0];
truthParams[Acts::eBoundLoc1]  = truthLocal[Acts::ePos1];
truthParams[Acts::eBoundPhi]   = phi(truthUnitDir);
truthParams[Acts::eBoundTheta] = theta(truthUnitDir);
truthParams[Acts::eBoundTime]  = truthPos4[Acts::eTime];

This is the cluster the new BoundTrackParameters::phi(params, surface) statics already target. Two problems:

  1. 477 of these 706 sites name loc0/loc1/phi/theta and therefore need a surface, but the call sites overwhelmingly hold a bare BoundVector with no surface in scope. The cost here is threading a surface through, not the textual edit.
  2. There is no mutable local-position accessor. localPosition(const BoundVector&) returns Vector2 by value, so writes to loc0/loc1 — like the snippet above — have nowhere to go.

Worst offenders: Gx2fTests.cpp (48), ImpactPointEstimatorTests.cpp (45), RootTrackStatesWriter.cpp (43), RootTrackSummaryWriter.cpp (32), TransformHelpersTests.cpp (32).

Suggested avenue: add mutable localPosition(BoundVector&, const Surface&) returning a writable Eigen block (or an assignable proxy), so the read and write paths are symmetric. For the surface-threading problem, consider whether a lightweight BoundParametersRef{BoundVector&, const Surface&} view type would let call sites bind surface + vector once rather than passing the surface to every accessor.


Cluster 2 — Covariance elements · 366 sites (24%), 22 files

// Examples/Io/Csv/src/CsvTrackParameterWriter.cpp:76
data.cov_d0z0     = cov(Acts::eBoundLoc0, Acts::eBoundLoc1);
data.cov_d0phi    = cov(Acts::eBoundLoc0, Acts::eBoundPhi);
data.cov_d0theta  = cov(Acts::eBoundLoc0, Acts::eBoundTheta);
data.cov_phitheta = cov(Acts::eBoundPhi,  Acts::eBoundTheta);

This cluster has no replacement API at all today. It is the single largest uncovered gap and it is structurally harder than Cluster 1: a covariance entry is indexed by two parameters, so a named-accessor design has to answer what cov(phi, loc0) should be called, and how the surface argument enters when only one of the two indices is surface-dependent.

Good news: it is highly concentrated. CsvTrackParameterReader/Writer (102 combined), RootAthenaNTupleReader (50), PropagationTests.hpp (38), EDM4HepMeasurementTests.cpp (27) cover ~60%.

Suggested avenues:

  • A BoundTrackParameters::covariance(cov, surface) accessor returning a small wrapper with named 2-index lookup, e.g. c.get(BoundParam::Phi, BoundParam::Theta).
  • Or accept a scoped, non-deprecated index enum used only for covariance/matrix indexing, keeping the deprecation confined to parameter reads. This is probably worth considering regardless — see Cluster 5.

Cluster 3 — BoundIndices as a type · 174 sites, 38 files

// Examples/Algorithms/Digitization/include/ActsExamples/Digitization/GeometricConfig.hpp
std::vector<Acts::BoundIndices> indices;
std::map<Acts::BoundIndices, std::vector<double>> varianceMap = {};

// Fatras/include/ActsFatras/Digitization/UncorrelatedHitSmearer.hpp:53
std::array<Acts::BoundIndices, kSize> indices{};

Also BoundIndices::eBoundLoc0-style qualified access throughout Core/src/Vertexing/, and template <BoundIndices Idx> in GsfComponentMerging.hpp.

Cluster 4 — Measurement / subspace index as a value · 170 sites, 26 files

// Plugins/EDM4hep/src/EDM4hepUtil.cpp:320
auto loc0 = std::ranges::find(indices, eBoundLoc0);
auto loc1 = std::ranges::find(indices, eBoundLoc1);
auto time = std::ranges::find(indices, eBoundTime);

// Tests/UnitTests/Core/EventData/AnyTrackStateProxyTests.cpp:460
indices[0] = eBoundLoc0;
indices[1] = eBoundLoc1;
BOOST_CHECK_EQUAL(retrieved[0], eBoundLoc0);

Clusters 3 + 4 together: 344 sites (22%) — arguably not the same concept

These do not mean "read parameter X of a track". They mean "which components does this measurement constrain" — a subspace label. They can't be expressed through a surface-aware accessor at all, because there is no track and often no surface involved.

A parallel type already exists in Core/include/Acts/EventData/Types.hpp:

using SubspaceIndex = std::uint8_t;
template <std::size_t measdim> using SubspaceIndices = std::array<SubspaceIndex, measdim>;
using BoundSubspaceIndices = SubspaceIndices<kBoundVectorSize>;

Suggested avenue: promote this into a proper scoped enum (with an explicit invalid sentinel, per the blocking issue above) and migrate digitization configs, TestSourceLink, RootMeasurementIo, UncorrelatedHitSmearer, ModuleClusters, and the EDM4hep/Root measurement IO onto it. That takes ~22% of the migration off the critical path entirely and removes it from the surface-dependency question.


Cluster 5 — Jacobian / layout indices · 102 sites (7%), 14 files

This is the cluster that resists the accessor design.

// Core/src/Surfaces/DiscSurface.cpp:218
jacToGlobal.block<3, 1>(eFreePos0, eBoundLoc0) = lcphi * lx + lsphi * ly;
jacToGlobal(eFreeTime, eBoundTime) = 1;
jacToGlobal.block<3, 2>(eFreeDir0, eBoundPhi) =
    sphericalToFreeDirectionJacobian(direction);
jacToGlobal(eFreeQOverP, eBoundQOverP) = 1;

// Core/src/Vertexing/HelicalTrackLinearizer.cpp:120
completeJacobian(eBoundLoc0,  eLinPos0) = -sinPhi;
completeJacobian(eBoundLoc1,  eLinPhi)  = -d0 / tanTheta;
completeJacobian(eBoundPhi,   eLinPhi)  = 1.;
completeJacobian(eBoundTheta, eLinTheta) = 1.;

// Core/include/Acts/Propagator/AtlasStepper.hpp:255
pVector[8]  = transform(0, eBoundLoc0);
pVector[16] = transform(0, eBoundLoc1);

Here the bound index is a row/column offset into a rectangular matrix mixing bound × free / alignment / linearization axes. There is no "read parameter X of a track" happening — the code is asserting the memory layout of BoundVector. A named accessor cannot express this.

Concentrated in HelicalTrackLinearizer.cpp (28), CurvilinearSurfaceTests.cpp (24), Core/src/Surfaces/{DiscSurface,Surface,CurvilinearSurface,LineSurface,PointSurface}.cpp (~32 combined), GlobalChiSquareFitter.hpp, PointwiseMaterialInteraction.hpp.

Suggested avenue: introduce a blessed, non-deprecated internal layout constant set (e.g. detail::BoundLayout::kLoc0, kPhi, …) that these sites use explicitly. Without it we will end up with permanent ACTS_PUSH_IGNORE_DEPRECATED() blocks in Core/src/Surfaces/ — which is precisely where a surface-dependent parametrization most needs to be correct and reviewable.


Cluster 6 — Compile-time template arguments · 36 sites, 6 files

// Core/include/Acts/TrackFitting/detail/GsfComponentMerging.hpp:47
template <BoundIndices Idx>
struct CyclicAngle {
  constexpr static BoundIndices idx = Idx;
  constexpr static double constant = 1.0;
};

template <> struct AngleDescription<Surface::Disc> {
  using Desc = std::tuple<CyclicAngle<eBoundLoc1>, CyclicAngle<eBoundPhi>>;
};

Needs a constexpr-usable index type; a runtime accessor cannot substitute. Note the interesting wrinkle: AngleDescription is already keyed on Surface::SurfaceType, i.e. this code has independently discovered that the meaning of the local parameters is surface-dependent. Worth using as a design reference.

Also Tests/IntegrationTests/PropagationTests.hpp (12) and the parameter-test files, mostly via get<eBoundX>().


One more pattern worth noting

eBoundLoc0 is used as a loop lower bound — implicitly assuming it is zero and that the enum is contiguous:

// Core/src/Seeding/EstimateTrackParamsFromSeed.cpp:179
for (std::size_t i = eBoundLoc0; i < kBoundVectorSize; ++i) {
  double sigma = config.initialSigmas[i];
  ...
  if (i == eBoundQOverP) { ... }
  if (i == eBoundTime && !hasTime) { ... }
}

Any replacement needs to say whether iterating the parameter vector by index remains supported, and if so through what.


Proposed ordering

  1. Fix the build — the five errors above (trivial, unblocks everything).
  2. Decide the covariance API (Cluster 2, 24%). Until it exists, a quarter of the migration is blocked, and it constrains the shape of the whole design.
  3. Split off the subspace-index concept (Clusters 3+4, 22%). Independent of the surface question, parallelisable, removes the sentinel problem.
  4. Add a non-deprecated layout constant set for Clusters 5+6 (138 sites) — otherwise these become permanent suppression blocks in exactly the wrong place.
  5. Then migrate Cluster 1 (45%), which is the bulk but also the most mechanical once the surface-threading pattern is settled.

Steps 2–4 are the ones that need a design decision. Step 5 is largely legwork and can be split per-directory across contributors.


Open questions

  • What is cov(phi, loc0) called once phi is surface-dependent but loc0's meaning is fixed by the same surface? Does the covariance accessor take one surface, or is the surface implicit in the whole matrix?
  • Should qOverP / time (343 sites, surface-independent) migrate on the same schedule, or be left alone until the surface-dependent ones land?
  • Does BoundVector remain index-addressable at all for Core-internal code (Cluster 5), or is the intent to eliminate positional access entirely?

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions