Skip to content

W3Pi Emulation and Test Vectors - #49003

Open
Jared-Fraticelli wants to merge 12 commits into
cms-sw:masterfrom
Jared-Fraticelli:w3pi_pr
Open

W3Pi Emulation and Test Vectors#49003
Jared-Fraticelli wants to merge 12 commits into
cms-sw:masterfrom
Jared-Fraticelli:w3pi_pr

Conversation

@Jared-Fraticelli

Copy link
Copy Markdown

PR description:

The TkTriplet word is updated to contain a valid bit, triplet pT, eta, and phi fields (which are currently set to 0), a triplet invariant mass field, the three triplet track pTs, and an unassigned field. The new word spans 97 bits, and is padded with zeroes to span 128 bits (evenly spanning two 64-bit link data words). The tktriplet emulation plugin finds the three highest pT tracks in an event, computes the invariant mass from them, and fills the output word with the aforementioned nonzero fields.

https://indico.cern.ch/event/1588031/

  • See the above indico page for a talk regarding the W->3pi updates

PR validation:

Tests have been done to ensure that the output tktriplet word is correctly written in APX board format. The word currently spans the first two 64-bit link words of each event on link 2, and tests using HLS firmware have shown that the invariant mass and triplet track pT fields can be correctly decoded from the test vectors.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

@cmsbuild

cmsbuild commented Sep 26, 2025

Copy link
Copy Markdown
Contributor

cms-bot internal usage

@cmsbuild

Copy link
Copy Markdown
Contributor

@cmsbuild

Copy link
Copy Markdown
Contributor

A new Pull Request was created by @Jared-Fraticelli for master.

It involves the following packages:

  • DataFormats/L1Trigger (l1)
  • L1Trigger/DemonstratorTools (l1)
  • L1Trigger/L1TTrackMatch (upgrade, l1)

@BenjaminRS, @Moanwar, @cmsbuild, @quinnanm, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @dinyar, @eyigitba, @missirol, @mmusich, @rovere, @thomreis this is something you requested to watch as well.
@ftenchini, @mandrenguyen, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@quinnanm

Copy link
Copy Markdown
Contributor

test parameters:

workflow = 29634.78
relvals_opt = --what upgrade

@quinnanm

Copy link
Copy Markdown
Contributor

please test

@cmsbuild

Copy link
Copy Markdown
Contributor

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e554af/48361/summary.html
COMMIT: 6027c5e
CMSSW: CMSSW_16_0_X_2025-09-29-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/49003/48361/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 1 lines to the logs
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 52
  • DQMHistoTests: Total histograms compared: 4058318
  • DQMHistoTests: Total failures: 23
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 4058275
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 51 files compared)
  • Checked 223 log files, 192 edm output root files, 52 DQM output files
  • TriggerResults: no differences found

process.l1tTrackTripletEmulation.pair1_dzMax = 10e7
process.l1tTrackTripletEmulation.pair2_dzMin = -1
process.l1tTrackTripletEmulation.pair2_dzMax = 10e7

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add some comments adding context to these numbers?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, are you referring to what the variables are or what the specific choice of numeric values here means

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the choice of numbers. but it's not vital

Comment on lines +347 to +350
l1ttripletemu::pxyz_t pz1 = (l1ttripletemu::pxyz_t)trk1.f_Pt * sinhLUT_[coshIndex1];
l1ttripletemu::pxyz_t pz2 = (l1ttripletemu::pxyz_t)trk2.f_Pt * sinhLUT_[coshIndex2];
l1ttripletemu::pxyz_t pz3 = (l1ttripletemu::pxyz_t)trk3.f_Pt * sinhLUT_[coshIndex3];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be sinhLUT_[sinhIndex1] instead of sinhLUT_[coshIndex1] ? May be a copy-paste bug

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be, that has been fixed now. At the moment, that doesn't affect the output since the LUTs are the same size and take in eta as a parameter. In the future, though, since the table sizes could be changed, this distinction is crucial.

