PurgeDuplicates test-vectors (11_3_X) #70

aehart · 2021-02-12T17:32:29Z

PR description:

This is a duplicate of PR #69, but rebased to L1TK-dev-11_3_0_pre3.

This PR includes various changes needed to output a first version of the test-vectors for the development of the PurgeDuplicates HLS module, including both the TrackBuilder and TrackMerger steps.

These changes involve turning on writing the TrackFitMemories and CleanTrackMemories (corresponding to the output vectors for the TrackBuilder and TrackMerger, respectively) in the TrackletEventProcessor, and adjusting the output format in Tracklet::trackfitstr(). The stub r has also been added in Tracklet::fullmatchstr() and Tracklet::fullmatchdiskstr() (input vectors for the TrackBuilder).

PR validation:
Running the code with writeMem set to true in Settings.h correctly produces the test-vectors currently in use for development of the PurgeDuplicates module. The usual code checks and format scripts have also been run.

skinnari · 2021-02-12T17:38:35Z

L1Trigger/TrackFindingTracklet/src/Tracklet.cc

-  string stubid2 = "111111111";
-  string stubid3 = "111111111";
+const std::string Tracklet::layerstubstr(const unsigned layer) const {
+  assert(layer <= 5);


super minor but can the 5 be a constant? (like N_DISK if that's what the 5 means)

correction: can be "layer < N_LAYER" ? where N_LAYER==6 (didn't read properly first...)

skinnari · 2021-02-12T17:39:06Z

L1Trigger/TrackFindingTracklet/src/Tracklet.cc

    }
+    const FPGAWord tmp(trackIndex_, 7, true, __LINE__, __FILE__);


can we avoid hardcoded "7"?

skinnari · 2021-02-12T17:40:37Z

L1Trigger/TrackFindingTracklet/src/Tracklet.cc

-    }
-  }
+const std::string Tracklet::diskstubstr(const unsigned disk) const {
+  assert(disk <= 4);


similarly here, can this be "disk < N_DISK" where N_DISK=5 from Settings.h?

skinnari · 2021-02-12T17:41:06Z

L1Trigger/TrackFindingTracklet/src/Tracklet.cc

+  if (!diskresid_[disk].valid())
+    oss << "0|0000000|0000000000|000000000000|000000000000|0000000";
+  else {
+    if (trackIndex_ < 0 || trackIndex_ > 127) {


avoid 127 hardcoded if possible

skinnari · 2021-02-19T14:55:29Z

@tomalin could you please have a look and see that you agree with the updates, then we can merge this? thanks!

tomalin · 2021-02-19T15:41:02Z

Are the names of the output memories sensible?

Output of TrackBuilder is TrackFitMemories, despite the fact TrackBuilder doesn't fit the tracks.
Output of TrackMerger is CleanTrackMemories. (Though DupKilledMemories might have been clearer). And slide 2 of https://indico.cern.ch/event/957554/contributions/4025232/attachments/2118379/3564551/HLSUpdateL1TrkAlg071020.pdf says TrackMerger writes MergeTrackMemories.

aehart · 2021-02-19T16:39:47Z

Are the names of the output memories sensible?

No, not really. I was hesitant to change the names because they are somewhat baked into the wiring files. That being said, I don't have an opinion one way or the other, so if you would like the names changed, I will change them.

If so, I would propose doing the following:

Throughout the C++ code, change the TrackFitMemory to something like BuiltTrackMemory and CleanTrackMemory to either DupKilledMemory or MergeTrackMemory (I'm open to suggestions on those names).
Update the following lines in Sector.cc so that both the previous names and the new names are supported in the wiring files:
https://github.com/aehart/cmssw/blob/8654097cc794f190486fa2ac4bbfc63d2732f0e6/L1Trigger/TrackFindingTracklet/src/Sector.cc#L119-L122
So we would wind up with something like the following:

 } else if (memType == "TrackFit:" || memType == "BuiltTrack:") {
   addMemToVec(BT_, memName, settings_, isector_, phimin_, phimax_);
 } else if (memType == "CleanTrack:" || memType == "DupKilled:") {
   addMemToVec(DK_, memName, settings_, isector_, phimin_, phimax_);

Update the wiring files with the new names. I guess this is done with a PR to the cms-data/L1Trigger-TrackFindingTracklet?
When everything settles down and is merged, remove the old names from Sector.cc.

Technically, we don't need to update the names of the memories in the wiring files, but I think consistency is a good goal.

tomalin · 2021-02-19T17:00:15Z

@aryd as you're preparing C++ wiring code, would the proposed memory name change mentioned above cause any problems that you can see?

aryd · 2021-02-19T17:11:38Z

I have not looked through the code; but is this backward compatible in the sense that you can still run the workflow with the 'old' trackfit and the duplicate removal? In particular, is the BT_ and DK_ new types of memories? If so I presume that they would not work with the track fit and the duplicate removal.

…

On 2/19/21 12:00 PM, Ian Tomalin wrote: @aryd <https://github.com/aryd> as you're preparing C++ wiring code, would the proposed memory name change mentioned above cause any problems that you can see? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABI4LTIGJMQOP3N6IO4R6B3S72KLBANCNFSM4XRBXU7Q>.

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

aehart · 2021-02-19T17:51:21Z

In particular, is the BT_ and DK_ new types of memories?

@aryd No, these would just be new names for what are now known as the TrackFitMemory and CleanTrackMemory. Nothing about the classes would change other than their names, which are meant to better reflect their roles in the hybrid algorithm.

tomalin · 2021-02-19T18:15:31Z

I favour the name change. I think the TrackFitMemory name is particularly confusing.

If we decide to go ahead with it, (i.e. if it is not too much work), I assume we'll have to make the same name change in the HLS code too). And yes the updated data files would go in https://github.com/cms-data/L1Trigger-TrackTracklet.git . (We could perhaps put both old & new names there for a while, to minimise problems). We'd also have to put the name change in both python & C++ wiring scripts.

The name change can be done as a separate PR, so I am approving this one.

tomalin

I've checked the code quality. And run the code and verified that we get output files.

* Various changes for outputting PurgeDuplicates test-vectors. * Removed some magic numbers. * Updated trackfitstr() to be more readable and include more comments. * Removed more magic.

…ms-patatrack#106) Can be included with the following snippet in the configuration: from RecoPixelVertexing.Configuration.customizePixelTracksForProfiling import customizePixelTracksForProfiling process = customizePixelTracksForProfiling(process) Removes validation, DQM, and output modules. As suggested in #70 (comment), an `AsciiOutputModule` is used to require the `pixelTracks`.

* Various changes for outputting PurgeDuplicates test-vectors. * Removed some magic numbers. * Updated trackfitstr() to be more readable and include more comments. * Removed more magic.

aehart added 3 commits February 12, 2021 12:13

Various changes for outputting PurgeDuplicates test-vectors.

aea9e72

Removed some magic numbers.

97c6d79

Updated trackfitstr() to be more readable and include more comments.

db74ea9

aehart requested review from tomalin and skinnari February 12, 2021 17:32

skinnari reviewed Feb 12, 2021

View reviewed changes

Removed more magic.

8654097

tomalin approved these changes Feb 19, 2021

View reviewed changes

aehart merged commit 1606921 into cms-L1TK:L1TK-dev-11_3_0_pre3 Feb 21, 2021

aehart deleted the purge_duplicates_test_vectors branch February 21, 2021 23:17

aehart mentioned this pull request Mar 10, 2021

L1L2 TrackBuilder + FullMatchMemory fix cms-L1TK/firmware-hls#134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PurgeDuplicates test-vectors (11_3_X) #70

PurgeDuplicates test-vectors (11_3_X) #70

aehart commented Feb 12, 2021 •

edited by tomalin

skinnari Feb 12, 2021

skinnari Feb 12, 2021

aehart Feb 12, 2021

skinnari Feb 12, 2021

aehart Feb 12, 2021

skinnari Feb 12, 2021

aehart Feb 12, 2021

skinnari Feb 12, 2021

aehart Feb 12, 2021

skinnari commented Feb 19, 2021

tomalin commented Feb 19, 2021

aehart commented Feb 19, 2021

tomalin commented Feb 19, 2021

aryd commented Feb 19, 2021 via email

aehart commented Feb 19, 2021

tomalin commented Feb 19, 2021

tomalin left a comment

		}
		const FPGAWord tmp(trackIndex_, 7, true, __LINE__, __FILE__);

PurgeDuplicates test-vectors (11_3_X) #70

PurgeDuplicates test-vectors (11_3_X) #70

Conversation

aehart commented Feb 12, 2021 • edited by tomalin

PR description:

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 Feb 19, 2021

tomalin commented Feb 19, 2021

aehart commented Feb 19, 2021

tomalin commented Feb 19, 2021

aryd commented Feb 19, 2021 via email

aehart commented Feb 19, 2021

tomalin commented Feb 19, 2021

tomalin left a comment

Choose a reason for hiding this comment

aehart commented Feb 12, 2021 •

edited by tomalin