Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Phase 2 MET cosLUT fixes to match firmware #43021

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

NJManganelli
Copy link
Contributor

PR description:

This PR forward-ports two extra changes to the TrackMET, one of them removing a bug previously retained to match firmware behavior (explicitly wrong setting of a LUT value in the cos function), and the other removing an off-by-one bug in the encoding of the MET word for the APx and EMP test vectors.

This is a folow-up to:
#42553

This is a partial forward-port of the following PR (which back-ported the above PR to the cms-l1t-offline phase2 integration branch, then added the 3 commits being forward-ported here):
cms-l1t-offline#1160

The new changes correspond to the following bug fix in GTT's LibHLS repo:
https://gitlab.cern.ch/GTT/LibHLS/-/merge_requests/31
(GTT members: see the Issue related to the above merge request for more details on why this change was necessary and deferred)

This PR slightly changes the Px and Py lookups for tracks going into the TrackMET calculation, for about 0.1% of tracks, thus the differences will be small (and now physically correct)

PR validation:

This PR passes
scram b
scram b code-checks
scram b code-format

and it is used to successfully generate GTT test vectors (the only place the codecs_etsums play a part).

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43021/37193

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @NJManganelli (Nick) for master.

It involves the following packages:

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

@srimanob, @cmsbuild, @epalencia, @AdrianoDee, @aloeliger can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@srimanob
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-84e2d1/35194/summary.html
COMMIT: 9d2bf42
CMSSW: CMSSW_13_3_X_2023-10-15-0000/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43021/35194/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: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3356920
  • DQMHistoTests: Total failures: 9
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3356889
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 214 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@aloeliger
Copy link
Contributor

+l1

@srimanob
Copy link
Contributor

Hi @NJManganelli @aloeliger

Do I understand correctly that we don't see any fail comparison because we don't have L1T DQM for Phase-2? Thx.

@NJManganelli
Copy link
Contributor Author

Hi @NJManganelli @aloeliger

Do I understand correctly that we don't see any fail comparison because we don't have L1T DQM for Phase-2? Thx.

Hi @srimanob, I think there is little (maybe no?) testing of GTT L1T. This issue in particular is very specific and tied to such a low level detail that I'm unsure typical DQM style plots would even capture it (i.e. seeing it in the bulk of a GTT Object's distribution would be challenging, it might need the same bit-level checks object-by-object that we're doing against the firmware). I think @aloeliger raised a point of needing better testing of L1T in general, at the Athens workshop, if that's where your thoughts are heading. If I misunderstood the question, let me know

Cheers,
Nick

@aloeliger
Copy link
Contributor

aloeliger commented Oct 15, 2023

@srimanob as far as I am aware, phase 2 DQM of L1T hasn't hit the drawing board yet. I'm not yet involved in those efforts though.

Nick is right though, in that I would like to be behind more automated testing of L1T in CMSSW

@srimanob
Copy link
Contributor

+Upgrade

We don't expect to see failure in comparison. The failure in Run-3 wf is not related to this PR.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 21b9b4f into cms-sw:master Oct 16, 2023
11 checks passed
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