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

feat: Allow reflection of track parameters #3682

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Oct 2, 2024

Add helper functions to reflect track parameters. This is useful when seed parameters are estimated for the spacepoints in reverse direction which is necessary for strip seeds.


This pull request introduces functionality to reflect track parameters for both bound and free track parameters in the Acts framework. The key changes include adding new methods to reflect parameters and the corresponding implementation to handle the reflection logic.

New Methods for Reflecting Track Parameters:

Helper Functions for Reflection:

@andiwand andiwand added this to the next milestone Oct 2, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Event Data Model labels Oct 2, 2024
@timadye
Copy link
Contributor

timadye commented Oct 2, 2024

I think the API is a great addition.

The implementation seems to do a lot of copying around. Would it be better to have the helper functions do the in-place update, and use that in the track parameters' methods?

@andiwand
Copy link
Contributor Author

andiwand commented Oct 2, 2024

The implementation seems to do a lot of copying around. Would it be better to have the helper functions do the in-place update, and use that in the track parameters' methods?

True I thought it might be fine because of inlining but it is a source implementation. I am a bit in the middle of moving it to the header or providing a mutable reference. I am not a big fan of mutating input and use that as an output and I think the optimizer should achieve the same performance if it sees the implementation

@timadye
Copy link
Contributor

timadye commented Oct 2, 2024

True I thought it might be fine because of inlining but it is a source implementation. I am a bit in the middle of moving it to the header or providing a mutable reference. I am not a big fan of mutating input and use that as an output and I think the optimizer should achieve the same performance if it sees the implementation

Wow! I just checked with godbolt and it can indeed optimise it all away - as long as it is in a header. So I think putting the helper in a header is the best option.

Copy link

github-actions bot commented Oct 2, 2024

📊: Physics performance monitoring for 4188224

Full contents

physmon summary

paulgessinger
paulgessinger previously approved these changes Oct 17, 2024
@andiwand andiwand marked this pull request as ready for review October 17, 2024 12:37
Copy link
Contributor

@timadye timadye left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

Core/src/EventData/TransformationHelpers.cpp Outdated Show resolved Hide resolved
Co-authored-by: Tim Adye <T.J.Adye@rl.ac.uk>
@paulgessinger paulgessinger modified the milestones: next, v37.1.0 Oct 17, 2024
Copy link

sonarcloud bot commented Oct 17, 2024

@kodiakhq kodiakhq bot merged commit bf36f46 into acts-project:main Oct 17, 2024
42 checks passed
@andiwand andiwand deleted the feat-reflect-track-params branch October 17, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants