acts-project / acts-project/acts
Deprecating `BoundIndices`: full-tree survey of the 1554 remaining call sites, and where the accessor design does not yet reach
- Dominant language
- C++
- Stars
- 131
- Forks
- 276
- Avg merge
- 3d 13h
- Merged PRs (30d)
- 112
Description
## 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:
```cpp
// Core/include/Acts/EventData/VectorMultiTrajectory.hpp:520
// Plugins/EDM4hep/include/ActsPlugins/EDM4hep/PodioTrackStateContainer.hpp:988
requires(... Eigen::PlainObjectBase::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`:
```cpp
// 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 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()`.
```cpp
// 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**
```cpp
// 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**
```cpp
// Examples/Algorithms/Digitization/include/ActsExamples/Digitization/GeometricConfig.hpp
std::vector indices;
std::map> varianceMap = {};
// Fatras/include/ActsFatras/Digitization/UncorrelatedHitSmearer.hpp:53
std::array indices{};
```
Also `BoundIndices::eBoundLoc0`-style qualified access throughout `Core/src/Vertexing/`, and `template ` in `GsfComponentMerging.hpp`.
## Cluster 4 — Measurement / subspace index as a value · **170 sites, 26 files**
```cpp
// 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`:
```cpp
using SubspaceIndex = std::uint8_t;
template using SubspaceIndices = std::array;
using BoundSubspaceIndices = SubspaceIndices;
```
**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.**
```cpp
// 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**
```cpp
// Core/include/Acts/TrackFitting/detail/GsfComponentMerging.hpp:47
template
struct CyclicAngle {
constexpr static BoundIndices idx = Idx;
constexpr static double constant = 1.0;
};
template <> struct AngleDescription {
using Desc = std::tuple, CyclicAngle>;
};
```
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()`.
---
## One more pattern worth noting
`eBoundLoc0` is used as a **loop lower bound** — implicitly assuming it is zero and that the enum is contiguous:
```cpp
// 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?
Contributor guide
Assessment
This issue has not been assessed yet.