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: fix displaced track bug & disable binning #266

Closed tomalin closed 8 months ago

tomalin commented 8 months ago

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 https://github.com/cms-L1TK/cmssw/pull/265 .

tomalin commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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.

skinnari commented 8 months ago

@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.

dally96 commented 8 months ago

Hi Ian,

I noticed there seems to be a dip in the eta efficiency that corresponds with an increase in tracks for the extended tracking (I’ve attached plots). Is there anything specific about the extended tracking w.r.t. eta? effVsEta_D88_promptVsDisplaced.pdf extended_trk_eta 1.pdf

Thanks, Daniel



On Mar 27, 2024, at 11:29 PM, Ian Tomalin @.***> wrote:

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.

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.

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