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

Move CSC trigger and GEM-CSC trigger lookup tables into eventsetup objects (CCLUT-17) #34779

Merged
merged 1 commit into from
Sep 3, 2021
Merged

Move CSC trigger and GEM-CSC trigger lookup tables into eventsetup objects (CCLUT-17) #34779

merged 1 commit into from
Sep 3, 2021

Conversation

dildick
Copy link
Contributor

@dildick dildick commented Aug 4, 2021

PR description:

Move CSC trigger and GEM-CSC trigger lookup tables into eventsetup objects. This should prevent cmsRun from spending too much time loading lookup tables for the pattern finding logic. This PR requires cms-data/L1Trigger-CSCTriggerPrimitives#11

In response to #34633

@Dr15Jones @makortel Supersedes #34693.

  • The CSC trigger is being upgraded for Phase-2 displaced muon triggering with (1) CCLUT that provides a more precise position and incident angle of the L1 CSC stub, and (2) GEM-CSC integrated local trigger to recover L1 CSC stub efficiency, improve ME1/1 and ME2/1 bending angle, and reduce muon trigger rate. The algorithms rely on lookup tables which are loaded into the FPGAs in the peripheral crates.
  • Existing lookup tables, which are in text files in cms-data/L1Trigger-CSCTriggerPrimitives, are loaded through a new reader called CSCL1TPLookupTableEP which is an ESProducer.
  • CSCL1TPLookupTableEP produces 3 types of ES data: (a) CSCL1TPLookupTableCCLUT, (b) CSCL1TPLookupTableME11ILT, and (c)CSCL1TPLookupTableME21ILT. (a) is for upgrade (1). (b) and (c) are for upgrade (2).
  • (b) and (c) convert GEM system coordinates to CSC coordinates in order to match GEM clusters with L1 CSC stubs. They also contain utilities to allow for purer GEM-CSC matches (i.e. more likely to be the correct match in MC truth) based on position and slope from CSCL1TPLookupTableCCLUT.
  • The ES data objects are requested in the CSC trigger plugin and propagate to specific components that actually use them. For instance, CSCL1TPLookupTableCCLUT follows CSCTriggerPrimitivesProducer->CSCTriggerPrimitivesBuilder->CSCMotherBoard->CSCCathodeLCTProcessor->ComparatorCodeLUT.

Proposal was presented at https://indico.cern.ch/event/1065089/contributions/4478403/attachments/2292436/3897925/GEM_CSC_Trigger_AlcaDB_20210808.pdf

PR validation:

Tested the code so far on /RelValSingleMuPt10/CMSSW_11_3_0-113X_mcRun4_realistic_v7_2026D76noPU-v1/GEN-SIM-DIGI-RAW. Efficiency plots for ME1/1 and ME2/1 are the same as in #34582.

if this PR is a backport please specify the original PR and why you need to backport that PR:

N.A.

Before submitting your pull requests, make sure you followed this checklist:

@tahuang1991

@dildick
Copy link
Contributor Author

dildick commented Aug 4, 2021

FYI @rathjd

@dildick
Copy link
Contributor Author

dildick commented Aug 4, 2021

I see the exact same 2 validation plots as in #34582. So the LUTs are looked and used alright.

Screen Shot 2021-08-03 at 3 43 06 PM

Screen Shot 2021-08-04 at 11 34 40 AM

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 4, 2021

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34779/24446

  • This PR adds an extra 476KB to repository

  • Found files with invalid states:

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

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@dildick
Copy link
Contributor Author

dildick commented Aug 4, 2021

Originally I had the luts pass by reference in CalibMuon/CSCCalibration/plugins/CSCL1TPLookupTableEP.cc. @Dr15Jones suggest to change to pass by value + std::move. code-checks is suggesting to remove the std::move, which would make it pass by value only. Not sure which recommendation to follow.

pset_.getParameter<std::vector<std::string>>("esDiffToSlopeME1bFiles");

// read the text files and extract the data
const auto& GEM_pad_CSC_hs_ME1a_even_ = load(padToHsME1aFiles_[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Change the type from const auto& to just auto. This will allow the std::move to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good!

@tvami
Copy link
Contributor

tvami commented Aug 4, 2021

Hi @dildick while we are still at the code checks part, can you please squash some of the commits (bugfix, change name, several code checks)? Thanks!

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 4, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34779/24450

  • This PR adds an extra 476KB to repository

  • Found files with invalid states:

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 4, 2021

A new Pull Request was created by @dildick (Sven Dildick) for master.

It involves the following packages:

  • CalibMuon/CSCCalibration (alca)
  • CondFormats/CSCObjects (db, alca)
  • CondFormats/DataRecord (db, alca)
  • DQM/L1TMonitor (dqm)
  • L1Trigger/CSCTriggerPrimitives (l1)
  • L1Trigger/Configuration (l1)
  • L1Trigger/L1TMuon (l1)

@malbouis, @andrius-k, @yuanchao, @kmaeshima, @ErnestaP, @ahmad3213, @rvenditti, @cmsbuild, @rekovic, @jfernan2, @tlampen, @ggovi, @pohsun, @francescobrivio, @cecilecaillol, @tvami can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @tocheng, @valuev, @ptcox, @thomreis, @dinyar, @mmusich, @seemasharmafnal this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy, @perrotta you are the release manager for this.

cms-bot commands are listed here

@dildick
Copy link
Contributor Author

dildick commented Aug 4, 2021

@tvami Done. It can all go into a single commit.

@tvami
Copy link
Contributor

tvami commented Aug 4, 2021

@dildick great thanks! Another thing that when a new record is introduced, we usually like if there is a presentation about it at the AlCaDB meeting. Would you be available to present at the next meeting (Monday, 16:00)?

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 4, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34779/24453

  • This PR adds an extra 128KB to repository

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

@qliphy
Copy link
Contributor

qliphy commented Aug 31, 2021

please test
(just to re-test to make sure..)

@dildick
Copy link
Contributor Author

dildick commented Aug 31, 2021

@qliphy Are the tests stuck?

@tvami
Copy link
Contributor

tvami commented Aug 31, 2021

@cmsbuild , ping

@tvami
Copy link
Contributor

tvami commented Aug 31, 2021

@cmsbuild , please test

@dildick
Copy link
Contributor Author

dildick commented Sep 1, 2021

ping @tvami. Is the bot stuck again on the same test?

@perrotta
Copy link
Contributor

perrotta commented Sep 1, 2021

@smuzaffar : tests look stuck ("Not yet started" since several hours) in several PR. Is there anyfhing that can be done to start them?

@smuzaffar
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 1, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-297c50/18195/summary.html
COMMIT: 7787a9b
CMSSW: CMSSW_12_1_X_2021-08-31-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34779/18195/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: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3000404
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3000376
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

jfernan2 commented Sep 2, 2021

+1

@dildick
Copy link
Contributor Author

dildick commented Sep 2, 2021

@qliphy Can this PR be signed off? Thanks!

@qliphy
Copy link
Contributor

qliphy commented Sep 3, 2021

+1

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.