DR: Excluding Duplicates for CM #267

dally96 · 2024-03-29T15:14:49Z

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 · 2024-04-02T21:52:08Z

@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 · 2024-04-03T09:25:26Z

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 <#267 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARUNMFZHRZLOIYQPXP77WE3Y3MSB3AVCNFSM6AAAAABFORV2GWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGE3DONZTGI>. You are receiving this because you were mentioned.

tomalin · 2024-04-06T18:24:27Z

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

tomalin · 2024-04-06T18:25:12Z

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 · 2024-04-08T09:37:51Z

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 · 2024-08-05T10:09:44Z

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

tomalin · 2024-09-13T08:36:19Z

L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc

-              const unsigned int curSeed = aTrack->seedIndex();
-              static const std::vector<int> ranks{1, 5, 2, 7, 4, 3, 8, 6};
+              unsigned int curSeed = aTrack->seedIndex();
+              std::vector<int> ranks{1, 5, 2, 7, 4, 3, 8, 6};


Why has the "static const" qualifier been removed here?

Put "static const" back.

tomalin · 2024-09-13T08:44:50Z

L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc

@@ -181,38 +187,70 @@ void PurgeDuplicate::execute(std::vector<Track>& outputtracks, unsigned int iSec
        if (inputtracklets_.empty())
          continue;
        const unsigned int numStublists = inputstublists_.size();
+        std::vector<int> seedRankIdx(numStublists);


Add comment that you're ordering the tracks by seed rank.

Comment has been included.

tomalin · 2024-09-13T08:57:57Z

L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc

@@ -86,6 +87,11 @@ void PurgeDuplicate::execute(std::vector<Track>& outputtracks, unsigned int iSec
  inputstublists_.clear();
  mergedstubidslists_.clear();

+  sortedinputtracklets_.clear();


I've never understood why all these vectors are declared as class data members. If they were instead declared inside the loop over bins, e.g. near here https://github.com/cms-L1TK/cmssw/blob/dally_CMFix/L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc#L140 , they would automatically be deleted and recreated between loops, so no clear would be needed either here nor near L410. Am I missing something?

No, I just wanted to mirror the actual class data members, but I can change them to only be declared in the loop over bins

tomalin · 2024-09-13T11:56:41Z

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.

dally96 · 2024-12-18T15:15:13Z

@tomalin It seems the number of CM affects hybrid displaced more than hybrid. I haven't optimized DR for displaced tracking, so would it be fine to merge this anyway?

tomalin · 2024-12-19T13:59:11Z

@tomalin It seems the number of CM affects hybrid displaced more than hybrid. I haven't optimized DR for displaced tracking, so would it be fine to merge this anyway?

The Displaced Tracking fails CI, as the number of reconstructed tracks has gone up by about one quarter. Do you understand why this PR should have this effect? (Recall the git CI runs on ttbar + 0PU). Have you checked what happens in ttbar+200PU?

Hi @dally96 there are still several unanswered review comments. I can't merge until they are answered.

dally96 · 2024-12-19T20:10:01Z

@tomalin It seems the number of CM affects hybrid displaced more than hybrid. I haven't optimized DR for displaced tracking, so would it be fine to merge this anyway?

The Displaced Tracking fails CI, as the number of reconstructed tracks has gone up by about one quarter. Do you understand why this PR should have this effect? (Recall the git CI runs on ttbar + 0PU). Have you checked what happens in ttbar+200PU?

Hi @dally96 there are still several unanswered review comments. I can't merge until they are answered.

I don't know why this is the case for the displaced case, since we didn't study the effects of this PR on extended tracking. However, a quick fix would be to increase the number of CMs to 64. When I run on a TTbar + PU200 sample fro 32 CM, this my output

tomalin · 2024-12-19T22:30:14Z

L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc

+        bool barrel = (i > 0 && i <= N_LAYER);
+        bool endcapA = (i > N_LAYER);
+        bool endcapB = (i < 0);
+        int lay = barrel * (i - 1) + endcapA * (i - 5) - endcapB * i;  // encode in range 0-15


Magic number 5 not allowed

tomalin · 2024-12-19T22:30:36Z

L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc

+    bool barrel = (i > 0 && i <= N_LAYER);
+    bool endcapA = (i > N_LAYER);
+    bool endcapB = (i < 0);
+    int lay = barrel * (i - 1) + endcapA * (i - 5) - endcapB * i;  // encode in range 0-15


Magic number 5 not allowed

tomalin · 2024-12-19T22:32:59Z

@tomalin It seems the number of CM affects hybrid displaced more than hybrid. I haven't optimized DR for displaced tracking, so would it be fine to merge this anyway?

The Displaced Tracking fails CI, as the number of reconstructed tracks has gone up by about one quarter. Do you understand why this PR should have this effect? (Recall the git CI runs on ttbar + 0PU). Have you checked what happens in ttbar+200PU?
Hi @dally96 there are still several unanswered review comments. I can't merge until they are answered.

I don't know why this is the case for the displaced case, since we didn't study the effects of this PR on extended tracking. However, a quick fix would be to increase the number of CMs to 64. When I run on a TTbar + PU200 sample fro 32 CM, this my output

How do your ttbar+PU200 results for displaced tracking compare for CM=32, 64 and before you made your PR? I suspect the L1 trigger group would be unhappy if the number of displaced tracks go up by a quarter.

…acks excluding duplicates

…te tracks are skipped in the comparison

…=' which accounts for duplicate tracks with the same seed rank

…illing to merging in DR. Changed some magic numbers in PurgeDuplicate.cc when encoding the barrel and disk layers. Created a doCompareAll function to mirror doCompareBest in PurgeDuplicate. Made minor fixes and included comments.

dally96 · 2025-01-07T21:35:21Z

@tomalin It seems the number of CM affects hybrid displaced more than hybrid. I haven't optimized DR for displaced tracking, so would it be fine to merge this anyway?

The Displaced Tracking fails CI, as the number of reconstructed tracks has gone up by about one quarter. Do you understand why this PR should have this effect? (Recall the git CI runs on ttbar + 0PU). Have you checked what happens in ttbar+200PU?
Hi @dally96 there are still several unanswered review comments. I can't merge until they are answered.

I don't know why this is the case for the displaced case, since we didn't study the effects of this PR on extended tracking. However, a quick fix would be to increase the number of CMs to 64. When I run on a TTbar + PU200 sample fro 32 CM, this my output

How do your ttbar+PU200 results for displaced tracking compare for CM=32, 64 and before you made your PR? I suspect the L1 trigger group would be unhappy if the number of displaced tracks go up by a quarter.

32 CM:

64 CM:

Before PR with 9999 CM:

Before PR with 64 CM:

dally96 · 2025-01-15T22:17:43Z

@tomalin All checks have passed, but I did add a separate comparison of seed rank if hybrid displaced is being run. Hopefully that isn't a problem. I'm assuming once Alaa has looked at the seed ranking of the dispalced seeds, that we can go back to what I originally had in the PR>

tomalin · 2025-01-20T14:17:07Z

A general comment -- please try to keep CMSSW code as short and simple and CPU efficient as possible, since it makes it easier to understand and maintain. PurgeDuplicates.cc is now 915 lines long, which seems a lot for such a simple algorithm.

…ep for extended tracking. Number of CM for extended tracking increased to 9999 while we work out a fix

…ded seeds

dally96 · 2025-01-30T20:52:46Z

@tomalin I put back in the < comparison instead of <= comparison for only extended seeds. Before I had them for every seed if we ran extended tracking. Here are the results for 9999 CM TTbar+PU200

Instead of a 4% increase in duplicates, it's a 3.4% increase. If this still is a problem, I can just use < for all seeds in extended tracking while we wait for developments to be made on that front

tomalin · 2025-02-28T16:14:27Z

L1Trigger/TrackFindingTracklet/interface/Settings.h

@@ -448,15 +450,15 @@ namespace trklet {
    //have the factor if 2
    double krprojshiftdisk() const { return 2 * kr(); }

-    double benddecode(unsigned int ibend, unsigned int layerdisk, bool isPSmodule) const {


Why have you changed "unsigned" to "int" in the arguments of function Settings::benddecode()?

tomalin · 2025-02-28T16:15:04Z

L1Trigger/TrackFindingTracklet/interface/Settings.h

      if (layerdisk >= N_LAYER && (!isPSmodule))
        layerdisk += N_DISK;
      double bend = benddecode_[layerdisk][ibend];
      assert(bend < 99.0);
      return bend;
    }

-    double bendcut(unsigned int ibend, unsigned int layerdisk, bool isPSmodule) const {


Why have you changed "unsigned" to "int" in the arguments of Settings::bendcut()?

tomalin · 2025-02-28T16:37:11Z

The git CI runs on ttbar + 0PU.
I therefore tried running myself the prompt tracking "HYBRID" over 100 events of ttbar+200PU, with the following results:

BEFORE THIS PR:
Efficiency = 94.80%
Tracks/event (pt > 2.0) = 165.96
Percentage duplicate tracks = 1.22%

AFTER THIS PR:
Efficiency = 95.07%
Tracks/event (pt > 2.0) = 238.56
Percentage duplicate tracks = 27.00%

There is therefore a significant bug somewhere.

As a sanity check, I also tried running HYBRID on the ttbar+PU0 sample, and also the duplicate rate rises from 0.7% to 3.3%, this causes only a slight increase in the tracks/event, which is why git CI passes.

tomalin requested a review from trholmes April 2, 2024 23:20

dally96 added the CANT BE DONE YET label Apr 5, 2024

dally96 marked this pull request as draft April 5, 2024 10:05

dally96 removed the CANT BE DONE YET label Apr 5, 2024

tomalin reviewed Sep 13, 2024

View reviewed changes

dally96 changed the base branch from L1TK-dev-14_0_0_pre2 to L1TK-dev-14_2_0_pre4 December 9, 2024 14:58

dally96 force-pushed the dally_CMFix branch from d3fd270 to 4c5a793 Compare December 9, 2024 14:59

dally96 marked this pull request as ready for review December 17, 2024 20:11

dally96 force-pushed the dally_CMFix branch from 382106a to b10714f Compare December 19, 2024 21:51

tomalin reviewed Dec 19, 2024

View reviewed changes

Daniel Ally added 6 commits January 7, 2025 16:25

Changed PurgeDuplicate such that numTracksComparedPerBin() cuts on tr…

f6ca770

…acks excluding duplicates

Formatting issues

a91b0b7

Newer version of DR where tracks are ordered by seed rank and duplica…

ed68688

…te tracks are skipped in the comparison

Fixed doCompareBest in PurgeDuplicate

80c5d67

Added a fix to comparing duplicate tracks that switches a '<' to a '<…

694a48e

…=' which accounts for duplicate tracks with the same seed rank

Changed number of CM and root file name

b3931c8

Separate CM for extended tracking included

8311a1f

dally96 force-pushed the dally_CMFix branch from b10714f to 8311a1f Compare January 14, 2025 01:10

Daniel Ally added 4 commits January 14, 2025 02:30

code-format

879e527

Back to normal

cab8fca

Added functionality for if settings_.extended() is true

3752943

code-format

23e527b

Daniel Ally added 4 commits January 22, 2025 16:06

Reversed previous decision to include < for comparison and merging st…

6413207

…ep for extended tracking. Number of CM for extended tracking increased to 9999 while we work out a fix

Added duplicatefrac_seed plot to L1TrackNtuplePlot.C

b47dc85

Added seed histogram to L1TrackNtuplePlot

2ac1c43

Different comparison and merging behavior in PurgeDuplicate for exten…

4094c78

…ded seeds

tomalin reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DR: Excluding Duplicates for CM #267

DR: Excluding Duplicates for CM #267

dally96 commented Mar 29, 2024

tomalin commented Apr 2, 2024

dally96 commented Apr 3, 2024 via email

tomalin commented Apr 6, 2024

tomalin commented Apr 6, 2024

dally96 commented Apr 8, 2024 •

edited

Loading

tomalin commented Aug 5, 2024

tomalin Sep 13, 2024

dally96 Dec 19, 2024

tomalin Sep 13, 2024

dally96 Dec 19, 2024

tomalin Sep 13, 2024 •

edited

Loading

dally96 Dec 19, 2024

tomalin commented Sep 13, 2024

dally96 commented Dec 18, 2024

tomalin commented Dec 19, 2024 •

edited

Loading

dally96 commented Dec 19, 2024 •

edited

Loading

tomalin Dec 19, 2024

tomalin Dec 19, 2024

tomalin commented Dec 19, 2024

dally96 commented Jan 7, 2025

dally96 commented Jan 15, 2025

tomalin commented Jan 20, 2025 •

edited

Loading

dally96 commented Jan 30, 2025

tomalin Feb 28, 2025

tomalin Feb 28, 2025

tomalin commented Feb 28, 2025

DR: Excluding Duplicates for CM #267

Are you sure you want to change the base?

DR: Excluding Duplicates for CM #267

Conversation

dally96 commented Mar 29, 2024

tomalin commented Apr 2, 2024

dally96 commented Apr 3, 2024 via email

tomalin commented Apr 6, 2024

tomalin commented Apr 6, 2024

dally96 commented Apr 8, 2024 • edited Loading

tomalin commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalin Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalin commented Sep 13, 2024

dally96 commented Dec 18, 2024

tomalin commented Dec 19, 2024 • edited Loading

dally96 commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalin commented Dec 19, 2024

dally96 commented Jan 7, 2025

dally96 commented Jan 15, 2025

tomalin commented Jan 20, 2025 • edited Loading

dally96 commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalin commented Feb 28, 2025

dally96 commented Apr 8, 2024 •

edited

Loading

tomalin Sep 13, 2024 •

edited

Loading

tomalin commented Dec 19, 2024 •

edited

Loading

dally96 commented Dec 19, 2024 •

edited

Loading

tomalin commented Jan 20, 2025 •

edited

Loading