Skip to content

refactor: Move logic into TrackerHitLocal#5234

Merged
kodiakhq[bot] merged 15 commits intoacts-project:mainfrom
paulgessinger:refactor/trackerhitlocal-api
Mar 21, 2026
Merged

refactor: Move logic into TrackerHitLocal#5234
kodiakhq[bot] merged 15 commits intoacts-project:mainfrom
paulgessinger:refactor/trackerhitlocal-api

Conversation

@paulgessinger
Copy link
Member

This moves the subspace encoding / decoding into headers that are located with the EDM.

@github-actions github-actions bot added the Component - Plugins Affects one or more Plugins label Mar 11, 2026
@github-actions github-actions bot added this to the next milestone Mar 11, 2026
@paulgessinger paulgessinger force-pushed the refactor/trackerhitlocal-api branch from cc4e5c1 to 7dbd485 Compare March 11, 2026 17:23
@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

📊: Physics performance monitoring for 63c0fc6

Full contents

physmon summary

@paulgessinger paulgessinger requested a review from tmadlener March 12, 2026 16:11
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, this looks good, I think. There are a few technical details below and the question(s) on how to best fulfill the edm4hep::TrackerHit interface (which, I suppose would be part of the final goal).

The accessors to [gs]etValue and [gs]etCov are currently std::size_t. This should not block something like what is done in key4hep/EDM4hep#170 with accessing them via some enum to improve the ergonomics a bit, right? IIUC there is enough information encoded into the type here such that we can map between enum values (e.g. the Acts::BoundIndices) and the size_t?

@paulgessinger
Copy link
Member Author

@tmadlener I can change the std::size_t arguments to templated integrals, but that wouldn't automatically implement the full "interpretation" mechanism that you added in that PR. The other point is about fulfilling the TrackerHit interface is a bit tricky. Realistically, I can't always fill the members with meaningful numbers. I don't fully know how the interface mechanism works, so e.g. if I could throw in case global position or time is not available.

@tmadlener
Copy link
Contributor

Does the current std::size_t argument allow me to pass in e.g. Acts::eBoundLoc0? IIUC, it should because it's not an enum class but just an enum. The main point I want to make is that there should be a 1:1 mapping between what the users use to index into the measurements and the covariance matrices and some constants that are defined somewhere. (As opposed of having to use a dedicated set of values here). That is the intended way of using this, right?

Regarding the interfaces it is not strictly necessary that a datatype provides the functionality that the interface requires via Members. You can also add dummy implementations via ExtraCode, as long as the getter names match. If you fail to do that the datamodel will fail to compile but (IIRC) we don't do any dedicated validation on the podio side to ensure this during code generation.

As an example from the podio tests:

@paulgessinger
Copy link
Member Author

@tmadlener I added the redirect mechanism now, where you can use an enum to access named components by looking up the local indices in the type variable. Let me know what you think!

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. I don't think we can do much more QoL here without without having to invent a lot of new stuff.

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Mar 19, 2026
@sonarqubecloud
Copy link

@kodiakhq kodiakhq bot merged commit 7a22856 into acts-project:main Mar 21, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component - Plugins Affects one or more Plugins Infrastructure Changes to build tools, continous integration, ...

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants