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: add HPS taus #33681

Merged
merged 9 commits into from May 13, 2021
Merged

L1T: add HPS taus #33681

merged 9 commits into from May 13, 2021

Conversation

cecilecaillol
Copy link
Contributor

@cecilecaillol cecilecaillol commented May 10, 2021 •
edited by silviodonato

PR description:

Implement HPS taus at L1T.
See #33681 (comment).

PR validation:

Rebase of cms-l1t-offline#911

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

Rebase of cms-l1t-offline#911

@cmsbuild cmsbuild added this to the CMSSW_12_0_X milestone May 10, 2021
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33681/22584

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

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33681/22586

ERROR: Build errors found during clang-tidy run.

L1Trigger/Phase2L1Taus/plugins/HPSPFTauProducer.cc:70:20: error: expected unqualified-id [clang-diagnostic-error]
    if (!vertices->->empty()) {
                   ^
Suppressed 1242 warnings (1241 in non-user code, 1 with check filters).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:128: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33681/22587

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

DataFormats/L1TParticleFlow
L1Trigger/Phase2L1Taus

The following packages do not have a category, yet:

L1Trigger/Phase2L1Taus
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@cmsbuild, @rekovic, @srimanob, @cecilecaillol, @kpedro88 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @rovere this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@cecilecaillol
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33681/22647

@cmsbuild
Copy link
Contributor

Pull request #33681 was updated. @cmsbuild, @rekovic, @srimanob, @cecilecaillol, @kpedro88 can you please check and sign again.

@cecilecaillol
Copy link
Contributor Author

please test

@cecilecaillol
Copy link
Contributor Author

+l1

@srimanob
Copy link
Contributor

Hi @sandeepbhowmik1
Thanks for the information. However, looking on the slide you posted, together with page 3 of
https://indico.cern.ch/event/1017753/contributions/4271145/attachments/2209533/3739186/Sandeep_Phase2_L1_TauTrigger_20210316_v1.pdf
I still don't get the conclusion what is the performance change. Actually, I don't see any mentions on PF->calo seed. Do I misunderstand anything?

@sandeepbhowmik1
Copy link
Contributor

sandeepbhowmik1 commented May 13, 2021 •
edited

We noticed that although we had added calojet in the code and it was running fine but there was no contribution from jet seeding. Then we found that the daughter information of caloJet is giving zero always.

After that we made workaround of that and saw the contribution due to jet seeding is almost negligible.

It increases only 1 GeV threshold (21 to 22 GeV) for Tight working point we used for HLT study and almost no effect in efficiency turn-on

We have attached that plot in presentation of HLT Upgrade meeting

you can have a look it here

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

https://indico.cern.ch/event/1030865/contributions/4328562/attachments/2235166/3788209/Sandeep_Phase2_L1andHLT_TauTrigger_20210427_v2.pdf

Slode 22

@srimanob
Copy link
Contributor

+Upgrade

It will be much better if the PR description mentions what is in it, and what to expect (change, adding something, technical). Since we don't have DQM for Phase-2 L1T and no description, it seems to be blind approval on Physics content. It will be difficult to track when we have an issue.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-638d81/15059/summary.html
COMMIT: 196fa70
CMSSW: CMSSW_12_0_X_2021-05-12-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33681/15059/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1264 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2648242
  • DQMHistoTests: Total failures: 3675
  • DQMHistoTests: Total nulls: 19
  • DQMHistoTests: Total successes: 2644526
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 45.703 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 140.53 ): 44.531 KiB Hcal/DigiRunHarvesting
  • DQMHistoSizes: changed ( 140.53 ): 1.172 KiB RPC/DCSInfo
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@fwyzard
Copy link
Contributor

fwyzard commented May 13, 2021 via email

@srimanob
Copy link
Contributor

srimanob commented May 13, 2021 •
edited

Hi Andrea,
I mean PR description, not in the context of TDR. I would expect to see what PR is doing, on which result was presented? Am I wrong to ask for a better PR description?

@fwyzard
Copy link
Contributor

fwyzard commented May 13, 2021 via email

@silviodonato silviodonato merged commit e65c382 into cms-sw:master May 13, 2021
@silviodonato
Copy link
Contributor

merge

@silviodonato
Copy link
Contributor

@cecilecaillol I've added in the PR description a link to #33681 (comment) that point to the expected physics results.

@cecilecaillol cecilecaillol mentioned this pull request May 21, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srimanob srimanob srimanob left review comments

@sandeepbhowmik1 sandeepbhowmik1 sandeepbhowmik1 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_0_X
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@cecilecaillol @cmsbuild @kpedro88 @fwyzard @srimanob @sandeepbhowmik1 @silviodonato