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

DR: fix displaced track bug & disable binning #266

Merged
merged 6 commits into from
Mar 28, 2024

Conversation

tomalin
Copy link
Collaborator

@tomalin tomalin commented Mar 25, 2024

PR description:

Fixed bug in the calculation of variable seedRank in PurgeDuplicate.cc , which was incorrect for displaced tracklets, and damaged the tracking performance.

Temporarily disable binning in DR, since it may be messing up displaced tracking.

These issues are also causing problems with attempts to switch to using "combined" tracking modules by default, as reported in #265 .

@tomalin
Copy link
Collaborator Author

tomalin commented Mar 25, 2024

Running the displaced tracking on ttbar+200PU, before and after this PR, I see the following:

BEFORE (=With original (Pt, phi) binning of DR):

number of tracks/event (pt > 2.00) = 332.65
efficiency for |eta| < 1.0 = 96.96 +- 0.15
efficiency for 1.0 < |eta| < 1.75 = 94.96 +- 0.27
efficiency for 1.75 < |eta| < 2.40 = 96.60 +- 0.30
combined efficiency for |eta| < 2.40 = 96.33 +- 0.12

AFTER (=Without (Pt,phi) binning of DR (and DR truncation per bin switched off))

number of tracks/event (pt > 2.00) = 270.91
efficiency for |eta| < 1.0 = 96.75 +- 0.16
efficiency for 1.0 < |eta| < 1.75 = 91.47 +- 0.35
efficiency for 1.75 < |eta| < 2.40 = 96.40 +- 0.31
combined efficiency for |eta| < 2.40 = 95.18 +- 0.14

