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

Ambiguity Resolution: remove duplicate measurements for each track #590

Conversation

SylvainJoube
Copy link
Contributor

@SylvainJoube SylvainJoube commented May 14, 2024

This PR ensures that the greedy solver deals with tracks that have unique measurement IDs, removing duplicates, if any.

I was able to replicate the issue with the geant4_ttbar_mu20 data provided by Attila on PR Data V7, main branch (2024.05.03.) #561 and the command line written at the beginning of issue Ambiguity Resolution Errors on ODD ttbar Simulation (2024.05.04.) #564.

It turns out that I did not anticipate that a track could be made of the same measurement_id several times. This happened twice on the generated data from geant4_ttbar_mu20: for tracks 1176 and 720, that’s why the greedy solver was showing an error. The same “error” should exist on ACTS too, since the Greedy Solver code remains mostly unchanged. I have just taken a look at file Core/include/Acts/AmbiguityResolution/GreedyAmbiguityResolution.ipp from ACTS.

For example, in issue Ambiguity Resolution Errors on ODD ttbar Simulation (2024.05.04.) #564, the track of index 720 has measurements: 2878 4167 5155 15837 15837, with 15837 twice.

I could have changed the std::vector<std::size_t> measurements into a std::set<std::size_t> but I'm not sure of the performance implications of such a change, and I'm a little short on time, so this is a quick fix!

@SylvainJoube SylvainJoube force-pushed the fix-greedy-solver-track-measurement-duplication branch from 19edd08 to 0d82fd1 Compare May 14, 2024 17:50
@SylvainJoube SylvainJoube changed the title Remove duplicate measurements for each track Ambiguity Resolution: remove duplicate measurements for each track May 15, 2024
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

It's excellent that you understood where the issues / error messages come from.

But I'm not convinced that this should be "fixed" in the ambiguity resolution algorithm. 🤔 Making the algorithm robust against such input is absolutely good. But the track finding should really not be attaching the same measurement multiple times to the same track.

@beomki-yeo, how would this happen? Looping tracks? 😕

For now, this sort of a check seems good. (Just making the thing into a warning.) But eventually, once the track finding is not expected to produce such tracks, I would demote these checks into assertions. I.e. something like:

assert([&]() {
    // code that decides whether there are measurement duplicates on a track
    }() == false);

(I've been thinking recently about adding a few more of such assertions into our algorithms...)

@SylvainJoube SylvainJoube force-pushed the fix-greedy-solver-track-measurement-duplication branch from c978123 to 4d3f330 Compare May 16, 2024 13:24
@SylvainJoube
Copy link
Contributor Author

Thank you for your feedback Attila!

I’ve modified the code to clearly display a single warning message per track with duplicate measurements. It can handle multiple duplicate measurements without being too verbose. The output now displays:

WARNING: @greedy_ambiguity_resolution_algorithm: Track 1171 has duplicated measurement(s): 15195 (2 times). Measurement list: 3291 3475 4518 7396 8617 9632 9625 10484 15195 15195 15269 15268

WARNING: @greedy_ambiguity_resolution_algorithm: Track 720 has duplicated measurement(s): 15837 (2 times). Measurement list: 2878 4167 5155 15837 15837

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Okay, let's go for this. 👍

@SylvainJoube SylvainJoube force-pushed the fix-greedy-solver-track-measurement-duplication branch from 4d3f330 to 85fb9a8 Compare May 31, 2024 12:11
@SylvainJoube
Copy link
Contributor Author

@krasznaa @beomki-yeo ping 🦊

@krasznaa
Copy link
Member

Ahh, shoot... I thought with the approval in place you would be able to merge it yourself. Completely forgot about this... 😦

@krasznaa krasznaa enabled auto-merge May 31, 2024 12:21
@SylvainJoube
Copy link
Contributor Author

Thank you Attila! No worries, I was just afraid that, as time went by, I would have complicated merge conflicts :’)

@krasznaa
Copy link
Member

Unfortunately we have a lot of traffic on the repository today, so my auto-merge will most likely not work anymore. But let's see... Since the CI takes a long time to run right now, we might as well wait for it to finish, and only rebase the PR if really needed.

@SylvainJoube
Copy link
Contributor Author

Okay, we'll wait then! The CI Bridge says Pipeline refused though, is it an issue or should we wait?

If the same measurement is found multiple times in a
single track: remove duplicates.
If at least one measurement is found multiple times in this track:
print warning message and display the track's measurement list
@krasznaa krasznaa force-pushed the fix-greedy-solver-track-measurement-duplication branch from 85fb9a8 to 5ffcc69 Compare May 31, 2024 15:15
@beomki-yeo
Copy link
Contributor

I dont know how it would happen. But as @krasznaa mentioned, this is not something to be fixed from ambiguity solver. At least, this kind of things should be documented somewhere cleary (e.g. issues)

@SylvainJoube Does this issue cause a serious problem in the final result? Otherwise, I would suggest documenting this in the issue category instead of fixing it in the ambiguity solver. Then someone will be able to try to handle the issue by fiximg the CKF or whatever.

Copy link
Contributor

@beomki-yeo beomki-yeo left a comment

Choose a reason for hiding this comment

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

The multiple module id should be understood better rather than fixing it in the ambiguity solver. For example, I would try to increase the configurarion value of minimal distance to reach the next surface in the finding_config.

@krasznaa krasznaa merged commit 15baf6d into acts-project:main May 31, 2024
20 checks passed
@beomki-yeo
Copy link
Contributor

beomki-yeo commented Jun 2, 2024

I don't get why @krasznaa merged this PR after I requested the change. Duplicate ID is not even a bug in case of a looping track.

@SylvainJoube I want to hear your opinion on this. Does this PR filter tracks which have the "sequential" duicate ID or just filter tracks with duplicate IDs? We should not kill the tracks that have duplicate IDs because of a loop. (Even though the loop is not supposed to happen due to the limitation of detray)

@krasznaa
Copy link
Member

krasznaa commented Jun 2, 2024

I was curious when the question would come.

This was because of auto-merge. You made your comment Beomki, but didn't de-activate the auto-merge that was active on the PR. So when the CI finally finished, and nobody else merged another PR to prevent this from getting merged (as happened once earlier that day), it finally went in.

So now we know that when there is one approval and one change request on a PR, GitHub considers the PR approved. 🤔

It's a bit unfortunate indeed, but I personally didn't consider this PR too hurtful. If you check the discussion on it, I agree with you that the ambiguity resolution is not the place to sort this issue out. At the same time, this looked like an okay intermediate relief for #564.

So now, it's time to start looking for a real fix. 😉

@beomki-yeo
Copy link
Contributor

I didn't know that the auto merge was activated. The PR itself is not very hurtful but I think we don't have to rush in merging PRs.

@krasznaa
Copy link
Member

krasznaa commented Jun 2, 2024

This was an unfortunate series of events. But "rushing" is a strange way to describe it. I approved this PR weeks ago. It was not something brand new.

@beomki-yeo
Copy link
Contributor

Fine. Didn't even realize that i was pinged that long time ago - I was too busy with other affairs before the vacation.

Still, this duplicate ID problem needs to posted in the issue rather being left behind and forgotten eventually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants