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
104 stars 166 forks source link

Common shared measurement computation? #3618

Open gagnonlg opened 1 week ago

gagnonlg commented 1 week ago

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.

andiwand commented 1 week ago

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).

AJPfleger commented 4 days ago

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

gagnonlg commented 3 days ago

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?

andiwand commented 3 days ago

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.

gagnonlg commented 3 days ago

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