search

acts-project / acts

Experiment-independent toolkit for (charged) particle track reconstruction in (high energy) physics experiments implemented in modern C++
https://acts.readthedocs.io
Mozilla Public License 2.0
106 stars 172 forks source link

refactor!: move source link creation, track state creation, measurement selection into a single unit outside of the CKF #3825

Open goetzgaycken opened 3 weeks ago

goetzgaycken commented 3 weeks ago

Merge retrieval of source link ranges, measurement selection and track state creation into one unit which the CKF interacts with via a single delegate. The delegates for measurement selection track state creation and calibration are removed from the CKF options/extensions.

The original building blocks (SourceLink accessor, measurement selector, and track state creator, or the combined measurement selector and track state creator) can still be used, however require to setup an additional helper object which combines these independent components. The algorithmic code is unchanged.

As a prerequisite the track states are converted to bound states also on empty sensitive surfaces.

This increases the computational effort slightly for empty, sensitive surfaces, since the computation of bound states is slightly more demanding than the computation of curvilinear states, but for complex events like HL-LHC events the degradation was not measurable.

--- END COMMIT MESSAGE ---

Any further description goes here, @-mentions are ok here!

Summary by CodeRabbit

Release Notes

coderabbitai[bot] commented 3 weeks ago

Walkthrough

Significant changes made to the CombinatorialKalmanFilter and related files. The CombinatorialKalmanFilter class has been restructured, removing the CombinatorialKalmanFilterBranchStopperResult enum and CombinatorialKalmanFilterExtensions struct, integrating their functionalities directly into the class. The template parameters for CombinatorialKalmanFilterOptions have been simplified. New files for TrackStateCreator and CombinatorialKalmanFilterExtensions were added, introducing new types and methods for track state management. The TrackFindingAlgorithm and tests were updated to reflect these changes, enhancing clarity and maintainability.

Changes

File Change Summary
Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp Removed CombinatorialKalmanFilterBranchStopperResult enum and CombinatorialKalmanFilterExtensions struct; simplified template parameters for CombinatorialKalmanFilterOptions and findTracks method.
Core/include/Acts/TrackFinding/CombinatorialKalmanFilterExtensions.hpp Added new file defining CombinatorialKalmanFilterExtensions, including a new enum for branch stopper results and various type aliases.
Core/include/Acts/TrackFinding/TrackStateCreator.hpp Introduced TrackStateCreator struct with methods for creating track states and managing measurements.
Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp Updated TrackFinderOptions to simplify template parameters.
Examples/Algorithms/TrackFinding/src/TrackFindingAlgorithm.cpp Integrated TrackStateCreator and adjusted measurement selection in the execute method.
Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp Updated tests to incorporate TrackStateCreator and simplified CombinatorialKalmanFilterOptions.
docs/_extensions/lazy_autodoc.py Added entry for Acts::TrackStateCreator in the documentation generator's role_instances.

Possibly related PRs

Suggested labels

automerge

Suggested reviewers

