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

MC/MP LUTs #176

Closed bryates closed 2 years ago

bryates commented 2 years ago

PR description:

This PR additional LUTs required for the MC/MP disk HLS implementation in PR #242. alpha inner/outer - LUT for disk 2S detector corrections rSS inner/outer - LUT for disk 2S r values.

PR validation:

With this PR we see full agreement in all MC disks (modulo some debugging).

tomalin commented 2 years ago

Please explain the the Description of the PR what the new LUT tables are. And also which PR to the HLS repo causes them to be read in by the HLS code.

bryates commented 2 years ago

Please explain the the Description of the PR what the new LUT tables are. And also which PR to the HLS repo causes them to be read in by the HLS code.

Done.

tomalin commented 2 years ago

Would you mind adding: 1) A comment to the top of TrackletLUT.h, explaining what this class is for. 2) I believe that each type in enum TrackletLUT::VMRTableType represents a different LUT? If so, could you add a comment to the declaration of this enum briefly indicating the meaning of each LUT type?

bryates commented 2 years ago

Would you mind adding:

  1. A comment to the top of TrackletLUT.h, explaining what this class is for.
  2. I believe that each type in enum TrackletLUT::VMRTableType represents a different LUT? If so, could you add a comment to the declaration of this enum briefly indicating the meaning of each LUT type?

I've added some comments.

bryates commented 2 years ago

Looks fine to me, except that @bryates needs to run "scram b -j8 code-format" to avoid the PR failing CI tests.

@aryd , do you have any objections to us merging this?

Just in case it was missed, I've pushed the code format changes. I think we're just waiting in any feedback from @aryd before merging.