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

Atownse2 calc bend cuts #178

Closed atownse2 closed 1 year ago

atownse2 commented 2 years ago

PR description:

This PR allows the bend decoding to be a function of the FE stub bend windows.

PR validation:

All tests are using TTbar_200PU events. The name "L1TK" refers to the default code and "CalcBendCuts" refers to the version with the proposed changes.

In the TrackletLUT::initPhiCorrTable I added the option of using z bins in the tilted layers, the options below show the comparison of 1, 2, and 13 z bins. Also shown is the output when the flag useCalcBendCuts is set to false (changes off).

Edit: Because there is no definite performance difference between 1, 2, and 13 z bins in the PhiCorrTable, to simplify the code only 1 z bin is used.

Screen Shot 2022-07-12 at 2 16 06 PM

Using more that one z bin would require changes in firmware to calculate the z bin of a stub in the tilted barrel, because of this and the lack of substantial improvement I would recommend using just one z bin here. Later versions shown will be using only one z bin in the PhiCorrTable.

To validate that this code does indeed use the FE stub windows, I compare the output of running both versions with modified stub windows. These tunes are the result of work done by Reza Goldouzian (reza.goldouzian@cern.ch). The validation done here is only to show that this code can adapt to modified FE stub bend windows.

The tunes ["newTight", "newLoose"] are similar to ["tight", "loose"] ; ["0p5", "7p0"] are tunes where the FE stub bend windows are all set to the same value, 0.5 and 7.0 respectively.

Screen Shot 2022-07-12 at 2 43 38 PM

Also tested is the combined version, using TrackletProcessor instead of TrackletEngine+TrackletCalculator.

Screen Shot 2022-07-12 at 2 45 29 PM

In L1Trigger/TrackFindingTracklet/interface/Settings.h I define "bendcut" parameters that are found through a process of optimizing for several metrics in the TE (ME), including pass rate of tp matched tracklets (matches), rate of truncation, and pass rate of fake (not tp matched) tracklets (matches).

Some plots of this are shown here: https://indico.cern.ch/event/1161122/contributions/4897597/attachments/2453548/4204791/L1TK%20Update%20053122.pdf

Before submitting your pull requests, make sure you followed this checklist:

runall-report-step123-L1TK.log runall-report-step123-CalcBendCuts.log

tomalin commented 2 years ago

@aryd Although it doesn't affect the approval of this PR, I think these results show that switching from separate to combined modules (with your default 1 bin) reduce the tracking efficiency from 95.30% (calcBendCuts_1zbinPhiCorr) to 94.82% (calcBendCuts_TrackletProcessor_1zbinPhiCorr). I guess this is due to truncation in the TP. It would be nice to fix this one day ...

aryd commented 2 years ago

I'm not happy that we now have the dependency on Setup in the code. The include was added in multiple places where it is not really needed. But the real use is in the TrackletLUT.cc. I did not try to understand exactly what is needed, but I wonder if it is possible to 'reverse' the dependency so that instead of passing in the Setup object itself at the top level we just pass in the required data to allow this track finding code to be able to run standalone.

tomalin commented 2 years ago

@aryd & @atownse2 What is the status of this PR?

aryd commented 2 years ago

Austin and I was going to talk again today at 2PM EDT about some followup. We did discuss this earlier.

My biggest issue is that we are bringing in outside dependencies such that it will not be possible to run the code standalone. Maybe we will have to go that way - but my ability to do work development on the code will significantly drop since I just find the environment in CMSSW so much more tedious to work in….

-Anders

Anders Ryd @.**@.>

On Sep 2, 2022, at 11:21 AM, Ian Tomalin @.**@.>> wrote:

@arydhttps://github.com/aryd & @atownse2https://github.com/atownse2 What is the status of this PR?

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/178#issuecomment-1235627604, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTM6LOK2DRFYVPLIPILV4ILQHANCNFSM53MDAXIQ. You are receiving this because you were mentioned.Message ID: @.***>

atownse2 commented 2 years ago

The latest commit hopefully addresses all the comments made by @aryd. Please let me know if you need anything else.

tomalin commented 1 year ago

@aryd since Austin says he has addressed all your comments, are you able to approve this PR?

tomalin commented 1 year ago

Note from L1TrkAlgo meeting: @aryd believes this PR still breaks the stand-alone functionality of the L1Trk code. But given the amount of work to fix this, he's inclined to merge this PR anyway. We wait for him to approve it.