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

Emulation of DRin and DR added. Emulation of KFin altered. #205

Closed tschuh closed 1 year ago

tschuh commented 1 year ago

With this PR we replace the old emulation Chain Tracklet -> TBout -> KFin -> KF with the new Tracklet -> TBout -> DRin -> DR -> KFin -> KF chain.

DRin does read in the TRack Builder Output, create fake Seed stubs and remap the stub layer to kf layer. Seed Type channel will be rerouted to pt channel. It also transforms the tracklet data types into the ones needed by KF.

DR is an emulation of "a" duplicate removal algorithm, to be improved together with f/w.

KFin merges the pt channel to a smaller number to save kf resources and calculates stub residual uncertainties.

tschuh commented 1 year ago

I can run the L1TrackNtupleMaker_cfg.py fine configured as HYBRID as well as HYBRID_NEWKF running on RelVal samples and https://cernbox.cern.ch/remote.php/dav/public-files/FBoyXLyRfKTFpbx/skimmedForCI_12_6_0.root @tomalin Could you please fix the CI?

tschuh commented 1 year ago

As far as I can see the CI misses to checkout SimTracker/TrackTriggerAssociation and L1Trigger/TrackerDTC which causes the errors. @tomalin please fix that.

tomalin commented 1 year ago

As far as I can see the CI misses to checkout SimTracker/TrackTriggerAssociation and L1Trigger/TrackerDTC which causes the errors. @tomalin please fix that.

Since your PR doesn't modify any code in these two directories, they don't need to be checked out. The scram compilation will just take them from the official CMSSW release.

tschuh commented 1 year ago

I can just say that it works when one checks them out...

tomalin commented 1 year ago

@kwalkingshaw do you have time to review this, particularly for what concerns the DR algorithm that it implements?

kwalkingshaw commented 1 year ago

Approved changes.

tschuh commented 1 year ago

I guess I will now extend the emulation to take proper seed stub id from TrackBuilder, you may want to wait with the merge until that is done.

tschuh commented 1 year ago

We use now seed stubIds and performance is good (reducing duplicates by one order of magnitude). I found that stub radius send by TB uses dtc format except that it uses 12b instead of 7b for 2S disc stubs. Also seed stubIds only have 7 out of 10 bits available compared to projected stubs. I found that comparing one stub per layer (instead of all stubs with all stubs) using only 7 lsb stubId (instead of 4b detector layer + 10 stubId) to perform good enough. That means the routing into kf layer is beneficial. Therefore the code in this PR corresponds to the simplest f/w I can imagine. I am wondering if stub duplication at pt boundaries is actually needed...

tschuh commented 1 year ago

CI complains about: FAILURE -- TRACKING EFFICIENCY TOO LOW 96.1228 < 97.0 Historic tracking efficiency (for comparison) = 97.3 what shall I say, realistic duplicate removal performs not as good as fancy s/w. You have to break an egg to make an omelette... @tomalin can you please do something about that?

tomalin commented 1 year ago

@tschuh , is this ready to merge in your opinion, or are you still playing with the DR algo to recover the lost tracking efficiency etc.?

tomalin commented 1 year ago

Please add a comment, e.g. to top of DR.h or DR.cc explaining, in a few sentences, which duplicate removal algo has actually been implemented.

tomalin commented 1 year ago

CI complains about: FAILURE -- TRACKING EFFICIENCY TOO LOW 96.1228 < 97.0 Historic tracking efficiency (for comparison) = 97.3 what shall I say, realistic duplicate removal performs not as good as fancy s/w. You have to break an egg to make an omelette... @tomalin can you please do something about that?

I've modified the CI to reduce the HYBRID_NEWKF reference tracking efficiency to 96.0%.

tschuh commented 1 year ago

Many thanks Ian. Tackling the loss inside DR needs mayor changes. I am not sure if we want to make these eventually. So I would wait with modifying the code. I am also having the hope to tackle this loss by changing the Track Builder process order.