EDM De-Algebraification #1, main branch (2026.04.08.) by krasznaa · Pull Request #1287 · acts-project/traccc

krasznaa · 2026-04-08T10:01:54Z

With this let me start yet another big rewrite / redesign. 🤔

It's been bugging me for a long time that the data model we use depends on the algebra that we use. Which in my mind is not a workable API. The data itself needs to be fixed somehow.

So I started by making traccc::edm::silicon_cell and traccc::edm::measurement not be dependent on the build setup of the project. As these seemed technically the easiest to do, and since edm::measurement is used pretty much everywhere, I thought that this would be a good stress-test for the rewrite.

Making edm::silicon_cell always use float values for its "activation" and "time" properties was a trivial change, that didn't actually necessitate any code changes. So that was surprisingly easy.

But of course edm::measurement was a bit more tricky. What we do currently is that we use the detray::point2D type for storing the local coordinates and the variances of those local coordinates. But I believe it's a lot better idea to just store plain std::array<float, 2> objects. Independent of what algebra we want to use for our calculations.

To help with this, I added a couple of additional functions to edm::measurement. Which would help with both retrieving and setting the local_position and local_variance variables using Detray objects directly. Just so that the actual algorithmic code would look a bit more "natural". Like:

    const point3 global = sf.local_to_global(
        gctx,
        meas.template local_position_in<typename detector_t::algebra_type>(),
        {});

(Okay, maybe "natural" is a bit over-selling it. 🤔)

I see this all as one step towards hiding the whole algebra business behind the scenes. As the API of the GPU tools should really not reveal what algebra is being used underneath. In case we end up using array and Eigen algebras interchangeably in the end after all. 🤔

I assume there will be plenty of discussion around this. Especially around what we'll do with edm::track_state afterwards. (As there I do have some worries about what we actually want to do.) But let's see how this goes...

core/include/traccc/edm/impl/measurement_helpers.ipp

core/include/traccc/edm/measurement_collection.hpp

stephenswat

Removing the algebra from the EDM is largely uncontroversial, so I'm on board with this. The only real issue with this PR is that it tasks the EDM with the responsibility of type conversions where that should really be the callers' responsibility.

stephenswat · 2026-04-08T10:56:23Z

core/include/traccc/utils/subspace.hpp

