refactor!: move source link creation, track state creation, measurement selection into a single unit outside of the CKF #3825
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Core
This allows to merge retrieval of source link ranges, measurement selection and track state creation into one unit. 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.
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
c1ce59a
to
d3a565e
Compare
|
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
d3a565e
to
b44b8fe
Compare
|
Why should there be brackets arround slAccessorDelegate TrackFindingAlgorithm.cpp:365 (macos build complains): |
|
| TrackStateCandidateCreatorType trackStateCreator{ | ||
| slAccessorDelegate, | ||
| Acts::makeDelegate<&MeasurementCalibratorAdapter::calibrate>(calibrator), | ||
| Acts::makeDelegate<&MeasurementSelector::select>(measSel)}; |
There was a problem hiding this comment.
I'm not sure if this pattern of deducing and then assigning the delegate types is a good idea.
The idea behind this connect API that I originally introduced is that you separate the signature definition from when you register a function.
Is this really so much worse than:
| TrackStateCandidateCreatorType trackStateCreator{ | |
| slAccessorDelegate, | |
| Acts::makeDelegate<&MeasurementCalibratorAdapter::calibrate>(calibrator), | |
| Acts::makeDelegate<&MeasurementSelector::select>(measSel)}; | |
| TrackStateCandidateCreatorType trackStateCreator; | |
| trackStateCreator.sourceLinkAccessor.connect<&IndexSourceLinkAccessor::range>(&slAccessor); | |
| trackStateCreator.calibrator.connect<&MeasurementCalibratorAdapter::calibrate>(calibrator); | |
| trackStateCreator.measurementSelector.connect<&MeasurementSelector::select>(measSel)}; |
? I my view, this is more legible and explicit.
There was a problem hiding this comment.
You create a not properly initialized object, and the compiler would not tell You if You missed something.
There was a problem hiding this comment.
Since these are supposed to be runtime configurable, at least optionally, I don't see this as an issue. What we do everywhere else is either introduce default functions (which you do) than can error on invocation, or when pushing the object to a consumer it can check if all of the delegates are connected.
There was a problem hiding this comment.
I don't have a strong opinion. But if it is preferred I can remove these helper methods and the starting point would be an uninitialized object and it is up to the user to remember what needs to be connected.
Though I don't see it as an improvement to move something that could be caught during compile time to the runtime. But of course You are right that delegates could be fully configurable thus it often is a runtime problem anyways.
It also seems to simplify client code. There are a few cases like
Delgate delegate;
delegate.connect<&func>(obj):
Options option(delegate);
which just becomes
Options option(makeDelegate<&func>(obj));
Of course the first version could just be
Options option;
option.delegate.connect<&func>(obj):
But then the delegate has to get a default value also in cases in which there is no meaningful default value, and in which the default value is only short lived and in principle unused. The default value also could generate dead code.
There was a problem hiding this comment.
I would prefer this, yes. There's also the added benefit that explicitly writing the delegate being connected avoids any ordering confusion (although this is a compile error due to mismatching types).
There was a problem hiding this comment.
I don't understand the argument about ordering confusion, either way You cannot make a "connected" delegate to a non existing object or function by accident, or at least I do not see how one snippet is more likely for errors than the other, except for uninitialized member delegates which are caught at runtime in one case and at compile time in the other.
But I see You prefer for some reason unused default values and or more boiler plate code.
| secondOptions.targetSurface = m_cfg.reverseSearch ? nullptr : pSurface.get(); | ||
| secondOptions.skipPrePropagationUpdate = true; | ||
| secondOptions.extendOrBranchTrajectory = |
There was a problem hiding this comment.
As we discussed, we might want to roll this into the extentions struct.
There was a problem hiding this comment.
At the moment the extensions struct would only contain a delegate for the updater and branchStopper. Both are optional, aren't they? And in theory could be identical for the entire job. In practice e.g. the branchStopper might not be stateless (e.g. information whether it is the in-side-out or out-side-in pass). The extendOrBranchTrajectory has to know at least something about the current event, so cannot be stateless (not considering some thread local global variables ... )
It is not fully clear to me what the role of the extensions should be or become. At the moment the extensions seem to be a bit of an arbitrary subset of the options. But if it makes sense to collect all delegates in one struct and everything which is not a delegate stays in the parent struct then certainly the extendOrBranchTrajectory should be moved to the extensions.
There also was the question about the naming. Is this a good name ? Or is it misleading ? It does add either one or more new track states to the current branch (which still need some post processing), so it should do what the name suggests.
There was a problem hiding this comment.
You're right that it really only pragmatically collects all delegates to the CKF as a grouped struct. The name comes from the delegates being "extension points" of the CKF, at least that's what I originally came up with.
There was a problem hiding this comment.
Concerning the name, I did not question the name "extensions", but the was wondering about "extendOrBranchTrajectory"
|
|
||
| /// the stateCandidator to be used | ||
| /// @note will be set to a default trackStateCandidateCreator or the one | ||
| // provided via the extension | ||
| TrackStateCandidateCreator trackStateCandidateCreator; | ||
| TrajectoryExtender extendOrBranchTrajectory; |
There was a problem hiding this comment.
This is a bit inconsistent already in this actor but maybe we should rename all of these to m_*?
There was a problem hiding this comment.
At the moment none of the members in options or extensions has the "m_" prefix. In that sense adding an "m_" prefix would make it inconsistent.
| using SourceLinkRange = decltype(sourceLinkAccessor(surface)); | ||
| std::optional<SourceLinkRange> slRange = sourceLinkAccessor(surface); |
There was a problem hiding this comment.
I don't understand how these two function calls can have different return types? This looks to me like you're unconditionally boxing SourceLinkRange into an extra std::optional only to unbox it right after. I'm probably missing something.
There was a problem hiding this comment.
That is copy&pasted from the original code in the CKF the optional lost its purpose. So should be removed. the sourceLinkAccessor returns a range not an optional.
| // data which is to be filled by the next stage e.g. the | ||
| // CombinatorialKalmanFilter. | ||
| template <typename source_link_iterator_t, typename track_container_t> | ||
| struct TrackStateCandidateCreatorImpl |
There was a problem hiding this comment.
This is the adapter class that you mentioned, right @goetzgaycken?
There was a problem hiding this comment.
This is the class which got the original code from the CKF. The code is just moved over.
There was a problem hiding this comment.
Maybe the class should be renamed to fit the delegate name. In the original implementation in which the hierarchy was inverted this was the base class, and then this name made more sense.
| // CAREFUL! This trackstate has a previous index that is not in this | ||
| // MultiTrajectory Visiting brackwards from this track state will | ||
| // fail! | ||
| auto ts = bufferTrajectory.makeTrackState(mask, prevTip); |
There was a problem hiding this comment.
The only remaining reason for this bufferTrajectory is to allow the use of the previous measurement selector after calibration, which requires track states, right?
There was a problem hiding this comment.
yes exactly. The buffer trajectory could also become a member. Then the interfaces including the new interface of extendOrBranchTrajectory could be simplified. But the object would gain a state.
| }; | ||
| }; | ||
|
|
||
| /// @brief Base class for the ComposableTrackStateCreator which simply delegates its tasks |
There was a problem hiding this comment.
This is now derived, not base anymore, isn't it?
There was a problem hiding this comment.
exactly. Thanks!
|
|
||
| SourceLinkAccessor sourceLinkAccessor; | ||
|
|
||
| const track_state_creator_derived_t& derived() const { |
There was a problem hiding this comment.
This could be private maybe.
There was a problem hiding this comment.
Not sure whether that is something that needs to be hidden from the public, but it is presumably also not really necessary to keep it public:
There was a problem hiding this comment.
IMO it is always a good idea to keep the public surface as small as possible. I don't think this is useful to the user?
| options.sourceLinkAccessor.connect<&Fixture::TestSourceLinkAccessor::range>( | ||
| &slAccessor); | ||
|
|
||
| auto trackStateCreator = makeTrackStateCreator(slAccessor, f.measSel); |
There was a problem hiding this comment.
Yes, that is the delegate for extendOrBranchTrajectory. Maybe I should rename it ?
andiwand
changed the title
|
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 A single composable struct that by itself has the three (?) original delegates including the 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. |
|
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. |
|
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. |
| namespace detail { | ||
| // helper method to deduce the return type and arguments of the member function | ||
| template <auto memberfunc_ptr, typename return_t, typename instance_t, | ||
| typename... Args> | ||
| inline auto makeDelegate( | ||
| [[maybe_unused]] return_t (instance_t::*memberfunc)(Args...) const, | ||
| const instance_t& instance) { | ||
| Delegate<return_t(Args...)> delegate; | ||
| delegate.template connect<memberfunc_ptr>(&instance); | ||
| return delegate; | ||
| } | ||
| // helper method to deduce the return type and arguments of the function | ||
| template <auto func_ptr, typename return_t, typename... Args> | ||
| inline auto makeDelegate([[maybe_unused]] return_t (*func)(Args...)) { | ||
| Delegate<return_t(Args...)> delegate; | ||
| delegate.template connect<func_ptr>(); | ||
| return delegate; | ||
| } | ||
| } // namespace detail | ||
|
|
||
| /// @brief convenience method to create and connect a delegate. | ||
| /// @tparam memberfunc_ptr a pointer to a member function of the class instance_t | ||
| /// @param instance an instance which outlives this delegate. | ||
| template <auto memberfunc_ptr, typename instance_t> | ||
| inline auto makeDelegate(const instance_t& instance) { | ||
| return detail::makeDelegate<memberfunc_ptr>(memberfunc_ptr, instance); | ||
| } | ||
|
|
||
| /// @brief convenience method to create and connect a delegate. | ||
| /// @tparam func_ptr a pointer to a function | ||
| template <auto func_ptr> | ||
| inline auto makeDelegate() { | ||
| return detail::makeDelegate<func_ptr>(func_ptr); | ||
| } |
There was a problem hiding this comment.
This seems a bit generic for a header called ComposableTrackStateCreator.hpp. Either this should be a utility placed in the delegate header or dropped in favor of doing this by hand.
I think the deduction should also work without a function wrapper?
There was a problem hiding this comment.
- I agree that these helper functions would be better located somewhere else.
- I haven't found a solution without the unused argument:
// 1:
template <return_t(*T_Func)(Args...), typename return_t, typename... Args> auto makeDelegate(); // does not compile
// 2:
template <auto func_ptr, typename return_t, typename... Args>
constexpr auto makeDelegate2([[maybe_unused]] return_t (*func)(Args...) =func_ptr) ;
... {
auto a = makeDelegate2( &func ) ; // deduction failed
}
| /// stateCandidateCreator | ||
| /// @note if the number of states exceeds this number dynamic memory allocation will occur. | ||
| /// the number is chosen to yield a container size of 64 bytes. | ||
| static constexpr std::size_t s_maxBranchesPerSurface = 10; |
There was a problem hiding this comment.
This and the CkfTypes should not really be in this header. In principle this header is optional but it would definitions used by the CKF. But where to put it ?
If extendOrBranchTrajectory becomes an extension than it could go to the extensions header.
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
b44b8fe
to
b00f570
Compare
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.
goetzgaycken
force-pushed
the
refactor_move_sl_creation_measurement_selection_to_single_unit
branch
from
b00f570
to
2cc2ecc
Compare
|
Changes from b44b8fe to b00f570 :
changes from b00f570 to 2cc2ecc:
I did not remove the adapter to support the components of the version 2 interface, which is currently used in ATLAS. I agree that there is no good reason to keep that around for long, but I am not sure whether it is a good strategy to make the interface change on the Acts side, and directly migrate on the ATLAS side to the new interface. The changes are not super complicated but also not trivial. Minimal changes to support the new interface on the athena side gitlab:EliminateSourceLinkAccessor_main~1, direct support of the new interface gitlab:EliminateSourceLinkAccessor_main, |
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!