So the fix does reduce the number of tracks, as hoped. But it also gives far worse tracking efficiency for 1 < eta < 1.75. So something is going horribly wrong. If I disable the DR altogether (by setting RemovalType=”” in l1tTTTracksFromTrackletEmulation_cfi.py, then the “combined efficiency” rises to 97.46%, (with the number of tracks rising to 988), so the DR is causing the efficiency loss.

I also looked at the displaced tracking performance in CMSSW 13_3_1, which is before any the binned DR code was added. The performance there was much better, with “combined efficiency” of 96.7%, an efficiency in the problematic 1 < eta < 1.75 region of 95.9%, and only 237 tracks/event. So it looks to me as if some additional change to the DR, made since 13_3_1, and which is not disabled by my config fix, is messing up the displaced tracking.

In 13_3_2, where DR binning in Pt is implemented in the C++, if I set the number of Pt bins to 1 (and also set maxTracksComparedPerBin=9999 to avoid truncation), this gives bad efficiency (91.3% for 1 < eta < 1.75) and many tracks/event = 303, so the problem is here. If I repeat, but replacing the 13_3_2 version of PurgeDuplicates.cc by the version from 13_3_1, then good performance is restored.
P.S. If I set inventStubs=false, which was a new option introduced in 13_3_2, then, the bad performance is slightly improved (efficiency = 92.1% for 1 < eta < 1.75 and tracks/event = 264), so this option seems to be partially to blame.

NEWS -- BUG FOUND
@dally96 @sarafiorendi @skinnari

The calculation of the variable seedRank in PurgeDuplicate.cc was incorrect for displaced tracklets. If I fix it, then the last set of results above improve: efficiency increases from 92.1% to 96.0% whilst tracks/event remains unchanged. I'll update the PR.

@sarafiorendi
Copy link

About the performance of the "inventStubs" option on displaced tracking, its effect was shown in these slides page 9-11: basically a negligible degradation in efficiency was seen in the 1 < eta < 1.75 region (from 64.85 ± 0.89 to 64.82 ± 0.89) on a displaced Mu PU 200 sample (I did not check the displaced algo on the TTBar sample).

The comment here does not mean that the invent stub does not work for displaced tracking, but was to stress that we are currently only inventing two out of the 3 stubs fo the displaced seeds, given the performance plots shown in the slides.

@sarafiorendi
Copy link

I see in slide 8 of this presentation that it is mentioned that if DR is turned off and prompt tracks are also allowed, lower efficiency is seen due to an artefact in the ntuple maker code. I don't know if this has been fixed, nor if it would especially affect the 1 < eta < 1.75 region. Maybe you're seeing the same effect? sorry for the spam if that issue was already changed in the ntuple maker.

@tomalin
Copy link
Collaborator Author

tomalin commented Mar 27, 2024

I see in slide 8 of this presentation that it is mentioned that if DR is turned off and prompt tracks are also allowed, lower efficiency is seen due to an artefact in the ntuple maker code. I don't know if this has been fixed, nor if it would especially affect the 1 < eta < 1.75 region. Maybe you're seeing the same effect? sorry for the spam if that issue was already changed in the ntuple maker.

== @sarafiorendi my understanding of the presentation is that the total tracking efficiency (which is what I report above) is correct. It is only if one attempts to plot the tracking efficiency obtained only by the displaced triplet seeds, then the results can be confusing.

@tomalin
Copy link
Collaborator Author

tomalin commented Mar 27, 2024

About the performance of the "inventStubs" option on displaced tracking, its effect was shown in these slides page 9-11: basically a negligible degradation in efficiency was seen in the 1 < eta < 1.75 region (from 64.85 ± 0.89 to 64.82 ± 0.89) on a displaced Mu PU 200 sample (I did not check the displaced algo on the TTBar sample).

The comment here does not mean that the invent stub does not work for displaced tracking, but was to stress that we are currently only inventing two out of the 3 stubs fo the displaced seeds, given the performance plots shown in the slides.

=== @sarafiorendi My understanding is that inventing stubs should have zero effect on the DR performance. But that it can potentially affect the KF, if the invented stubs have coordinates that differ from the original ones. Furthermore, it is impossible to invent all 3 stubs for the displaced triplet seeds, as these seeds do not preciseIy lie on the seed helix in the r-z plane, since the seed helix in r-z was determined using only 2 stubs. Is this correct? -- But given my observations, perhaps the invented stubs for displaced tracklets are imprecise, and this is messig up the KF performance?

@tomalin
Copy link
Collaborator Author

tomalin commented Mar 27, 2024

@dally96 if you could please suggest how to modify PurgeDuplicate.cc , so that numTracksComparedPerBin cuts on the "number of tracks excluding duplicates", rather than the "number of tracks", so corresponding to Thomas's FW, we can include that fix in this PR too.

@tomalin
Copy link
Collaborator Author

tomalin commented Mar 27, 2024

After fixing the bug in the calculation of "seedRank", the previous results become:

With original (Pt, phi) binning of DR:

number of tracks/event (pt > 2.00) = 296.55
efficiency for |eta| < 1.0 = 96.89 +- 0.15
efficiency for 1.0 < |eta| < 1.75 = 95.83 +- 0.25
efficiency for 1.75 < |eta| < 2.40 = 96.91 +- 0.29
combined efficiency for |eta| < 2.40 = 96.59 +- 0.12

Without (Pt,phi) binning of DR (and DR truncation per bin switched off)

number of tracks/event (pt > 2.00) = 269.97
efficiency for |eta| < 1.0 = 96.88 +- 0.15
efficiency for 1.0 < |eta| < 1.75 = 95.72 +- 0.25
efficiency for 1.75 < |eta| < 2.40 = 96.85 +- 0.29
combined efficiency for |eta| < 2.40 = 96.55 +- 0.12

So good efficiency with or without DR binning. But the binned DR has worse performance with 10% more output tracks. Since the binned DR emulation doesn't correspond to the latest FW, which uses only 1 bin, and a different recipe for truncation within the bin, I propose to disable the binning until it matches the FW.

@sarafiorendi Setting invent=false increases the efficiency by 0.2% and cuts the number of output tracks by 9%, so stub invention gives a small, but significant worsening of performance.

@tomalin tomalin marked this pull request as ready for review March 27, 2024 22:36
@tomalin tomalin changed the title Disable binning in DR DR: fix displaced track bug & disable binning Mar 27, 2024
@tomalin
Copy link
Collaborator Author

tomalin commented Mar 28, 2024

I'm merging this PR now, given the urgency with which it is needed by the trigger group and by us for the change over to combined modules. If people wish to improve on it, they can submit a second PR.

@tomalin tomalin merged commit ee50448 into L1TK-dev-14_0_0_pre2 Mar 28, 2024
1 check passed
@skinnari
Copy link

@tomalin thanks for tracking this down! for what it's worth, PR looks fine other than that we will probably want to push this to central CMSSW too, and then can't have the outcommented code.

tomalin added a commit that referenced this pull request Mar 28, 2024
* Disable binning in DR

* bug fix

* add comment

* code format

* tweak comment

* fix previous erroneous commit
@dally96
Copy link

dally96 commented Mar 29, 2024 via email

tomalin added a commit that referenced this pull request Apr 8, 2024
* Disable binning in DR

* bug fix

* add comment

* code format

* tweak comment

* fix previous erroneous commit
aryd added a commit that referenced this pull request Jun 11, 2024
* Andrews KF crash fix (#263)

* Add z0 resolution to performance printout (#264)

* Add z0 resolution to performance printout

* code format

* DR: fix displaced track bug & disable binning (#266)

* Disable binning in DR

* bug fix

* add comment

* code format

* tweak comment

* fix previous erroneous commit

* DUMMY COMMIT BEFORE PR TO CENTRAL CMSSW

* Run combined modules by default (#265)

* Make combined modules default

* tweak

* Improve USEHYBRID ifdef range

* Fix compiler error for pure Tracklet algo

* Move fitpattern.txt refs, so only used for pure Tracklet algo

* code format

* Numerical stability fix (#269)

* Made calculations more numerically stable.

* Explicitly restrict to domain of asin/acos.

* Added comments.

* Code format.

* Fixed typos in comments.

* Only do calculations when needed.

* Added const where applicable.

* Fix inventStubs bug in duplicate removal (#271)

* Fix inventStubs bug in duplicate removal

When using L2L3D1 seeds, it is only L2L3 that are used to determined the r-z helix params, not L2D1 as the code currently assumes.

* Update PurgeDuplicate.cc

* add comment

* formatted

* Manually incorporating DTC stub, TT stub changes to CMSSW 14 dev branch

* First pass at removing non-combined modules

* Removed unused iMath code

* Avoid stale pointers on subsequent events

* Cleaner MP pointer checks

Full agreement with HLS (100 events L1PHIC - D5PHIC)

* First code for a ProjectionCalculator module

* Set of changes to make a configuration that uses the Projection Calculator module

* Fix mistake in code merging

* Changes needed for the VMSME Router module

* Fully implementing VMSMERouter, VMRouterCM no longer producing VMMEStubs

* Manually incorporating DTC stub, TT stub changes to CMSSW 14 dev branch

* Readding TCBase, VMSMER compatible w/ updated stub format

* Addressing comments, fixing code format

* Removing unneeded diskpswrittenr variable

* Removing unneeded + 1 to rbits in TP

* Updates to make sure we don't have missing projections

---------

Co-authored-by: Ian Tomalin <ian.tomalin@stfc.ac.uk>
Co-authored-by: Andrew Hart <ahart@cern.ch>
Co-authored-by: Anders <aryd@cern.ch>
Co-authored-by: bryates <brent.yates@email.ucr.edu>
@tomalin tomalin deleted the ianDisableDRbinning branch July 18, 2024 10:16
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.

4 participants