Clean HGCAL digitization. Fix Premixing for HGCAL #32397

rovere · 2020-12-07T10:33:19Z

PR description:

Purely technical. Remove support for older HGCAL geometries that are no longer supported. Clean and simplify the code.

Coupled with the restructuring of the code there's also a fix from @adas1994 (af8481d) to the premixing workflow.

PR validation:

Usual Phase2 workflows, including HFNose ones. No regression expected, but for the premixing workflows that should be fixed by this PR.

cmsbuild · 2020-12-07T10:40:02Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32397/20266

This PR adds an extra 48KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File SimCalorimetry/HGCalSimProducers/interface/HGCDigitizer.h modified in PR(s): Migrate most of MixingModule and PreMixingModule to EventSetup consumes #31697

cmsbuild · 2020-12-07T10:40:30Z

A new Pull Request was created by @rovere (Marco Rovere) for master.

It involves the following packages:

SimCalorimetry/HGCalSimProducers

@cmsbuild, @civanch, @kpedro88, @mdhildreth can you please review it and eventually sign? Thanks.
@vandreev11, @sethzenz, @makortel, @lgray, @cseez, @apsallid, @pfs, @hatakeyamak 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

rovere · 2020-12-07T10:44:26Z

@cmsbuild please test

cmsbuild · 2020-12-07T12:06:37Z

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cms-sw/cmssw#32397/11364/summary.html
CMSSW: CMSSW_11_3_X_2020-12-05-1100
SCRAM_ARCH: slc7_amd64_gcc900

cmsbuild · 2020-12-07T15:56:14Z

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-594c52/11384/summary.html
CMSSW: CMSSW_11_3_X_2020-12-05-1100
SCRAM_ARCH: slc7_amd64_gcc900

kpedro88 · 2020-12-07T16:49:31Z

SimCalorimetry/HGCalSimProducers/plugins/HGCDigitizerPluginFactory.cc

+#include "HFNoseDigitizer.h"
+
+EDM_REGISTER_PLUGINFACTORY(HGCDigitizerPluginFactory, "HGCDigitizerPluginFactory");
+DEFINE_EDM_PLUGIN(HGCDigitizerPluginFactory, HGCEEDigitizer, "HGCEEDigitizer");


The use of a factory to handle the per-subdetector digitizers is very nice.

Since these classes are now moved into the plugins directory, the content of the header files could put into the .cc files, as we usually recommend for plugins. Then each plugin could be registered in its own .cc file.

One could also define a simpler macro:

#define DEFINE_HGC_DIGITIZER(name) DEFINE_EDM_PLUGIN(HGCDigitizerPluginFactory, name, #name);

kpedro88 · 2020-12-07T16:54:23Z

@rovere thanks for this PR. In addition to the specific review comment, a few general questions/comments:

@bsunanda will this cleanup impact test beam analysis at all? (I think TB analysis that uses digitization can still proceed with the old CMSSW releases...)
There are a few other open issues relevant to digitization cleanup: Clean up HGCFEElectronics #25799, Clean up HGCal subdetector terminology and usage #26225 (digitizers are still called HGCHEfront and HGCHEback, while they're really silicon and scintillator). Will these be addressed in future PRs, or does it make sense to address them here?

bsunanda · 2020-12-07T17:29:44Z

Test beam analysis does not use the digitization code in current version of CMSSW. So this cleanup makes sense. I would like to see what they use in their analysis and make it a part of CMSSW for future usage.

cmsbuild · 2020-12-07T17:44:36Z

Comparison results are now available
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-594c52/11384/summary.html
CMSSW: CMSSW_11_3_X_2020-12-05-1100
SCRAM_ARCH: slc7_amd64_gcc900

Comparison Summary:

No significant changes to the logs found
ROOTFileChecks: Some differences in event products or their sizes found
Reco comparison results: 3141 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2747014
DQMHistoTests: Total failures: 28341
DQMHistoTests: Total nulls: 13
DQMHistoTests: Total successes: 2718638
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
Checked 153 log files, 37 edm output root files, 36 DQM output files

kpedro88 · 2020-12-07T18:03:14Z

There appear to be changes in the premixing workflow, e.g.:

@adas1994 did this PR accidentally solve #32050?

adas1994 · 2020-12-07T18:09:50Z

I had sent Marco the code that would fix the RecHits energy issue raised in #32050. It was a simple one-liner bug that someone else had introduced. The bug is present in as early version as CMSSW_11_1_0_pre8. Thanks for the update @kpedro88 .

kpedro88 · 2020-12-07T18:19:16Z

Aha, I hadn't looked at the commit messages carefully: the fix is deliberately included in af8481d. Thanks @adas1994 ! @rovere can you update the PR description to clarify this?

kpedro88 · 2020-12-14T23:02:04Z

@rovere what is the status of this PR?

rovere · 2020-12-15T15:43:03Z

@cmsbuild please test

rovere · 2020-12-15T15:45:23Z

@rovere thanks for this PR. In addition to the specific review comment, a few general questions/comments:

@bsunanda will this cleanup impact test beam analysis at all? (I think TB analysis that uses digitization can still proceed with the old CMSSW releases...)

There are a few other open issues relevant to digitization cleanup: Clean up HGCFEElectronics #25799, Clean up HGCal subdetector terminology and usage #26225 (digitizers are still called HGCHEfront and HGCHEback, while they're really silicon and scintillator). Will these be addressed in future PRs, or does it make sense to address them here?

Changing the names of the digitizer plugins would be possible but it would create a mismatch with the naming convention used for digi and rechits. Possibly, a separate PR could address all those kind of changes in one go later on.

cmsbuild · 2020-12-15T15:46:15Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32397/20417

This PR adds an extra 84KB to repository
Found files with invalid states:
- SimCalorimetry/HGCalSimProducers/plugins/HFNoseDigitizer.h:
  - Added: fc17a9b
  - Deleted: b8c7643
- SimCalorimetry/HGCalSimProducers/plugins/HGCHEbackDigitizer.h:
  - Added: fc17a9b
  - Deleted: b8c7643
- SimCalorimetry/HGCalSimProducers/plugins/HGCHEfrontDigitizer.h:
  - Added: fc17a9b
  - Deleted: b8c7643
- SimCalorimetry/HGCalSimProducers/plugins/HGCEEDigitizer.h:
  - Added: fc17a9b
  - Deleted: b8c7643
There are other open Pull requests which might conflict with changes you have proposed:
- File SimCalorimetry/HGCalSimProducers/interface/HGCDigitizer.h modified in PR(s): Migrate most of MixingModule and PreMixingModule to EventSetup consumes #31697

cmsbuild · 2020-12-15T15:46:44Z

Pull request #32397 was updated. @civanch, @kpedro88, @mdhildreth can you please check and sign again.

cmsbuild · 2020-12-15T19:12:30Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-594c52/11694/summary.html
CMSSW: CMSSW_11_3_X_2020-12-15-1100/slc7_amd64_gcc900

Comparison Summary

Summary:

No significant changes to the logs found
ROOTFileChecks: Some differences in event products or their sizes found
Reco comparison results: 3154 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2747284
DQMHistoTests: Total failures: 30863
DQMHistoTests: Total nulls: 13
DQMHistoTests: Total successes: 2716386
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
Checked 153 log files, 37 edm output root files, 36 DQM output files

kpedro88 · 2020-12-15T19:17:36Z

+upgrade

civanch · 2020-12-16T14:51:45Z

+1

cmsbuild · 2020-12-16T14:52:08Z

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 · 2020-12-16T16:03:19Z

+1

cmsbuild added this to the CMSSW_11_3_X milestone Dec 7, 2020

cmsbuild added code-checks-pending orp-pending pending-signatures simulation-pending tests-pending upgrade-pending labels Dec 7, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 7, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 7, 2020

kpedro88 reviewed Dec 7, 2020

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Dec 7, 2020

rovere changed the title ~~Clean HGCAL digitization~~ Clean HGCAL digitization. Fix Premixing for HGCAL Dec 8, 2020

rovere added 3 commits December 15, 2020 09:14

Remove unused parameter

a3125f4

Remove support for pre-TDR digitization

6990857

Remove unnecessary template

d4df7d4

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-approved labels Dec 15, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 15, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 15, 2020

cmsbuild added tests-approved and removed tests-started labels Dec 15, 2020

cmsbuild added upgrade-approved and removed upgrade-pending labels Dec 15, 2020

cmsbuild added fully-signed simulation-approved and removed pending-signatures simulation-pending labels Dec 16, 2020

cmsbuild added orp-approved and removed orp-pending labels Dec 16, 2020

cmsbuild merged commit 19088c2 into cms-sw:master Dec 16, 2020

slava77 mentioned this pull request Dec 19, 2020

HGCAL in phase-2 premix workflow .999: missing HEB rechits, low response in HEF and EE #32050

Open

kpedro88 mentioned this pull request Feb 12, 2021

Deprecate code support for HGCal v8 #27095

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean HGCAL digitization. Fix Premixing for HGCAL #32397

Clean HGCAL digitization. Fix Premixing for HGCAL #32397

rovere commented Dec 7, 2020 •

edited by silviodonato

Loading

cmsbuild commented Dec 7, 2020

cmsbuild commented Dec 7, 2020

rovere commented Dec 7, 2020

cmsbuild commented Dec 7, 2020

cmsbuild commented Dec 7, 2020

kpedro88 Dec 7, 2020

kpedro88 commented Dec 7, 2020

bsunanda commented Dec 7, 2020

cmsbuild commented Dec 7, 2020

kpedro88 commented Dec 7, 2020

adas1994 commented Dec 7, 2020 •

edited

Loading

kpedro88 commented Dec 7, 2020

kpedro88 commented Dec 14, 2020

rovere commented Dec 15, 2020

rovere commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

kpedro88 commented Dec 15, 2020

civanch commented Dec 16, 2020

cmsbuild commented Dec 16, 2020

silviodonato commented Dec 16, 2020

Clean HGCAL digitization. Fix Premixing for HGCAL #32397

Clean HGCAL digitization. Fix Premixing for HGCAL #32397

Conversation

rovere commented Dec 7, 2020 • edited by silviodonato Loading

PR description:

PR validation:

cmsbuild commented Dec 7, 2020

cmsbuild commented Dec 7, 2020

rovere commented Dec 7, 2020

cmsbuild commented Dec 7, 2020

cmsbuild commented Dec 7, 2020

kpedro88 Dec 7, 2020

Choose a reason for hiding this comment

kpedro88 commented Dec 7, 2020

bsunanda commented Dec 7, 2020

cmsbuild commented Dec 7, 2020

kpedro88 commented Dec 7, 2020

adas1994 commented Dec 7, 2020 • edited Loading

kpedro88 commented Dec 7, 2020

kpedro88 commented Dec 14, 2020

rovere commented Dec 15, 2020

rovere commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

cmsbuild commented Dec 15, 2020

Comparison Summary

kpedro88 commented Dec 15, 2020

civanch commented Dec 16, 2020

cmsbuild commented Dec 16, 2020

silviodonato commented Dec 16, 2020

rovere commented Dec 7, 2020 •

edited by silviodonato

Loading

adas1994 commented Dec 7, 2020 •

edited

Loading