adding MultiplicityValueProducers (HLT Phase-2 TDR) #33204

missirol · 2021-03-17T16:20:40Z

PR description:

During tests on the HLT Phase-2 menu (which is being integrated in #32903), @SWuchterl (BTV) noticed that one plugin was still coming from a user-pkg (so, not from CMSSW directly). This PR aims to integrate this in the release.

The plugin itself is rather simple: it's a (templated) producer that adds to the event the multiplicity value of a given EDM collection.

It is used in the current Phase-2 HLT to get the multiplicity of pixel clusters; that value, in turn, is used as PU-proxy input to the PUPPI producer in Phase-2 HLT studies (see #32341 for the relevant update to PUPPI).

At the moment, the only application of this is the HLT Phase-2 menu in the context of Jets/MET reconstruction, so I'm tentatively adding this plugin to HLTrigger/JetMET.

Attn: @fwyzard @trtomei

PS. If experts know of an existing CMSSW producer that already serves this same purpose, that would simplify things (and make this PR unnecessary).

PR validation:

Private tests in the context of the HLT Phase-2 TDR (i.e. in 11_1_X).

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

Not a backport.

Will need to be backported to 11_1_X (release for the HLT Phase-2 TDR).

cmsbuild · 2021-03-17T16:26:30Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33204/21634

This PR adds an extra 12KB to repository

cmsbuild · 2021-03-17T16:27:11Z

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

HLTrigger/JetMET

@Martin-Grunewald, @cmsbuild, @fwyzard can you please review it and eventually sign? Thanks.
@Martin-Grunewald 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

fwyzard · 2021-03-17T16:32:48Z

thanks @missirol

looking at the code

the output type used in the templates is double, but an integer type should still be fine; are you using double for easier compatibility with other producers ?
MultiplicityValueProducerFromNestedCollection should go in a separate file
both MultiplicityValueProducer and MultiplicityValueProducerFromNestedCollection look like they could be edm::global rather than edm::stream producers

missirol · 2021-03-17T17:15:13Z

Hi Andrea,

the output type used in the templates is double, but an integer type should still be fine; are you using double for easier compatibility with other producers ?

Yes, that's it. In #32341, I had to choose one type for the value in input to Puppi, and I stuck with double (maybe I could have used float, but the PUPPI producer mostly uses doubles internally).

MultiplicityValueProducerFromNestedCollection should go in a separate file

Okay, I'll separate the two. I realized now that in these changes (extracted from a user-pkg) the simpler plugin MultiplicityValueProducer is never instantiated, so it could be removed altogether (if it should, let me know).

both MultiplicityValueProducer and MultiplicityValueProducerFromNestedCollection look like they could be edm::global rather than edm::stream producers.

Okay, I'll test this. If possible, could you expand just a bit on how you assess this, and what is preferred? I had a look here, but my understanding of this is limited.

fwyzard · 2021-03-17T17:22:59Z

both MultiplicityValueProducer and MultiplicityValueProducerFromNestedCollection look like they could be edm::global rather than edm::stream producers.

Okay, I'll test this. If possible, could you expand just a bit on how you assess this, and what is preferred? I had a look here, but my understanding of this is limited.

The reason to prefer a global module to a stream module is a (small) reduction in memory usage: with a stream module there are N instances in a job (one per stream), while with a global module there is always only one instance.

The main distinction is that - without using extensions - a global module is not allowed to alter its data members during the event processing (effectively, its produce() method is const).
So, if a module does not have any non-const data members (or non-const data members that are not modified after the constructor), it is a good candidate to be global.
Instead, if a global module needs to use a edm::StreamCache<T>, it might be better of being a stream module.

fwyzard · 2021-03-17T17:24:04Z

allow @missirol test rights

fwyzard

This should be all the required changes for making the modules global

HLTrigger/JetMET/plugins/MultiplicityValueProducers.h

Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>

fwyzard

Can you add the usual macros to protect from multiple inclusion ?

HLTrigger/JetMET/plugins/MultiplicityValueProducers.h

Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>

cmsbuild · 2021-03-17T17:45:57Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33204/21636

ERROR: Build errors found during clang-tidy run.

HLTrigger/JetMET/plugins/MultiplicityValueProducers.h:73:62: error: no template named 'EDProducer' in namespace 'edm::stream'; did you mean 'edm::global::EDProducer'? [clang-diagnostic-error]
class MultiplicityValueProducerFromNestedCollection : public edm::stream::EDProducer<> {
                                                             ^~~~~~~~~~~~~~~~~~~~~~~
                                                             edm::global::EDProducer
--
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 · 2021-03-17T18:11:00Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33204/21637

This PR adds an extra 16KB to repository
Found files with invalid states:
- HLTrigger/JetMET/plugins/MultiplicityValueProducers.h:
  - Added: 43459a5
  - Modified: 1f1038b, e62cd8b
  - Deleted: 4be38a4
- HLTrigger/JetMET/plugins/MultiplicityValueProducers.cc:
  - Added: 43459a5
  - Deleted: 4be38a4

cmsbuild · 2021-03-18T08:17:14Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33204/21643

This PR adds an extra 20KB to repository
Found files with invalid states:
- HLTrigger/JetMET/plugins/MultiplicityValueProducerFromNestedCollection.cc:
  - Added: 4be38a4
  - Deleted: 6f15bd7
- HLTrigger/JetMET/plugins/MultiplicityValueProducerFromNestedCollection.h:
  - Added: 4be38a4
  - Modified: c24657f, 35d06bb, d613da7
  - Deleted: 6f15bd7
- HLTrigger/JetMET/plugins/MultiplicityValueProducers.h:
  - Added: 43459a5
  - Modified: 1f1038b, e62cd8b
  - Deleted: 4be38a4
- HLTrigger/JetMET/plugins/MultiplicityValueProducer.h:
  - Added: 4be38a4
  - Modified: c24657f, 35d06bb, d613da7
  - Deleted: 6f15bd7
- HLTrigger/JetMET/plugins/MultiplicityValueProducers.cc:
  - Added: 43459a5
  - Deleted: 4be38a4

cmsbuild · 2021-03-18T08:17:39Z

Pull request #33204 was updated. @Martin-Grunewald, @cmsbuild, @fwyzard can you please check and sign again.

fwyzard · 2021-03-18T08:23:21Z

Sorry, but in both PRs you need to provide an example/default VALUE for the parameters of the two plugins, ie, ConfDb parsing needs type, name and a value for each!

@Martin-Grunewald sorry about that :-(
Being for the Phase 2 menu I did not think about ConfDB - thanks for catching this, and thanks @missirol for the fixes.

Martin-Grunewald · 2021-03-18T09:35:56Z

please test

cmsbuild · 2021-03-18T12:11:17Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7f6f8b/13587/summary.html
COMMIT: 6f15bd7
CMSSW: CMSSW_11_3_X_2021-03-17-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33204/13587/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: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2639935
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2639912
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

Martin-Grunewald · 2021-03-18T13:45:53Z

+1

Thanks, looks good know. Please also update the backport PR!

cmsbuild · 2021-03-18T13:46:14Z

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

silviodonato · 2021-03-18T14:05:47Z

+1
fyi @cms-sw/reconstruction-l2

added MultiplicityValueProducer(s) to HLTrigger/JetMET

43459a5

cmsbuild added this to the CMSSW_11_3_X milestone Mar 17, 2021

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures tests-pending labels Mar 17, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 17, 2021

cmsbuild added the allow-missirol label Mar 17, 2021

fwyzard reviewed Mar 17, 2021

View reviewed changes

convert MultiplicityValueProducer* from stream to global

1f1038b

Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>

cmsbuild added code-checks-pending and removed code-checks-approved labels Mar 17, 2021

fwyzard reviewed Mar 17, 2021

View reviewed changes

HLTrigger/JetMET/plugins/MultiplicityValueProducers.h Outdated Show resolved Hide resolved

HLTrigger/JetMET/plugins/MultiplicityValueProducers.h Outdated Show resolved Hide resolved

added include guards to MultiplicityValueProducer*

e62cd8b

Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>

cmsbuild added code-checks-rejected and removed code-checks-pending labels Mar 17, 2021

split MultiplicityValueProducer* in separate headers

4be38a4

cmsbuild added code-checks-pending and removed code-checks-rejected labels Mar 17, 2021

cmsbuild removed the code-checks-pending label Mar 17, 2021

cmsbuild added code-checks-pending hlt-pending tests-pending and removed code-checks-approved hlt-rejected tests-approved labels Mar 18, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 18, 2021

cmsbuild added tests-started and removed tests-pending labels Mar 18, 2021

cmsbuild added tests-approved and removed tests-started labels Mar 18, 2021

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Mar 18, 2021

cmsbuild added orp-approved and removed orp-pending labels Mar 18, 2021

cmsbuild merged commit 339a37b into cms-sw:master Mar 18, 2021

missirol mentioned this pull request Oct 29, 2021

Producer and filter for jet timing trigger #35724

Merged

missirol mentioned this pull request Mar 2, 2022

Add PFDiJetCorrCheckerWithDiTau producer for DoubleTau+VBF-jets HLT paths #37116

Merged

missirol deleted the devel_1130pre4_multiplicityValueProducers branch March 27, 2022 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding MultiplicityValueProducers (HLT Phase-2 TDR) #33204

adding MultiplicityValueProducers (HLT Phase-2 TDR) #33204

missirol commented Mar 17, 2021

cmsbuild commented Mar 17, 2021

cmsbuild commented Mar 17, 2021

fwyzard commented Mar 17, 2021

missirol commented Mar 17, 2021

fwyzard commented Mar 17, 2021

fwyzard commented Mar 17, 2021

fwyzard left a comment

fwyzard left a comment

cmsbuild commented Mar 17, 2021

cmsbuild commented Mar 17, 2021

cmsbuild commented Mar 18, 2021

cmsbuild commented Mar 18, 2021

fwyzard commented Mar 18, 2021

Martin-Grunewald commented Mar 18, 2021

cmsbuild commented Mar 18, 2021

Martin-Grunewald commented Mar 18, 2021

cmsbuild commented Mar 18, 2021

silviodonato commented Mar 18, 2021

adding MultiplicityValueProducers (HLT Phase-2 TDR) #33204

adding MultiplicityValueProducers (HLT Phase-2 TDR) #33204

Conversation

missirol commented Mar 17, 2021

PR description:

PR validation:

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

cmsbuild commented Mar 17, 2021

cmsbuild commented Mar 17, 2021

fwyzard commented Mar 17, 2021

missirol commented Mar 17, 2021

fwyzard commented Mar 17, 2021

fwyzard commented Mar 17, 2021

fwyzard left a comment

Choose a reason for hiding this comment

fwyzard left a comment

Choose a reason for hiding this comment

cmsbuild commented Mar 17, 2021

cmsbuild commented Mar 17, 2021

cmsbuild commented Mar 18, 2021

cmsbuild commented Mar 18, 2021

fwyzard commented Mar 18, 2021

Martin-Grunewald commented Mar 18, 2021

cmsbuild commented Mar 18, 2021

Comparison Summary

Martin-Grunewald commented Mar 18, 2021

cmsbuild commented Mar 18, 2021

silviodonato commented Mar 18, 2021