search

cms-L1TK / cmssw

Fork of CMSSW where improvements to L1 tracking code are developed.
http://cms-sw.github.io/
Apache License 2.0
4 stars 5 forks source link

DR: Excluding Duplicates for CM #267

Open dally96 opened 7 months ago

dally96 commented 7 months ago

Fixed track comparison in PurgeDuplicate.cc so that the comparison modules ignore duplicate tracks. However, this does not overwrite the stub list if a duplicate is found and is merged.

tomalin commented 7 months ago

@dally96 given that in Settings.h , int numTracksComparedPerBin_ remains 9999, this PR should have no effect on tracking performance. But it fails git CI because the tracking performance has got worse. Do you understand why?

dally96 commented 7 months ago

I think it might have to do with the fact that this code doesn’t overwrite the inputstublists when a track is merged. For example, with this code, if track 1 is a duplicate of track 0, track 1’s stubs do not get added to track 0’s stub list, but track 1 will not be compared to the other tracks. So if track 2 is a duplicate of track 1, it won’t be removed. We tried overwriting the stublist before but it led to an error in the getPhiRes function in PurgeDuplicate that we didn’t figure out.

On Apr 2, 2024, at 11:52 PM, Ian Tomalin @.***> wrote:

@dally96 https://github.com/dally96 given that in Settings.h , int numTracksComparedPerBin_ remains 9999, this PR should have no effect on tracking performance. But it fails git CI because the tracking performance has got worse. Do you understand why?

— Reply to this email directly, view it on GitHub https://github.com/cms-L1TK/cmssw/pull/267#issuecomment-2033167732, or unsubscribe https://github.com/notifications/unsubscribe-auth/ARUNMFZHRZLOIYQPXP77WE3Y3MSB3AVCNFSM6AAAAABFORV2GWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGE3DONZTGI. You are receiving this because you were mentioned.

tomalin commented 7 months ago

Does the current code, before this PR, also behave incorrectly, by not overwriting the inputstublists when two tracks are merged?

tomalin commented 7 months ago

In your talk, you stated that if you correct this PR, so it does overwrite the inputstublists, then it crashes. My guess is that Tracklet:::proj() which you are calling from PurgeDuplicate is being called with a layer that fails the assert statement in Tracklet::validProj().

dally96 commented 7 months ago

Does the current code, before this PR, also behave incorrectly, by not overwriting the inputstublists when two tracks are merged?

There are two loops one to check if two tracks are duplicate and the other to merge the tracks if they are duplicates. In the loop to merge tracks, the stubs and stub ids do get merged, but after all the comparisons have been done. This error arises when I try to use the merged stubs for the comparison (although they should really be in one loop for this PR, I will fix that, but the same error persists).

tomalin commented 3 months ago

There's been no progress on this PR in 4 months. Shall we close it?

tomalin commented 2 months ago

This fails git CI only because the performance of HYBRID_DISPLACED is worse https://gitlab.cern.ch/cms-l1tk/cmssw_CI/-/pipelines/8056886 . The reason it is worse is presumably your discovery that that the DR binning equations need changing to work for displaced tracks. Since Thomas says that with his new "comparison modules ignore duplicate tracks" idea, the binning is no longer necessary, please try running with only 1 bin, and check compare the performance with what you get currently.