Modernized some modules in RecoBTag #35018

Dr15Jones · 2021-08-25T18:12:47Z

PR description:

now uses esConsumes
consolidated .h files into .cc
added fillDescriptions
switched from ::stream to ::global modules
various code modernizations

PR validation:

Code compiles. Non of these changes are meant to change the behavior of the modules.

resolves cms-sw/framework-team#247

- made modules global - use new Event get syntax - added fillDescriptions

cmsbuild · 2021-08-25T18:19:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35018/24872

This PR adds an extra 28KB to repository

cmsbuild · 2021-08-25T18:19:39Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

RecoBTag/PixelCluster (reconstruction)
RecoBTag/SoftLepton (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@JyothsnaKomaragiri, @emilbols, @demuller, @andrzejnovak, @AlexDeMoor this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2021-08-25T18:20:22Z

please test

cmsbuild · 2021-08-25T22:32:11Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-afe11b/18034/summary.html
COMMIT: 5b1ca8e
CMSSW: CMSSW_12_1_X_2021-08-25-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35018/18034/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

@bsunanda Run3-gex90 Make unique material names as default for all Run3 scenarios #34983
@bsunanda Run3-alca198 Modify the analyzers to record muons not on event basis but on single muon basis #34980
@missirol Fixing some edge cases in HLT-Patatrack customisations #34992
@dildick New option in CSC DigiToRaw to pack strip digis by CFEB in each chamber. #34783
@Dr15Jones Use sufficient digits when converting a double #34999

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-afe11b/18034/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-afe11b/18034/git-merge-result

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000324
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-08-26T14:35:58Z

RecoBTag/SoftLepton/plugins/SoftPFElectronTagInfoProducer.cc

+
+  if (PVCollection->empty() and not theJetCollection.empty() and not theGEDGsfElectronCollection.empty()) {
+    //we would need to access a vertex from the collection but there isn't one.
+    return;


I expect an event.emplace should still happen

I was following the logic done on line '49-50' in the old code

cmssw/RecoBTag/SoftLepton/plugins/SoftPFElectronTagInfoProducer.cc

Lines 49 to 50 in fd2a236

if (!PVCollection.isValid())

return;

I can push an empty container if you'd prefer

Just for some context, I added that condition check because in the original code if that condition was meet the algorithm would dereference a nullptr for the vertex and pass it to a function expecting a const &.

slava77 · 2021-08-26T14:44:07Z

RecoBTag/SoftLepton/plugins/SoftPFMuonTagInfoProducer.cc


  // Declare and open Vertex collection
-  edm::Handle<reco::VertexCollection> theVertexCollection;
-  iEvent.getByToken(vertexToken, theVertexCollection);
+  edm::Handle<reco::VertexCollection> theVertexCollection = iEvent.getHandle(vertexToken);
  if (!theVertexCollection.isValid() || theVertexCollection->empty())
    return;


event put/emplace is missing here as well

again, that is the logic which was there before this change. I can change it if you wish.

again, that is the logic which was there before this change. I can change it if you wish.

since this PR has fairly extensive changes, it would be nice to include this one as well.
(The other producers in this PR apparently produce a product per ::produce call already)
Thank you.

I am making the change so the module always pushes a container. That being said, if one of these conditions fails it will now be pretty hard to find the problem as it will just appear to be OK. Previously, if this failed later modules trying to access it would likely throw an exception.

cmsbuild · 2021-08-26T16:13:19Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35018/24891

This PR adds an extra 28KB to repository

cmsbuild · 2021-08-26T16:13:45Z

Pull request #35018 was updated. @jpata, @cmsbuild, @slava77 can you please check and sign again.

Dr15Jones · 2021-08-26T16:14:26Z

please test

cmsbuild · 2021-08-26T19:15:57Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-afe11b/18064/summary.html
COMMIT: cb85cb1
CMSSW: CMSSW_12_1_X_2021-08-25-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35018/18064/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: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000330
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-08-26T23:22:43Z

+reconstruction

for #35018 cb85cb1

code changes are technical, in line with the PR description and the follow up review
jenkins tests pass and comparisons with the baseline show no differences

cmsbuild · 2021-08-26T23:23:05Z

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

perrotta · 2021-08-27T20:58:51Z

+1

Dr15Jones added 3 commits August 25, 2021 09:39

Use esConsumes in RecoBTag modules

59dc13b

Move contents of .h files into .cc files for plugins

6214d48

Modernized modules

5b1ca8e

- made modules global - use new Event get syntax - added fillDescriptions

cmsbuild added this to the CMSSW_12_1_X milestone Aug 25, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 25, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 25, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 25, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 25, 2021

slava77 reviewed Aug 26, 2021

View reviewed changes

Add empty container to Event if there was a problem.

cb85cb1

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Aug 26, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 26, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 26, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 26, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Aug 26, 2021

cmsbuild added orp-approved and removed orp-pending labels Aug 27, 2021

cmsbuild merged commit 310e644 into cms-sw:master Aug 27, 2021

Dr15Jones deleted the esConsumesRecoBTag branch September 9, 2021 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernized some modules in RecoBTag #35018

Modernized some modules in RecoBTag #35018

Dr15Jones commented Aug 25, 2021 •

edited

cmsbuild commented Aug 25, 2021

cmsbuild commented Aug 25, 2021

Dr15Jones commented Aug 25, 2021

cmsbuild commented Aug 25, 2021

slava77 Aug 26, 2021

Dr15Jones Aug 26, 2021

Dr15Jones Aug 26, 2021

slava77 Aug 26, 2021

Dr15Jones Aug 26, 2021

slava77 Aug 26, 2021

Dr15Jones Aug 26, 2021

cmsbuild commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

Dr15Jones commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

slava77 commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

perrotta commented Aug 27, 2021

Modernized some modules in RecoBTag #35018

Modernized some modules in RecoBTag #35018

Conversation

Dr15Jones commented Aug 25, 2021 • edited

PR description:

PR validation:

cmsbuild commented Aug 25, 2021

cmsbuild commented Aug 25, 2021

Dr15Jones commented Aug 25, 2021

cmsbuild commented Aug 25, 2021

Comparison Summary

slava77 Aug 26, 2021

Choose a reason for hiding this comment

Dr15Jones Aug 26, 2021

Choose a reason for hiding this comment

Dr15Jones Aug 26, 2021

Choose a reason for hiding this comment

slava77 Aug 26, 2021

Choose a reason for hiding this comment

Dr15Jones Aug 26, 2021

Choose a reason for hiding this comment

slava77 Aug 26, 2021

Choose a reason for hiding this comment

Dr15Jones Aug 26, 2021

Choose a reason for hiding this comment

cmsbuild commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

Dr15Jones commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

Comparison Summary

slava77 commented Aug 26, 2021

cmsbuild commented Aug 26, 2021

perrotta commented Aug 27, 2021

Dr15Jones commented Aug 25, 2021 •

edited