DTC_merge2 branch updated to 1130 #74

skinnari · 2021-03-02T18:47:28Z

TEST: Anders' DTC_merge2 branch manual porting

~4 large changes (use of new DTC code; TrackletConfigBuilder; updates to combined modules; and cleanup of the Projection and Residuals classes (no longer separate for Disk and Layers). But also lots of small cleanup.

…tion

… moving functionality to Sector.

…implifies/unifies the code and makes it similar to the HLS

…d then move to the next sector. This saves memory

… in HLS

pwittich · 2021-04-01T14:13:23Z

L1Trigger/TrackFindingTracklet/src/VMRouterTable.cc

-          writeVMTable(settings_.tablePath(), "VMTableOuter" + fnamesuffix + ".tab", vmrtabletedisk_);
-        }
+      }
+      // write disk teouter tables (D1, D2, D4)


indentation problems here

This was more than indentation - a { was misplaced. Has been corrected.

skinnari · 2021-04-02T15:47:54Z

L1Trigger/TrackFindingTracklet/interface/CircularBuffer.h

@@ -28,6 +28,11 @@ namespace trklet {
    //Almost full if writer ptr incremented by 1 or 2 is same as read ptr
    bool almostfull() const { return (((wptr_ + 1) % size_) == rptr_) || (((wptr_ + 2) % size_) == rptr_); }

+    //Almost full if writer ptr incremented by 1 or 2 is same as read ptr


the comment for "nearfull" is identical to that of "almostfull" - can they be made more meaningful to distinguish?

Fixed comment.

skinnari · 2021-04-02T15:49:55Z

L1Trigger/TrackFindingTracklet/interface/L1SimTrack.h

@@ -0,0 +1,53 @@
+// This holds two classes: L1SimTrack (truth level simulated track), and SLHCEvent (support for maintaining standalone running)


fix comment, this now holds just one class "L1SimTrack"

Fixed - comment made relevant to the class.

skinnari · 2021-04-02T15:51:22Z

L1Trigger/TrackFindingTracklet/interface/L1TStub.h

@@ -43,10 +38,14 @@ namespace trklet {

    unsigned int layer() const { return layer_; }
    int disk() const {
+      if (layerdisk_ < 6) {


6 => N_LAYER ?

skinnari · 2021-04-02T15:51:42Z

L1Trigger/TrackFindingTracklet/interface/L1TStub.h

+      if (layerdisk_ < 6) {
+        return 0;
+      }
+      int disk = layerdisk_ - 5;


5 => "(N_LAYER-1)" ?

skinnari · 2021-04-02T15:53:03Z

L1Trigger/TrackFindingTracklet/interface/MatchEngineUnit.h

+              unsigned int nrzbin,
+              unsigned int rzbin,
+              unsigned int iphi,
+	      int shift,


this "formatting" makes me suspect that code formats have not been run? (in cmssw: scram b code-format )

Ran the reformat command (53 files updated)

skinnari · 2021-04-02T17:03:10Z

L1Trigger/TrackFindingTracklet/src/TrackletProcessor.cc

+      }
+
+      int lutval = -1;
+      if (iSeed_ < 6) {  //FIXME should only be one table...


Not now - need non-trivial coordination with HLS development.

skinnari · 2021-04-02T17:03:34Z

L1Trigger/TrackFindingTracklet/src/TrackletProcessor.cc

+            assert(ireg * nbins + ibin < outervmstubs_->nBin());
+            int nstubs = outervmstubs_->nVMStubsBinned(ireg * nbins + ibin);
+
+            if (print)


skinnari · 2021-04-02T17:06:35Z

L1Trigger/TrackFindingTracklet/src/VMRouterCM.cc

+          continue;
+
+        int absz = std::abs(stub->z().value());
+        if (layerdisk_ == 1 && absz < 50.0 / settings_.kz(layerdisk_))


what are the 50.0, 95.0, 55.0, 70.0 ?

Cut on z or r position - replaced with more meaningful constants.

skinnari · 2021-04-02T17:07:49Z

L1Trigger/TrackFindingTracklet/src/VMRouterTable.cc

        if (bin < 0)
          bin = 0;
        if (bin >= NBINS / 2)
-          bin = NBINS / 2 - 1;
+          //bin = NBINS / 2 - 1;


what's going on here? should this out-commented be deleted or is it a temporary fix?

Should be deleted - now done.

skinnari · 2021-04-02T17:09:31Z

L1Trigger/TrackFindingTracklet/test/BuildFile.xml

@@ -7,9 +7,15 @@
    <use   name="root"/>
    <use   name="heppdt"/>
    <use   name="CommonTools/UtilAlgos"/>
+    <use   name="CondFormats/Alignment"/>


this seems to be a fail in merging. this build file has been cleaned up by central CMSSW so these changes should be reverted.

skinnari · 2021-04-02T17:12:42Z

@aryd @tomalin @pwittich

i made a bunch of comments as i went through this code, to keep track on things that need to be fixed minimally prior to making a PR to central CMSSW. it's just cosmetics, so if this is needed for HLS developments i can merge it and we get the cleanups implemented separately?

this is assuming that tracking performance is still good, which I am in the process of checking too.

skinnari · 2021-04-02T18:44:44Z

Update on performance @aryd

-- displaced tracking crashes on first event (maybe this was known already, lost track):

cmsRun: /afs/cern.ch/work/s/skinnari/workarea/L1TK/PR_new/CMSSW_11_3_0_pre3/src/L1Trigger/TrackFindingTracklet/src/Sector.cc:103: bool trklet::Sector::addStub(trklet::L1TStub, std::string): Assertion `nadd == 1' failed.

-- performance comparison is attached for hybrid tracking with ttbar + PU200: efficiency is slightly reduced in particular for negative eta, it seems.

L1TK-comparison_2april2021.pdf

aryd · 2021-04-02T18:59:07Z

Louise, thanks for checking this. Yes the displaced tracking is not working; I have fixed some problems locally in the configuration such that it now runs - but there are still some efficiency issues I'm trying to resolve. The efficiency for the default tracking can probably be improved a little by tuning the bend pt cuts for the new bend encoding in the new DTC code. The new code has the truncation now carefully adjusted to correspond to what we can do in HLS. So the emulation is not realistic with respect to what we can do in HLS. I will look at the other comments on the code a.s.a.p. -Anders

…

On 4/2/21 2:44 PM, Louise Skinnari wrote: Update on performance @aryd <https://github.com/aryd> -- displaced tracking crashes on first event (maybe this was known already, lost track): cmsRun: /afs/cern.ch/work/s/skinnari/workarea/L1TK/PR_new/CMSSW_11_3_0_pre3/src/L1Trigger/TrackFindingTracklet/src/Sector.cc:103: bool trklet::Sector::addStub(trklet::L1TStub, std::string): Assertion `nadd == 1' failed. -- performance comparison is attached for hybrid tracking with ttbar + PU200: efficiency is slightly reduced in particular for negative eta, it seems. L1TK-comparison_2april2021.pdf <https://github.com/cms-L1TK/cmssw/files/6250863/L1TK-comparison_2april2021.pdf> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#74 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABI4LTJQOMS4MJAK34RLFYTTGYGCTANCNFSM4YPRFMXQ>.

-- =========================================================================== Anders Ryd E-mail: ***@***.*** Phone: 1-607-255-2529 (PSB), +41-75-411-5765 or 165765 (CERN) Address: Cornell University, 393 Physical Sciences Building, Ithaca NY 14853

…erge_1130

… tracking

* Anders DTC merge with a gazillion manual fixes * propagate buildfile fixes * code formats * fix cherrypick mess up * Remove LayerProjection class * Further cleanup of tracklet projections interface * Correct number of processing steps by changing < to <= * Add missing include of algorithm * Remove extra const * Remove some commented out code * Remove duplicate code * Fix message logger and DTC Stub for consistency with hybrid configuration * Fix problem with writing of input link memories * Create Residual class that will replace LayerResidual and DiskResidual * Remove the use of the class DiskResiduals * Remove unused nMatch and nMatchDisk method of Tracklet * (Re-)Implement the correction to writing the DTC data link file after moving functionality to Sector. * Combine addMatch method for disk and layers into on method * combine the disk and layer match into one method * Remove some redundant poiters to l1tstubs * Pass iSeed to Tracklet * Introduce an InputRouter module. Does not change functionality, but simplifies/unifies the code and makes it similar to the HLS * Cleanup of writing the DTC link files * Change processing order such that all steps in one sector are done and then move to the next sector. This saves memory * Interface updates for CMSSW following change to module processing order * Change in VMRouter to processing PS links before 2S in disks * Cleanup of unused iSector variable in processs and memory modules * Fixes to make the HybridTracks_cfg.py run * Cleanup of hardcoded numbers etc. * Updates to MP to put all regions into one memory slot in the ProjectionTemp * Fix to calculation of irinv for projections - no matches what is done in HLS * Remove now unused file paths for the old cable mapping code * Correct missplaced curly bracket * Fixes for the displaced tracking * Fix to avoid duplicate VMSTE name in D1 for standard configuration * Address comments from Louise S. * Ran scram b code-format * Address comments from Louise S. * Addressing more comments from Louise S. * More fixes to comments * Make running hybrid default (not displaced) * restore buildfile * Add DTC link config to Settings.h * Changes to suppress warning in MatchCalculator when running displaced tracking * Fix typo introduced in code cleanup for MatchProcessor Co-authored-by: Anders Ryd <ar322@cornell.edu> Co-authored-by: Anders <aryd@cern.ch>

skinnari and others added 30 commits March 2, 2021 19:37

Anders DTC merge with a gazillion manual fixes

67f080b

propagate buildfile fixes

8e775d1

code formats

650ff58

fix cherrypick mess up

b140ffe

Remove LayerProjection class

04d9dfa

Further cleanup of tracklet projections interface

549ac30

Correct number of processing steps by changing < to <=

e647457

Add missing include of algorithm

b51b039

Remove extra const

cf5498d

Remove some commented out code

1bf7df1

Remove duplicate code

936da8a

Fix message logger and DTC Stub for consistency with hybrid configura…

314eb77

…tion

Fix problem with writing of input link memories

0371772

Create Residual class that will replace LayerResidual and DiskResidual

c9c646a

Remove the use of the class DiskResiduals

da78939

Remove unused nMatch and nMatchDisk method of Tracklet

5f76609

(Re-)Implement the correction to writing the DTC data link file after…

a181be9

… moving functionality to Sector.

Combine addMatch method for disk and layers into on method

946b120

combine the disk and layer match into one method

4c6a774

Remove some redundant poiters to l1tstubs

6255829

Pass iSeed to Tracklet

c19d68d

Introduce an InputRouter module. Does not change functionality, but s…

52d12b8

…implifies/unifies the code and makes it similar to the HLS

Cleanup of writing the DTC link files

023c439

Change processing order such that all steps in one sector are done an…

00a8a23

…d then move to the next sector. This saves memory

Interface updates for CMSSW following change to module processing order

f4109cc

Change in VMRouter to processing PS links before 2S in disks

bd26658

Merge changes

c7487ed

Cleanup of unused iSector variable in processs and memory modules

d4d39b7

Fixes to make the HybridTracks_cfg.py run

44ea72e

Cleanup of hardcoded numbers etc.

6d8b6f8

Anders Ryd added 3 commits March 25, 2021 17:30

Fix to calculation of irinv for projections - no matches what is done…

ed2269e

… in HLS

Remove now unused file paths for the old cable mapping code

271ec43

Correct missplaced curly bracket

be7420e

pwittich reviewed Apr 1, 2021

View reviewed changes

skinnari assigned skinnari and unassigned skinnari Apr 2, 2021

skinnari commented Apr 2, 2021

View reviewed changes

Anders Ryd and others added 13 commits April 2, 2021 17:15

Fixes for the displaced tracking

c90b216

Fix to avoid duplicate VMSTE name in D1 for standard configuration

b26fc19

Address comments from Louise S.

576bda0

Ran scram b code-format

de0f887

Address comments from Louise S.

2a823e0

Addressing more comments from Louise S.

64cd602

More fixes to comments

cc9e791

Make running hybrid default (not displaced)

e164c32

restore buildfile

0c972b5

Add DTC link config to Settings.h

35928d1

Merge branch 'DTC_merge_1130' of github.com:cms-l1tk/cmssw into DTC_m…

e8618ed

…erge_1130

Changes to suppress warning in MatchCalculator when running displaced…

adcfecf

… tracking

Fix typo introduced in code cleanup for MatchProcessor

d51d818

skinnari merged commit 15d2ee6 into L1TK-dev-11_3_0_pre3 Apr 11, 2021

BenjaminRS mentioned this pull request May 2, 2023

L1FPGATrackProducer fails in special relval with outer tracker inefficiency in CMSSW_12_6_4 cms-sw/cmssw#41357

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DTC_merge2 branch updated to 1130 #74

DTC_merge2 branch updated to 1130 #74

skinnari commented Mar 2, 2021 •

edited by tomalin

Loading

pwittich Apr 1, 2021

aryd Apr 3, 2021

skinnari Apr 2, 2021

aryd Apr 3, 2021

skinnari Apr 2, 2021

aryd Apr 3, 2021

skinnari Apr 2, 2021

aryd Apr 3, 2021

skinnari Apr 2, 2021

aryd Apr 3, 2021

skinnari Apr 2, 2021

aryd Apr 4, 2021

skinnari Apr 2, 2021

aryd Apr 5, 2021

skinnari Apr 2, 2021

aryd Apr 5, 2021

skinnari Apr 2, 2021

aryd Apr 5, 2021

skinnari Apr 2, 2021

aryd Apr 5, 2021

skinnari Apr 2, 2021

skinnari commented Apr 2, 2021

skinnari commented Apr 2, 2021

aryd commented Apr 2, 2021 via email

		@@ -0,0 +1,53 @@
		// This holds two classes: L1SimTrack (truth level simulated track), and SLHCEvent (support for maintaining standalone running)

DTC_merge2 branch updated to 1130 #74

DTC_merge2 branch updated to 1130 #74

Conversation

skinnari commented Mar 2, 2021 • edited by tomalin Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinnari commented Apr 2, 2021

skinnari commented Apr 2, 2021

aryd commented Apr 2, 2021 via email

skinnari commented Mar 2, 2021 •

edited by tomalin

Loading