+    template <typename SIZE_TYPE>
+    TRACCC_HOST_DEVICE constexpr subspace(
+        const std::array<SIZE_TYPE, kSize>& indices) {


Why would we not just use the size_type that is already defined in the class?

size_type in this class is actually algebra specific. For std::array it's std::size_t. For some other algebras it would be unsigned int.

And I'm here wondering that we're wasting a lot of memory on this. For now I went with unsigned int in edm::measurement. But even std::uint8_t should be enough I reckon. 🤔

Absolutely yes, this could be a smaller integer type!

stephenswat · 2026-04-08T10:59:22Z

core/include/traccc/edm/measurement_collection.hpp

+    /// Set the subspace of the measurement
+    ///
+    /// Using a possibly slightly different array than the one used by the
+    /// object itself.
+    ///
+    /// @note This function must only be used on proxy objects, not on
+    ///       containers!
+    ///
+    /// @param subs The subspace to set
+    ///
+    template <std::integral TYPE>
+    TRACCC_HOST_DEVICE void set_subspace(const std::array<TYPE, 2u>& subs);


Rather than templating this function on the size type of subspace, callers of this function should rather ignore the size type of their local algebra and simply use subspace::size_type.

The whole reason that algebras advertise their size_type is that we had issues with using std::size_t versus unsigned int with the various algebras. With compilers freaking out about type conversions.

stephenswat · 2026-04-08T11:01:08Z

core/include/traccc/edm/measurement_collection.hpp

+    /// Get the local variance object in a specific (Detray) algebra
+    ///
+    /// @note This function must only be used on proxy objects, not on
+    ///       containers!
+    ///
+    /// @tparam ALGEBRA_TYPE The algebra to retrieve the local variance in
+    /// @return An algebra specific 1D/2D point object
+    ///
+    template <detray::concepts::algebra ALGEBRA_TYPE>
+    TRACCC_HOST_DEVICE detray::dpoint2D<ALGEBRA_TYPE> local_variance_in() const;
+    /// Set the local variance expressed in a specific (Detray) algebra
+    ///
+    /// @note This function must only be used on proxy objects, not on
+    ///       containers!
+    ///
+    /// @tparam ALGEBRA_TYPE The algebra to set the local variance in
+    /// @param var The local variance to set
+    ///
+    template <detray::concepts::algebra ALGEBRA_TYPE>
+    TRACCC_HOST_DEVICE void set_local_variance_in(
+        const detray::dpoint2D<ALGEBRA_TYPE>& var);


Embedding all of this conversion logic into this type is poor API design. It would be nicer to introduce a few free functions which can convert a detray::dpoint2D from any given algebra to a canonical standard type, and it should then be the responsibility of the call site to ensure that they produce a dpoint2D of the right type.

I guess we can have a free-standing function that can convert between whatever dpoint2D and std::array<float, 2>. But I'm not quite as convinced as you that it's hurtful to have the EDM class know exactly what type it itself has. For me this seemed like a quite good place to put such a conversion in.

The free function approach avoids quite a lot of awkwardness at both the call site (no need to call a templated member function) and the EDM (no need to define separate getters and setters, no need to duplicate conversion code, no need to include the algebra in a template variable).

This EDM design has four functions for each dpoint2D member, where only two are really necessary (both of which are not templated):

TRACCC_HOST_DEVICE auto& local_position() { return BASE::template get<0>(); } TRACCC_HOST_DEVICE const auto& local_position() const { return BASE::template get<0>(); } template <detray::concepts::algebra ALGEBRA_TYPE> TRACCC_HOST_DEVICE detray::dpoint2D<ALGEBRA_TYPE> local_position_in() const; template <detray::concepts::algebra ALGEBRA_TYPE> TRACCC_HOST_DEVICE void set_local_position_in( const detray::dpoint2D<ALGEBRA_TYPE>& pos);

I'm curious about your proposal. 😕 What exact arguments would those free-standing functions use?

If we want to make use of detray::dpoint2D, I don't think there's a way to avoid specifying explicitly which algebra we're using. As it's not possible to figure out purely from dpoint2D<T> what T should be. And I also would like to avoid using explicit Eigen, etc. types directly in the traccc code.

So, what did you have in mind exactly? 🤔

I'm suggesting we could have the following free functions:

template<typename T> std::array<float, 2u> to_canonical_point2d(const T & p) requires detray::concepts::point<T> { ... } template<typename Algebra> detray::dpoint2D<Algebra> from_canonical_point2d(const std::array<float, 2u> & p) { ... }

Since there were some pre-existing helper functions in measurement_helpers.hpp, I added functions there in the same style as those already existing things. It did not make an amazing API though. 😦

Still, client code will not need to be exposed to this type of code...

stephenswat · 2026-04-08T11:01:55Z

core/include/traccc/edm/impl/measurement_collection.ipp

+template <typename BASE>
+template <detray::concepts::algebra ALGEBRA_TYPE>
+TRACCC_HOST_DEVICE detray::dpoint2D<ALGEBRA_TYPE>
+measurement<BASE>::local_position_in() const {
+
+    detray::dpoint2D<ALGEBRA_TYPE> result;
+    getter::element(result, 0) =
+        static_cast<typename ALGEBRA_TYPE::value_type>(local_position()[0]);
+    getter::element(result, 1) =
+        static_cast<typename ALGEBRA_TYPE::value_type>(local_position()[1]);
+    return result;
+}
+
+template <typename BASE>
+template <detray::concepts::algebra ALGEBRA_TYPE>
+TRACCC_HOST_DEVICE void measurement<BASE>::set_local_position_in(
+    const detray::dpoint2D<ALGEBRA_TYPE>& pos) {
+
+    local_position()[0] = static_cast<float>(getter::element(pos, 0));
+    local_position()[1] = static_cast<float>(getter::element(pos, 1));
+}
+
+template <typename BASE>
+template <detray::concepts::algebra ALGEBRA_TYPE>
+TRACCC_HOST_DEVICE detray::dpoint2D<ALGEBRA_TYPE>
+measurement<BASE>::local_variance_in() const {
+
+    detray::dpoint2D<ALGEBRA_TYPE> result;
+    getter::element(result, 0) =
+        static_cast<typename ALGEBRA_TYPE::value_type>(local_variance()[0]);
+    getter::element(result, 1) =
+        static_cast<typename ALGEBRA_TYPE::value_type>(local_variance()[1]);
+    return result;
+}
+
+template <typename BASE>
+template <detray::concepts::algebra ALGEBRA_TYPE>
+TRACCC_HOST_DEVICE void measurement<BASE>::set_local_variance_in(


Case in point, this is all just duplicated code.

Made both of them use a concrete, FP32 interface. One that doesn't depend on how traccc was built exactly. Updated all clients in the core library to successfully use that fixed interface. Even when traccc is built in FP64 mode.

According to PR recommendations. Though I'm not completely happy with the "user code" that this modified API resulted in.

sonarqubecloud · 2026-04-09T11:27:08Z

Quality Gate passed

Issues
18 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
7.6% Duplication on New Code

See analysis details on SonarQube Cloud

krasznaa requested review from niermann999 and stephenswat April 8, 2026 10:01

krasznaa added the edm Changes to the data model label Apr 8, 2026

krasznaa commented Apr 8, 2026

View reviewed changes

core/include/traccc/edm/impl/measurement_helpers.ipp Outdated Show resolved Hide resolved

krasznaa commented Apr 8, 2026

View reviewed changes

core/include/traccc/edm/measurement_collection.hpp Outdated Show resolved Hide resolved

krasznaa force-pushed the EDMDeAlgebraification-main-20260401 branch from f0da9fb to de91c95 Compare April 8, 2026 10:53

stephenswat reviewed Apr 8, 2026

View reviewed changes

krasznaa added 10 commits April 9, 2026 13:24

Concretized the cells and measurements.

ed3a034

Made both of them use a concrete, FP32 interface. One that doesn't depend on how traccc was built exactly. Updated all clients in the core library to successfully use that fixed interface. Even when traccc is built in FP64 mode.

Adapted traccc::io to the concretized measurements.

638f034

Adapted the core device code to the cell+measurement concretization.

9d2a999

Adapted traccc::alpaka to the cell+measurement concretization.

8537ff1

Adapted traccc::cuda to the cell+measurement concretization.

e44b9a4

Adapted traccc::sycl to the cell+measurement concretization.

bb1d6a8

Adapted traccc::performance to the cell+measurement concretization.

caf8985

Adapted all examples to the cell+measurement concretization.

2bf4cd8

Adapted all tests to the cell+measurement concretization.

9b837de

Modified the API of edm::measurement.

edcd9a6

According to PR recommendations. Though I'm not completely happy with the "user code" that this modified API resulted in.

krasznaa force-pushed the EDMDeAlgebraification-main-20260401 branch from de91c95 to edcd9a6 Compare April 9, 2026 11:24

Conversation

krasznaa commented Apr 8, 2026

Uh oh!

Uh oh!

Uh oh!

stephenswat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenswat Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 9, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stephenswat Apr 8, 2026 •

edited

Loading