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

Open
#5,737 0 comments 0 reactions 0 assignees View on GitHub
Component - Core Event Data Model Improvement
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.