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

Common shared measurement computation? #3618

Open
gagnonlg opened this issue Sep 17, 2024 · 6 comments
Open

Common shared measurement computation? #3618

gagnonlg opened this issue Sep 17, 2024 · 6 comments
Labels

Comments

@gagnonlg
Copy link
Contributor

gagnonlg commented Sep 17, 2024

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
Copy link
Contributor

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 AJPfleger added the Needs Decision Needs decision or further information label Sep 24, 2024
@AJPfleger
Copy link
Contributor

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

@gagnonlg
Copy link
Contributor Author

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?

@AJPfleger AJPfleger removed the Needs Decision Needs decision or further information label Sep 25, 2024
@andiwand
Copy link
Contributor

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
Copy link
Contributor Author

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

Copy link

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 github-actions bot added the Stale label Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants