Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Rework projector #3453

Merged
merged 26 commits into from
Aug 19, 2024
Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Jul 29, 2024

Reworks the projector storage in the track EDM and the operations on vectors matrices. The bitset in the track EDM is replaced by a simple sequence of std::uint8_t which provide the mapping measurement index -> bound index. A helper is provided, SubspaceHelper, which collects convenience functions that convert to the legacy format and to matrix form and also do operations on vectors and matrices e.g. projectVector and projectMatrix.

The benefit of this change is that the mapping and storage of the mapping becomes easier to grasp and also computationally less expensive. Instead of relying on eigen matrix operations we map directly to the desired object.

A similar change in Athena improved the overall track finding CPU time by 30%.

In favor of SubspaceHelper I removed FixedSizeSubspace and VariableSizeSubspace and made the necessary changes in the Examples.

In principle these changes should be non-breaking. The switch in API should be scheduled for v37.

@andiwand andiwand added the 🚧 WIP Work-in-progress label Jul 29, 2024
@andiwand andiwand added this to the next milestone Jul 29, 2024
@andiwand
Copy link
Contributor Author

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Seems like a good idea overall if this is cheaper to compute.

The other enums changing underlying type is unrelated, right?

Core/include/Acts/EventData/SubspaceHelpers.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/ConeVolumeBounds.hpp Outdated Show resolved Hide resolved
@andiwand
Copy link
Contributor Author

andiwand commented Jul 29, 2024

This seems to be ~17% faster out of the box

image

There might be a bit more potential because I did not change the calibrator yet well actually I did 😄

@paulgessinger
Copy link
Member

17% is wild. I guess it's because in the ODD chain we still run the projection quite a lot for calibrating every single measurement.

@andiwand
Copy link
Contributor Author

I expected a bit more because @goetzgaycken got about 30% in Athena with a similar change but maybe it is due to the geometry and the eta coverage somehow

paulgessinger
paulgessinger previously approved these changes Aug 19, 2024
@andiwand andiwand marked this pull request as draft August 19, 2024 09:47
@andiwand andiwand marked this pull request as ready for review August 19, 2024 13:04
@paulgessinger paulgessinger modified the milestones: next, v36.2.0 Aug 19, 2024
@andiwand andiwand modified the milestones: v36.2.0, next Aug 19, 2024
Copy link

sonarcloud bot commented Aug 19, 2024

@kodiakhq kodiakhq bot merged commit a28acc3 into acts-project:main Aug 19, 2024
42 checks passed
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Aug 19, 2024
@andiwand andiwand deleted the rework-projector branch August 20, 2024 05:24
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Aug 20, 2024
andiwand added a commit to andiwand/acts that referenced this pull request Aug 21, 2024
kodiakhq bot pushed a commit that referenced this pull request Aug 21, 2024
#3453 is causing more downstream than expected and is on the edge of a breaking change. We will revert this and delay it until v37
@paulgessinger paulgessinger modified the milestones: next, v36.2.0 Aug 26, 2024
kodiakhq bot pushed a commit that referenced this pull request Aug 30, 2024
Originally #3453

---

Reworks the projector storage in the track EDM and the operations on vectors matrices. The bitset in the track EDM is replaced by a simple sequence of `std::uint8_t` which provide the mapping `measurement index -> bound index`. A helper is provided, `SubspaceHelper`, which collects convenience functions that convert to the legacy format and to matrix form and also do operations on vectors and matrices e.g. `projectVector` and `projectMatrix`.

The benefit of this change is that the mapping and storage of the mapping becomes easier to grasp and also computationally less expensive. Instead of relying on eigen matrix operations we map directly to the desired object.

A similar change in Athena improved the overall track finding CPU time by 30%.

In favor of `SubspaceHelper` I removed `FixedSizeSubspace` and `VariableSizeSubspace` and made the necessary changes in the Examples.

In principle these changes should be non-breaking. The switch in API should be scheduled for v37.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Event Data Model Fails Athena tests This PR causes a failure in the Athena tests Track Finding Track Fitting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants