acts icon indicating copy to clipboard operation
acts copied to clipboard

Common shared measurement computation?

Open gagnonlg opened this issue 1 year ago • 6 comments

in TrackFindingAlgorithm in the examples (here) the computeSharedHits method is prefaced with the following comment:

// TODO this is somewhat duplicated in AmbiguityResolutionAlgorithm.cpp
// TODO we should make a common implementation in the core at some point
template <typename source_link_accessor_container_t>
void TrackFindingAlgorithm::computeSharedHits(

I think it can make sense to have this decoupled from the ambiguity solving algorithms -- I'm happy to work on this but I guess we should discuss.

@andiwand any thoughts (git-blame points to you for this comment)?

Another point to discuss -- we might want to add another entry to the TrackStateFlag enum, something like MergedHitFlag, which is a shared hit but that's identified as a "true" (or allowed) multi-particle state.

gagnonlg avatar Sep 17 '24 13:09 gagnonlg

I think one problem is that apparently do not set this flag in the ambi solvers. We should definitely fix this.

For the track finding I am not sure how useful it is as there might be duplicated tracks just by seed duplication.

Anyways I think this functionality should be moved to Core, most likely into Core/include/Acts/Utilities/TrackHelpers.hpp. We can then call this function from the TrackFindingAlgorithm if it is really necessary and also after ambi solver if this information is not ready anyways (which I thought it is).

andiwand avatar Sep 17 '24 13:09 andiwand

As proposed in the dev meeting: Let's gather more information until Friday and take a decision then.

AJPfleger avatar Sep 24 '24 15:09 AJPfleger

Discussed briefly with @paulgessinger , we need to do this at the end of the track finding pass otherwise the track collection is immutable and we can't properly tag the state. So, plan of action is to move this function to core, keep the call where it is in the examples CKF pass, & remove it from the ambiguity solvers. PR expected soon(TM)

@AJPfleger you can remove the needs decision label?

gagnonlg avatar Sep 25 '24 15:09 gagnonlg

Having it in core definitely makes sense. At the end of the track finding I am not sure because I don't know if we really need this flag before the ambi is called anyways. But in ACTS world that could be a flag in the track finding as it is now I think.

In case of the ambi solvers I would still think that we could have a shortcut since they will have some similar data lying around. But the user can also just call your helper function at the end of the ambi for now.

andiwand avatar Sep 25 '24 15:09 andiwand

I can benchmark both scenarios -- end of CKF or start of ambi, if that's useful

gagnonlg avatar Sep 26 '24 08:09 gagnonlg

This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.

github-actions[bot] avatar Oct 26 '24 09:10 github-actions[bot]