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.

Sign up for GitHub

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

Jump to bottom

[L1T] Phase-2, track MET update for FW sync (new) #39836

Merged
merged 11 commits into from
Nov 3, 2022
Merged

[L1T] Phase-2, track MET update for FW sync (new) #39836

merged 11 commits into from
Nov 3, 2022

Conversation

cecilecaillol
Copy link
Contributor

PR description:

This PR updates the Track MET calculation in order to match to FW and simplifies code. Changes include:

Removal of track transformation step, using instead the L1GTTInputProducer and L1TrackSelectionProducer
Use AP_FIXED data types for internal Et calculation, simplifying code and avoiding unnecessary conversions
Use TTTrack_word phi granularity extended to 2 Pi instead of internal representation
Rewrite Cordic function to use ap_fixed removing extra conversions
Cordic now runs between -pi and pi rather than 0 and 2 pi as per output specification

PR validation:

Tested against FW, currently matches px and py calcuations as well as MET and MET phi. Differences between Emulation and Simulation are ongoing but going forwards with emulator and FW matching these differences can be investigated while also updating the FW.

Supersedes #39725, which got conflicts after merging #39422

Porting local cms-l1t-offline#1049

Chriisbrown and others added 10 commits October 24, 2022 19:18
@Chriisbrown
Update Track MHT and MET output
96b15a8 
Alexx's comments #1

Alexx's comments 2 + bug fix to debug printout

Alexx's Comments 3

(cherry picked from commit 5dbf6d2)
@Chriisbrown
initial MET update
76696e9 
(cherry picked from commit 4ac054f)
@Chriisbrown
Revert phi range to interface document
ad973f5 
(cherry picked from commit 1923329)
Changes which make the sum px and sum py match the firmware. There ar…
5feb3a8 
…e still some differences in the final MET and MET phi values.

(cherry picked from commit f28d65b)
@Chriisbrown
Fix to phi output
d9c2c80 
(cherry picked from commit 38ab830)
Re-write the CORDIC function to match the one used by the firmware.
867a309 
(cherry picked from commit b3e920e)
Fix a bug and update some log messages.
9ca054b 
(cherry picked from commit bb79c4b)
Fix some inconsistencies in the MET and MET phi computation between t…
4595ebf 
…he emulation and the firmware that were causing a mismatch.

(cherry picked from commit 1b9aa85)
Changes after the code format checks.
5db559a 
(cherry picked from commit db7adce)
remove L1TNtuples
1447421
@cmsbuild cmsbuild added this to the CMSSW_12_6_X milestone Oct 24, 2022
@cecilecaillol cecilecaillol mentioned this pull request Oct 24, 2022
Closed
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39836/32716

  • This PR adds an extra 200KB to repository

  • Found files with invalid states:

    • L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @cecilecaillol for master.

It involves the following packages:

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

@rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @beaucero, @trtomei this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@cecilecaillol
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4bcae7/28478/summary.html
COMMIT: 1447421
CMSSW: CMSSW_12_6_X_2022-10-24-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39836/28478/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • Reco comparison had 6 failed jobs
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3378384
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3378359
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 201 log files, 48 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@cecilecaillol
Copy link
Contributor Author

+l1

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39836/32888

  • This PR adds an extra 104KB to repository

  • Found files with invalid states:

    • L1Trigger/L1TNtuples/src/L1AnalysisPhaseIIStep1.cc:

@cecilecaillol
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2022

Pull request #39836 was updated. @epalencia, @AdrianoDee, @srimanob, @cecilecaillol, @rekovic can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4bcae7/28740/summary.html
COMMIT: 9fad9f0
CMSSW: CMSSW_12_6_X_2022-11-02-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39836/28740/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3416398
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3416376
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 206 log files, 48 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@AdrianoDee
Copy link
Contributor

+upgrade

@cecilecaillol
Copy link
Contributor Author

+l1

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2022

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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
Copy link
Contributor

+1

  • Concerns seem to be addressed.

@cmsbuild cmsbuild merged commit e01b0cd into cms-sw:master Nov 3, 2022
@cmsbuild cmsbuild mentioned this pull request Nov 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AdrianoDee AdrianoDee AdrianoDee left review comments

@Chriisbrown Chriisbrown Chriisbrown left review comments

Assignees
No one assigned
Labels
code-checks-approved fully-signed l1-approved orp-approved tests-approved upgrade-approved
Projects
None yet
Milestone
CMSSW_12_6_X
Development

Successfully merging this pull request may close these issues.
5 participants
@cecilecaillol @cmsbuild @AdrianoDee @rappoccio @Chriisbrown