In the code, changes abound,
Simplified paths, clarity found.
Track states created with ease,
New structures bring a gentle breeze.
Jedi wisdom guides the way,
In the realm of code, we play! ✨

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
🪧 Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit , please review it.` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (Invoked using PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. ### Other keywords and placeholders - Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. - Add `@coderabbitai summary` to generate the high-level summary at a specific location in the PR description. - Add `@coderabbitai` anywhere in the PR title to generate the title automatically. ### CodeRabbit Configuration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://docs.coderabbit.ai) for detailed information on how to use CodeRabbit. - Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
goetzgaycken commented 3 weeks ago
github-actions[bot] commented 3 weeks ago

📊: Physics performance monitoring for 56d00518687d870595fa46370908bc55898aca99

Full contents

physmon summary

goetzgaycken commented 3 weeks ago

changes d3a565e29b216f8ed7c6f69c89edb09c35a84a68 to b44b8fe89abae78002c327ff462b54ed7828be29:

goetzgaycken commented 3 weeks ago

Why should there be brackets arround slAccessorDelegate TrackFindingAlgorithm.cpp:365 (macos build complains):

364 | TrackStateCandidateCreatorType trackStateCreator{
365 |    slAccessorDelegate,
366 |    Acts::makeDelegate<&MeasurementCalibratorAdapter::calibrate>(calibrator),
367 |    Acts::makeDelegate<&MeasurementSelector::select>(measSel)};
paulgessinger commented 2 weeks ago

Couple more thoughts on this:

I think going with a single level of composition would be clearer. Currently there's the base composable class that connects to the previous SourceLinkAccessor, and then a second layer which accepts the other original delegates.

A single composable struct that by itself has the three (?) original delegates including the SourceLinkAccessor would be sufficient if we drop the compatibility with the intermediate track state creator style delegate.

What do we need this second layer for after this change? I think having one layer of adapter to the fully separate makes sense, but the surface area of the track state creator seems small enough to me to remove that altogether.

goetzgaycken commented 2 weeks ago

It is only needed for the transition period. In my opinion it would be better to first introduce the new api and then merge on the athena side the track state creator and source link accessor. The amount of code duplication when introducing different helper objects for the two cases is small, though the simplification is also not significant. Therefore I would rather not duplicate the code, and rather remove the unnecessary code and layers later when they are not needed anymore. But I do not have a strong opinion.

andiwand commented 2 weeks ago

The question for me would be, do we need a transition period? The change is breaking either way no? Touching this twice means we have to go through two breaking cycles.

goetzgaycken commented 2 weeks ago

The question for me would be, do we need a transition period? The change is breaking either way no? Touching this twice means we have to go through two breaking cycles.

It is certainly possible to merge at the same time on the athena side the source link accessor and track state creator. I don't have any strong argument to motivate why it is better to break this into smaller steps.

goetzgaycken commented 2 weeks ago

Changes from b44b8fe to b00f570 :

changes from b00f570 to 2cc2ecc:

goetzgaycken commented 1 week ago

changes from 2cc2ecc to dbfba96 :

goetzgaycken commented 4 days ago

changes from 2fa962c to 9493657:

goetzgaycken commented 4 days ago

Open discussion points:

goetzgaycken commented 3 days ago

changes from 9493657 to abedde4:

sonarcloud[bot] commented 3 days ago

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

andiwand commented 3 days ago

Open discussion points:

my 2 cents

  • should the new extension member be prefixed with "m_" although non of the others are prefixed in this way ? Extend this PR and add the prefix for all members ?

The lose guideline here is that for structs we don't use m_ but only for classes.

  • is the name "extendOrBranchTrajectory" a good name for the delegate ? Or should it be called trackStateCreator i.e the delegate this is kind of replacing (together with the sourceLinkAccessor)

I would be in favor of keeping trackStateCreator

  • Remove TrackStateCreatorDelegate which provides a helper which allows to use the previous track state create interface currently used by athena ? This would require to directly implement the new interface on the athena side. The idea is that e.g. the minimal changes to support the new interface would be simpler: (cern-gitlab:f8ac37ab3f, to directly support the new interface more changes are needed: cern-gitlab:171095fc26

To me it seems easier to make one sharp cut and break the API directly without a transition phase. I think Atlas is the only experiment using this delegate so it is really up to you IMO. What is easier for you?

  • Rename TrackStateCandidateCreatorImpl ? To what ?

I wondered if we remove the compatibility layer for the existing TrackStateCreator if we could/should collapse ComposableTrackStateCreator and TrackStateCandidateCreatorImpl into one class?

  • Remove the buffer trajectory etc. from the new interface ? This would require that the implementation of the ComposableTrackStateCreator (TrackStateCandidateCreatorImpl) would have to provide this i.e. would gain this as a mutable member.

I think this would be good to remove. In case we drop the existing TrackStateCreator this would come for free?

goetzgaycken commented 3 days ago
  • should the new extension member be prefixed with "m_" although non of the others are prefixed in this way ? Extend this PR and add the prefix for all members ?

The lose guideline here is that for structs we don't use m_ but only for classes.

advantage "m_" prefix:

disadvantage "m_" prefix:

  • is the name "extendOrBranchTrajectory" a good name for the delegate ? Or should it be called trackStateCreator i.e the delegate this is kind of replacing (together with the sourceLinkAccessor)

I would be in favor of keeping trackStateCreator

ok.

  • Remove TrackStateCreatorDelegate which provides a helper which allows to use the previous track state create interface currently used by athena ? This would require to directly implement the new interface on the athena side. The idea is that e.g. the minimal changes to support the new interface would be simpler: (cern-gitlab:f8ac37ab3f, to directly support the new interface more changes are needed: cern-gitlab:171095fc26

To me it seems easier to make one sharp cut and break the API directly without a transition phase. I think Atlas is the only experiment using this delegate so it is really up to you IMO. What is easier for you?

Certainly reduces the amount of new code.

  • Rename TrackStateCandidateCreatorImpl ? To what ?

I wondered if we remove the compatibility layer for the existing TrackStateCreator if we could/should collapse ComposableTrackStateCreator and TrackStateCandidateCreatorImpl into one class?

Indeed, if there is only a single implementation of ComposableTrackStateCreator there would not be a need to split off a derived class.

  • Remove the buffer trajectory etc. from the new interface ? This would require that the implementation of the ComposableTrackStateCreator (TrackStateCandidateCreatorImpl) would have to provide this i.e. would gain this as a mutable member.

I think this would be good to remove. In case we drop the existing TrackStateCreator this would come for free?

The Acts measurement selector needs the buffer trajectory, so it would be moved from the CKF result and become a member of the track state creator helper class (TrackStateCandidateCreatorImpl). Interfaces become simpler. The TrackStateCandidateCreatorImpl would get a mutable state, but already has the source link accessor, so has an event state anyways.

goetzgaycken commented 2 days ago

Changes from abedde4 to df55dfb:

Updated changes an the athena side can be found here cern-gitlab:main_support_for_new_extendOrBranchTrajectory_delegate)

goetzgaycken commented 2 days ago

changes df55dfb to 9261956:

goetzgaycken commented 8 hours ago

Changes from 9261956 to cae4ee3:

goetzgaycken commented 8 hours ago

Still an open question:

is the mutable member in the TrackStateCreator for the track state candidate proxies a good idea ?

In case track finding is executed in parallel each thread (or even coroutine) would need its own version of the TrackStateCreator. It may sound worse than it is, since the trajectory cannot be extended concurrently, and the TrackStateCreator contains also the source link accessor which must have some kind of event state.

Though, the latter are not unsolvable, the trajectory is passed as argument to the track finder, and the source link accessor could get the event state from thread local storage.

But, on the other hand is good to have an argument in the trackStateCreator delegate and a buffer in CKF Result which has no obvious purpose except for the one implementation of the trackStateCreator which is currently in Acts ?

Maybe it is better to delay the removal until there is an implementation in Acts which does not need this buffer (?)

andiwand commented 8 hours ago

I think you are right - this kind of hidden thread local state is what we usually try to avoid in ACTS so I agree with you that it might be best for now to carry it through. I was hoping to get rid of it already but forgot about this side effect.

goetzgaycken commented 6 hours ago

changes cae4ee3 to 56d0051:

corresponding update on the athena side is here: [main_support_for_new_extendOrBranchTrajectory_delegate](cern-gitlab:](https://gitlab.cern.ch/goetz/athena/-/commits/main_support_for_new_extendOrBranchTrajectory_delegate)

goetzgaycken commented 3 hours ago

I don't have any further ideas. Undrafting.

goetzgaycken commented 1 hour ago

A bit misleading summary by coder abitai,