search

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

DTC phi range hard-wired constants removed #141

Closed tomalin closed 2 years ago

tomalin commented 2 years ago

This eliminates the array of hard-wired constants from TrackletConfigBuilder that specified the phi ranges of modules read by each DTC. Instead these constants are determined at start of run from the CMS geometry DB. This is done by new function setDTCphirange().

Two additional functions are added, which are only needed to support stand-alone emulation, since it can't access the DB. writeDTCphirange() can optionally, when running within CMSSW, write the calculated DTC phi ranges to file dtcphirange.txt ; and setDTCphirange() can, when running stand-alone, read in dtcphirange.txt to avoid needing the DB.

The pragma CMSSW_GIT_HASH is used to switch between these depending on whether running standalone or not.

In addition, the calculation of the DTC phi ranges is now correct in the tilted barrel modules. Though this doesn't change the wiring map.

The file dtcphiranges.txt has been added to https://github.com/cms-L1TK/L1Trigger-TrackFindingTracklet .

Validation: I've checked that wires.dat etc. are unaffected.

P.S. I realized after that the only place that uses the DTC phi ranges is writeILMemories(), which added this offset to them all https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-12_0_0_pre4/L1Trigger/TrackFindingTracklet/src/TrackletConfigBuilder.cc#L1276 before using them. The only reason this was needed is that the DTC phi ranges had not been converted correctly from DTC to Tracklet format. I've now updated the DTC phi range calculation in setDTCphirange() to fix this, so the offset in writeILMemories() has been removed. This means that the calculated phi values are now all offset with respect to the original ones.

tomalin commented 2 years ago

@aryd I've verified that the DTC phi ranges calculated by this new PR are identical to your old hard-wired constants (aside from the improved tilted module treatment). However, I'm confused. 1) https://github.com/cms-L1TK/cmssw/blob/ianDTCphirange/L1Trigger/TrackFindingTracklet/src/TrackletConfigBuilder.cc#L99 shifts the DTC phi range from the DTC phi definition to the Tracklet phi definition. It does so by adding PI/N_SECTOR. But fromslide 2 of https://indico.cern.ch/event/965564/contributions/4066275/attachments/2123056/3573945/SectorDef.pdf , I think this should really be PI/NSECTOR + 0.5 * dphisectorHG? 2) writeILmemories() adds 0.5 dphisectorHG_ - PI/N_SECTOR to these DTC phi ranges before using them. I assume the 0.5dphisectorHG term is needed because (1) incorrectly left it out. But I don't understand the PI/N_SECTOR term.

aryd commented 2 years ago

Comments inline below:

@aryd I've verified that the DTC phi ranges calculated by this new PR are identical to your old hard-wired constants (aside from the improved tilted module treatment). However, I'm confused.

  1. https://github.com/cms-L1TK/cmssw/blob/ianDTCphirange/L1Trigger/TrackFindingTracklet/src/TrackletConfigBuilder.cc#L99 shifts the DTC phi range from the DTC phi definition to the Tracklet phi definition. It does so by adding PI/N_SECTOR. But fromslide 2 of https://indico.cern.ch/event/965564/contributions/4066275/attachments/2123056/3573945/SectorDef.pdf , I think this should really be PI/NSECTOR + 0.5 * dphisectorHG?

dphisectorHG is the full width of the hourglass sector, about 1.02 rad. So to shift from the DTC phi coordinate definition to the hybrid phi definition you add 0.5*dphisectorHG. This is what I see if I follow the link above to your new code. I'm not sure where you see PI/N_SECTOR.

  1. writeILmemories() adds 0.5 dphisectorHG_ - PI/N_SECTOR to these DTC phi ranges before using them. I assume the 0.5dphisectorHG term is needed because (1) incorrectly left it out. But I don't understand the PI/N_SECTOR term.

If you look at the phi coordinate here for the dtcs they range from about -0.18 to +0.88 which means that they are 'actual phi' measured from 'absolute phi=0'. Hence the shift to transform these phi coordinates are shifted by 0.5 * dphisectorHG_ - PI/N_SECTOR.

Does this make sense?

-Anders

tomalin commented 2 years ago

Comments inline below:

@aryd I've verified that the DTC phi ranges calculated by this new PR are identical to your old hard-wired constants (aside from the improved tilted module treatment). However, I'm confused.

  1. https://github.com/cms-L1TK/cmssw/blob/ianDTCphirange/L1Trigger/TrackFindingTracklet/src/TrackletConfigBuilder.cc#L99 shifts the DTC phi range from the DTC phi definition to the Tracklet phi definition. It does so by adding PI/N_SECTOR. But fromslide 2 of https://indico.cern.ch/event/965564/contributions/4066275/attachments/2123056/3573945/SectorDef.pdf , I think this should really be PI/NSECTOR + 0.5 * dphisectorHG?

dphisectorHG is the full width of the hourglass sector, about 1.02 rad. So to shift from the DTC phi coordinate definition to the hybrid phi definition you add 0.5*dphisectorHG. This is what I see if I follow the link above to your new code. I'm not sure where you see PI/N_SECTOR.

  1. writeILmemories() adds 0.5 dphisectorHG_ - PI/N_SECTOR to these DTC phi ranges before using them. I assume the 0.5dphisectorHG term is needed because (1) incorrectly left it out. But I don't understand the PI/N_SECTOR term.

If you look at the phi coordinate here for the dtcs they range from about -0.18 to +0.88 which means that they are 'actual phi' measured from 'absolute phi=0'. Hence the shift to transform these phi coordinates are shifted by 0.5 * dphisectorHG_ - PI/N_SECTOR.

Does this make sense?

-Anders

I think this makes sense. I added my "P.S." (and corresponding code change) in https://github.com/cms-L1TK/cmssw/pull/141#issue-1159495006 after understanding this.

Though since the two DTC sectors that are read into each L1 track sector are rotated in phi relative to the latter by +- PI/N_SECTORS, I'd expect to see the calculations of "_A" and "_B" (corresponding to these two DTC sectors), which is done at https://github.com/cms-L1TK/cmssw/blob/L1TK-dev-12_0_0_pre4/L1Trigger/TrackFindingTracklet/src/TrackletConfigBuilder.cc#L1301 having shifts in phi by - and + PI/N_SECTORS for _A and _B calculations respectively. But this I don't see.

tomalin commented 2 years ago

@aryd I've committed what I believe is a fix for a major bug in TrackletConfigBuilder::writeILMemories(). This neglected that the DTC sectors are rotated by half a sector w.r.t. the L1 tracking sector. My proposed bug fix is L1222-L1239 of https://github.com/cms-L1TK/cmssw/blob/ianDTCphirange/L1Trigger/TrackFindingTracklet/src/TrackletConfigBuilder.cc#L1222 , and the original code (commented out) is L1242-64.

To my astonishment, the L1 tracking performance (checked on 1k ttbar+200PU events) is identical before and after this fix. However, the number of connections between the IR & VMR goes down from 336 to 304. The bug fix deletes 32 connections that were originally there, and does not add any new ones. If the old code just had unnecessary extra connections, this may explain why the tracking performance didn't change.

In the case of the summer chain, I've checked that this bug fix reduces the number of IR to VMR connections from 18 to 15.

Am I correct?

tomalin commented 2 years ago

I've changed dtcphirange.txt from C++ code to simply numbers, as requested by Anders, and modified TrackletConfigBuilder.cc accordingly. And updated dtcphirange.txt in https://github.com/cms-L1TK/L1Trigger-TrackFindingTracklet . Note that TrackletConfigBuilder.cc now opens and reads this file rather than #including it. The code that does this read is not thread safe, but as the stand-alone code is single threaded, this should be OK.

skinnari commented 2 years ago

@aryd @tomalin are you both happy with where this PR stands now? can it be merged or are there further updates / fixes needed?

aryd commented 2 years ago

Ian and I meet at CERN a few weeks ago and discussed this. I think we agreed on a few more tweaks. Ian - have you implemented these changes?

tomalin commented 2 years ago

Ian and I meet at CERN a few weeks ago and discussed this. I think we agreed on a few more tweaks. Ian - have you implemented these changes?

@aryd I made the changes mentioned in https://github.com/cms-L1TK/cmssw/pull/141#issuecomment-1069008425 following our conversation. I believe this is everything you wanted.

aryd commented 2 years ago

I tried to go through and approve these changes. (I find the GitHub pages for code review extremely hard to follow...) I think I could now click 'Squash and merge' but I wanted to check that I was not missing something.