//The following says we do not know what parameters are allowed
// Please change this to state exactly what you do use, even if it is no parameters
ParameterSetDescription desc;
desc.add<edm::InputTag>("L1TrackInputTag", edm::InputTag("l1tTTTracksFromTrackletEmulation", "Level1TTTracks"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment: did you check that this behaves as expected for both displaced_ == True and False?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did not test setting displaced_ to true, but at this point in development, the algorithm should do the same thing for both the prompt and extended track collections. So we have removed this parameter.

for (unsigned int LUT_idx = 0; LUT_idx < l1ttripletemu::kCosLUTBins; LUT_idx++) {
cosLUT.push_back((cos_lut_fixed_t)(cos(phi)));
phi += stepPhi;
//std::cout << LUT_idx << "," << (cos_lut_fixed_t)(cos(phi)) << std::endl;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove commented printouts

@cmsbuild

cmsbuild commented Nov 4, 2025

Copy link
Copy Markdown
Contributor

@cmsbuild

cmsbuild commented Nov 4, 2025

Copy link
Copy Markdown
Contributor

Pull request #49003 was updated. @BenjaminRS, @Moanwar, @cmsbuild, @quinnanm, @subirsarkar can you please check and sign again.

@Jared-Fraticelli

Copy link
Copy Markdown
Author

The new push has updated files implementing the comments from above.

@BenjaminRS

Copy link
Copy Markdown
Contributor

please test

@cmsbuild cmsbuild modified the milestones: CMSSW_16_1_X, CMSSW_17_0_X Apr 4, 2026
@Jared-Fraticelli

Copy link
Copy Markdown
Author

Hi, I just wanted to check in and see if there is anything else that should be done on my end for this PR. Thank you.

@BenjaminRS

Copy link
Copy Markdown
Contributor

+l1
Hi Jared - sorry for the long delay in getting back to this PR; thanks for making the changes following the previous round of comments

@aehart

aehart commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

+l1 Hi Jared - sorry for the long delay in getting back to this PR; thanks for making the changes following the previous round of comments

@BenjaminRS We were wondering if there had been any progress on the issues reported by the menu group related to this PR? Is there anything we can do on our side to help get it merged?

@cmsbuild

cmsbuild commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Milestone for this pull request has been moved to CMSSW_20_0_X. Please open a backport if it should also go in to CMSSW_17_0_X.

@makortel

makortel commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Just checking, is this PR intended for Phase 2 (20_1_X) or Run 3 legacy (17_0_X) or both?

@aehart

aehart commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Just checking, is this PR intended for Phase 2 (20_1_X) or Run 3 legacy (17_0_X) or both?

This is only intended for Phase 2.

@cmsbuild

cmsbuild commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Milestone for this pull request has been moved to CMSSW_20_1_X. Please open a backport if it should also go in to CMSSW_20_0_X.

@cmsbuild

Copy link
Copy Markdown
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49003/49734

ERROR: Build errors found during clang-tidy run.

      |                                                                          ^~~~~~~~~~~~
      |                                                                          kMassSize
--
src/DataFormats/L1Trigger/interface/TkTripletWord.h:97:78: error: no member named 'kMassMagSize' in 'l1t::io_v1::TkTripletWord::TkTripletBitWidths'; did you mean 'kMassSize'? [clang-diagnostic-error]
   97 |     typedef ap_ufixed<TkTripletBitWidths::kMassSize + 7, TkTripletBitWidths::kMassMagSize + 7, AP_RND_CONV, AP_SAT>
      |                                                                              ^~~~~~~~~~~~
      |                                                                              kMassSize
--
src/DataFormats/L1Trigger/interface/TkTripletWord.h:99:43: error: no member named 'kTrk1PtSize' in 'l1t::io_v1::TkTripletWord::TkTripletBitWidths' [clang-diagnostic-error]
   99 |     typedef ap_ufixed<TkTripletBitWidths::kTrk1PtSize, TkTripletBitWidths::kTrk1PtMagSize, AP_RND_CONV, AP_SAT>
      |                                           ^~~~~~~~~~~
src/DataFormats/L1Trigger/interface/TkTripletWord.h:164:10: error: member reference base type 'tktriplet_trk_pt_t' (aka 'int') is not a structure or union [clang-diagnostic-error]
  164 |       ret.V = tkTripletWord()(TkTripletBitLocations::kTrk1PtMSB, TkTripletBitLocations::kTrk1PtLSB);
      |       ~~~^~
src/DataFormats/L1Trigger/interface/TkTripletWord.h:170:10: error: member reference base type 'tktriplet_trk_pt_t' (aka 'int') is not a structure or union [clang-diagnostic-error]
  170 |       ret.V = tkTripletWord()(TkTripletBitLocations::kTrk2PtMSB, TkTripletBitLocations::kTrk2PtLSB);
      |       ~~~^~
src/DataFormats/L1Trigger/interface/TkTripletWord.h:176:10: error: member reference base type 'tktriplet_trk_pt_t' (aka 'int') is not a structure or union [clang-diagnostic-error]
  176 |       ret.V = tkTripletWord()(TkTripletBitLocations::kTrk3PtMSB, TkTripletBitLocations::kTrk3PtLSB);
      |       ~~~^~
Suppressed 1200 warnings (1196 in non-user code, 4 NOLINT).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

@cmsbuild

Copy link
Copy Markdown
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49003/49742

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild

Copy link
Copy Markdown
Contributor

Pull request #49003 was updated. @BenjaminRS, @cmsbuild, @quinnanm can you please check and sign again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants