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!: Fuse Actor and Aborter #3573

Merged
merged 19 commits into from
Sep 24, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Aug 28, 2024

Fuses Actors and Aborters into a single object with seperate act and check calls. The motivation for this is that Aborters cannot hold any state by their own currently but need an Actor to point to. To simplify this I propose to couple these two concepts.

At the same time I reworked the call mechanism for the actor and aborter functions which concepts and a simpler dispatch mechanism.

Class names changed a bit from ActionList to ActorList for example.

@andiwand andiwand added this to the v37.0.0 milestone Aug 28, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Component - Documentation Affects the documentation Track Finding Track Fitting labels Aug 28, 2024
@asalzburger
Copy link
Contributor

Yes, why not - we actually had actor, observer, aborter in the original design I suppose, and I think that was over-designed.

The only comment I have here is that the ordering of the actors gets even more important, but that's not a bad thing - on the contrary, I would say.

@andiwand
Copy link
Contributor Author

The way I combined them right now preserves the ordering of the actor/aborter. I just combined the act and check function into one object. The calls are still happening as before, first act then check, in sequence.

Potentially we could also fuse the two functions since it is really just the return value which is different but then we have to be more careful about the ordering like you said @asalzburger

cc @paulgessinger @benjaminhuth

Copy link

github-actions bot commented Aug 29, 2024

📊: Physics performance monitoring for 24735fb

Full contents

physmon summary

@andiwand andiwand mentioned this pull request Aug 29, 2024
Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

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

Looks very solid; minor changes only.

stephenswat
stephenswat previously approved these changes Aug 30, 2024
@acts-policybot acts-policybot bot dismissed stephenswat’s stale review September 8, 2024 06:25

Invalidated by push of 09941f0

@andiwand andiwand marked this pull request as ready for review September 10, 2024 10:20
Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

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

Some comments on how this might be improved, but I am not 100% sure it's equivalent.

Core/include/Acts/TrackFitting/KalmanFitter.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/ActorConcepts.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/ActorConcepts.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/ActorConcepts.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/ActorConcepts.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/ActorConcepts.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/ActorConcepts.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Propagator/ActorList.hpp Outdated Show resolved Hide resolved
@andiwand
Copy link
Contributor Author

@stephenswat I was wondering if the Actor should just flag what it will provide and only then we couple to the functions instead of deducing it from if the function exists or not. What do you think? Since this is breaking anyways we might just make this more explicit

@andiwand
Copy link
Contributor Author

I left the flags out and applied your comments @stephenswat. I think that makes it way easier to just check for methods and not combinations

stephenswat
stephenswat previously approved these changes Sep 11, 2024
@acts-policybot acts-policybot bot dismissed stephenswat’s stale review September 12, 2024 08:39

Invalidated by push of dadfbfe

Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

stephenswat added a commit to stephenswat/acts that referenced this pull request Sep 16, 2024
This commit removes the mostly unused MPL library. The few cases in
which it was being used could be easily refactored into uses of fold
expressions, which should allow better error messages.

Depends on acts-project#3573.
Copy link

@kodiakhq kodiakhq bot merged commit 17b7b92 into acts-project:main Sep 24, 2024
44 checks passed
stephenswat added a commit to stephenswat/acts that referenced this pull request Sep 24, 2024
This commit removes the mostly unused MPL library. The few cases in
which it was being used could be easily refactored into uses of fold
expressions, which should allow better error messages.

Depends on acts-project#3573.
@andiwand andiwand deleted the refactor-fuse-actor-and-aborter branch September 24, 2024 12:29
stephenswat added a commit to stephenswat/acts that referenced this pull request Sep 24, 2024
This commit removes the mostly unused MPL library. The few cases in
which it was being used could be easily refactored into uses of fold
expressions, which should allow better error messages.

Depends on acts-project#3573.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Component - Documentation Affects the documentation Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Track Finding Track Fitting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants