Migrate most of MixingModule and PreMixingModule to EventSetup consumes #31697

makortel · 2020-10-07T15:25:30Z

PR description:

This PR is part of #31061. In addition of plain migration to EventSetup consumes I did some refactoring related to getting the EventSetup products (like moving the get() calls from beginRun/beginLumi to Event, and passing around const references to ES products instead of edm::ESHandle). It has two caveats (which make the "most of") that need to be discussed/agreed:

I noticed the MixingModule (and BMixingModule base class, so affects all mixing modules) have the capability of updating the mixing parameters from the EventSetup, see e.g.

cmssw/SimGeneral/MixingModule/plugins/MixingModule.cc

Lines 290 to 301 in c51856d

    
           void MixingModule::reload(const edm::EventSetup& setup) { 
        
             //change the basic parameters. 
        
             edm::ESHandle<MixingModuleConfig> config; 
        
             setup.get<MixingRcd>().get(config); 
        
             minBunch_ = config->minBunch(); 
        
             maxBunch_ = config->maxBunch(); 
        
             bunchSpace_ = config->bunchSpace(); 
        
             //propagate to change the workers 
        
             for (unsigned int ii = 0; ii < workersObjects_.size(); ++ii) { 
        
               workersObjects_[ii]->reload(setup); 
        
             } 
        
           }

cmssw/Mixing/Base/src/PileUp.cc

Lines 235 to 240 in c51856d

    
           void PileUp::reload(const edm::EventSetup& setup) { 
        
             //get the required parameters from DB. 
        
             edm::ESHandle<MixingModuleConfig> configM; 
        
             setup.get<MixingRcd>().get(configM); 
        
             const MixingInputConfig& config = configM->config(inputType_);

For the EventSetup consumes that alone is fine, but the pixel digitizer uses the bunchspacing to decide which of the two ES products it gets

cmssw/SimTracker/SiPixelDigitizer/plugins/SiPixelDigitizerAlgorithm.cc

Lines 587 to 595 in c51856d

    
           void SiPixelDigitizerAlgorithm::init_DynIneffDB(const edm::EventSetup& es, const unsigned int& bunchspace) { 
        
             if (AddPixelInefficiency && !pixelEfficiencies_.FromConfig) { 
        
               if (bunchspace == 50) 
        
                 es.get<SiPixelDynamicInefficiencyRcd>().get("50ns", SiPixelDynamicInefficiency_); 
        
               else 
        
                 es.get<SiPixelDynamicInefficiencyRcd>().get(SiPixelDynamicInefficiency_); 
        
               pixelEfficiencies_.init_from_db(geom_, SiPixelDynamicInefficiency_); 
        
             } 
        
           }

In principle it could be possible for the product-to-be-got to change in the IOV boundaries of MixingRcd (if this mode of operation would be enabled), on the other hand updates after the first event are, in fact, already made impossible by

cmssw/SimTracker/SiPixelDigitizer/plugins/SiPixelDigitizer.cc

Lines 260 to 264 in c51856d

    
           if (firstFinalizeEvent_) { 
        
             const unsigned int bunchspace = PileupInfo_->getMix_bunchSpacing(); 
        
             _pixeldigialgo->init_DynIneffDB(iSetup, bunchspace); 
        
             firstFinalizeEvent_ = false; 
        
           }

Nevertheless, it could be that the mixing parameters would be read from an EventSetup object instead of configuration. If this mode of operation needs to be supported, we need to figure out some way to get only (right) one of the SiPixelDynamicInefficiency products instead of both as in this PR. Possibilities include an ESProducer mayConsume-type feature, or abstracting the choice between the products into a new ESProducer. Anyway, the big question is if MixingModule infrastructure has to continue to support changing mixing parameters based on MixingModuleConfig EventSetup product?

Answer below: this mode should continue to be supported. To be done in a subsequent PR.

I did not migrate DataMixingModule (because it is not being used). Likely it becomes even more broken with this PR. I hope this is acceptable.

Answer below: Breaking DataMixingModule even more is acceptable.

PR validation:

Limited matrix runs.

cmsbuild · 2020-10-07T15:25:59Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-10-07T15:31:07Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31697/18856

This PR adds an extra 252KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File SimCalorimetry/EcalSimProducers/interface/EcalDigiProducer.h modified in PR(s): Ecal phase2 sim reco 11 1 x #29386
- File SimCalorimetry/EcalSimProducers/interface/EcalTimeDigiProducer.h modified in PR(s): Ecal phase2 sim reco 11 1 x #29386
- File SimCalorimetry/EcalSimProducers/src/EcalDigiProducer.cc modified in PR(s): Ecal phase2 sim reco 11 1 x #29386
- File SimCalorimetry/EcalSimProducers/src/EcalTimeDigiProducer.cc modified in PR(s): Ecal phase2 sim reco 11 1 x #29386

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

code-checks:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31697/18856/code-checks.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31697/18856/code-checks.patch | patch -p1
You can also run scram build code-checks to apply code checks directly
code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31697/18856/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31697/18856/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2020-10-07T15:37:05Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-10-07T15:42:55Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31697/18858

This PR adds an extra 252KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File SimCalorimetry/EcalSimProducers/interface/EcalDigiProducer.h modified in PR(s): Ecal phase2 sim reco 11 1 x #29386
- File SimCalorimetry/EcalSimProducers/interface/EcalTimeDigiProducer.h modified in PR(s): Ecal phase2 sim reco 11 1 x #29386
- File SimCalorimetry/EcalSimProducers/src/EcalDigiProducer.cc modified in PR(s): Ecal phase2 sim reco 11 1 x #29386
- File SimCalorimetry/EcalSimProducers/src/EcalTimeDigiProducer.cc modified in PR(s): Ecal phase2 sim reco 11 1 x #29386

cmsbuild · 2020-10-07T15:44:25Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

CalibTracker/SiPixelESProducers
RecoLocalTracker/SiStripZeroSuppression
SimCalorimetry/CastorSim
SimCalorimetry/EcalSimProducers
SimCalorimetry/EcalTestBeam
SimCalorimetry/HGCalSimProducers
SimCalorimetry/HcalSimProducers
SimCalorimetry/HcalTestBeam
SimFastTiming/FastTimingCommon
SimGeneral/CaloAnalysis
SimGeneral/MixingModule
SimGeneral/TrackingAnalysis
SimTracker/SiPhase2Digitizer
SimTracker/SiPixelDigitizer
SimTracker/SiStripDigitizer

@perrotta, @civanch, @yuanchao, @slava77, @christopheralanwest, @mdhildreth, @cmsbuild, @tocheng, @tlampen, @jpata, @pohsun, @kpedro88 can you please review it and eventually sign? Thanks.
@echabert, @felicepantaleo, @yduhm, @robervalwalsh, @argiro, @pfs, @thomreis, @lgray, @threus, @mmusich, @cericeci, @sethzenz, @abdoulline, @JanFSchulte, @jhgoh, @dgulhan, @apsallid, @prolay, @HuguesBrun, @slomeo, @ferencek, @pieterdavid, @vandreev11, @abbiendi, @GiacomoSguazzoni, @rovere, @VinInn, @cseez, @tocheng, @deguio, @hatakeyamak, @alesaggio, @ebrondol, @mtosi, @fabiocos, @rchatter, @trocino, @gbenelli, @Fedespring, @calderona, @dkotlins, @gpetruc, @mariadalfonso, @folguera, @tvami 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

makortel · 2020-10-07T15:47:06Z

@cmsbuild, please test

cmsbuild · 2020-10-07T15:47:30Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-10-07-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9778/console Started: 2020/10/07 17:48

cmsbuild · 2020-10-07T17:07:48Z

+1
Tested at: 316e388
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2430a0/9778/summary.html
CMSSW: CMSSW_11_2_X_2020-10-07-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-10-07T17:07:50Z

Comparison job queued.

cmsbuild · 2020-10-07T18:32:41Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2430a0/9778/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2542225
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2542202
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

cmsbuild · 2021-04-20T16:11:24Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2430a0/14351/summary.html
COMMIT: 7f1c3b1
CMSSW: CMSSW_12_0_X_2021-04-19-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/31697/14351/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2430a0/14351/llvm-analysis/cmsclangtidy.txt for details.

Comparison Summary

Summary:

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

perrotta · 2021-04-21T08:17:29Z

+1

Mostly AlCa and Sim: for Reco there are only two classes in RecoLocalTrackers touched, with rather simple updates
Jenkins tests pass, with no differences in output

yuanchao · 2021-04-23T03:07:01Z

+1

migrated most of EventSetupRecord::get(ESHandle&) to EventSetup::getHandle(ESGetToken&); two in TODO explained
WF matrix tests and unit tests all passed

srimanob · 2021-04-24T09:44:17Z

+Upgrade

civanch · 2021-04-24T13:44:35Z

+1

silviodonato · 2021-04-26T09:16:20Z

kind reminder @cms-sw/db-l2

makortel · 2021-04-29T15:11:37Z

@ggovi Could you please review and sign? Thanks.

ggovi · 2021-04-30T08:02:45Z

+1

cmsbuild · 2021-04-30T08:03:09Z

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-04-30T08:16:42Z

+1

cmsbuild added this to the CMSSW_11_2_X milestone Oct 7, 2020

cmsbuild added alca-pending code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending upgrade-pending labels Oct 7, 2020

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Oct 7, 2020

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

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

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

cmsbuild added tests-approved and removed tests-started labels Apr 20, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Apr 21, 2021

cmsbuild added alca-approved and removed alca-pending labels Apr 23, 2021

cmsbuild added upgrade-approved and removed upgrade-pending labels Apr 24, 2021

cmsbuild added simulation-approved and removed simulation-pending labels Apr 24, 2021

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Apr 30, 2021

cmsbuild added orp-approved and removed orp-pending labels Apr 30, 2021

cmsbuild merged commit 87a7cf9 into cms-sw:master Apr 30, 2021

cmsbuild mentioned this pull request Apr 30, 2021

Pixel local reco: reduce code duplication between SiPixelDigiErrorsFromSoA and SiPixelRawToDigi #33526

Merged

makortel mentioned this pull request May 4, 2021

Treatment of bunch spacing in MixingModule and PreMixingModule for EventSetup consumes #33616

Closed

pieterdavid mentioned this pull request May 5, 2021

esConsumes migration for SiStripFedZeroSuppression, SiStripHitEffFromCalibTree, and SiStripClusterToDigiProducer #33632

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate most of MixingModule and PreMixingModule to EventSetup consumes #31697

Migrate most of MixingModule and PreMixingModule to EventSetup consumes #31697

makortel commented Oct 7, 2020 •

edited

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

makortel commented Oct 7, 2020

cmsbuild commented Oct 7, 2020 •

edited

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Apr 20, 2021

perrotta commented Apr 21, 2021

yuanchao commented Apr 23, 2021

srimanob commented Apr 24, 2021

civanch commented Apr 24, 2021

silviodonato commented Apr 26, 2021

makortel commented Apr 29, 2021

ggovi commented Apr 30, 2021

cmsbuild commented Apr 30, 2021

silviodonato commented Apr 30, 2021

	void MixingModule::reload(const edm::EventSetup& setup) {
	//change the basic parameters.
	edm::ESHandle<MixingModuleConfig> config;
	setup.get<MixingRcd>().get(config);
	minBunch_ = config->minBunch();
	maxBunch_ = config->maxBunch();
	bunchSpace_ = config->bunchSpace();
	//propagate to change the workers
	for (unsigned int ii = 0; ii < workersObjects_.size(); ++ii) {
	workersObjects_[ii]->reload(setup);
	}
	}

	void PileUp::reload(const edm::EventSetup& setup) {
	//get the required parameters from DB.
	edm::ESHandle<MixingModuleConfig> configM;
	setup.get<MixingRcd>().get(configM);

	const MixingInputConfig& config = configM->config(inputType_);

	void SiPixelDigitizerAlgorithm::init_DynIneffDB(const edm::EventSetup& es, const unsigned int& bunchspace) {
	if (AddPixelInefficiency && !pixelEfficiencies_.FromConfig) {
	if (bunchspace == 50)
	es.get<SiPixelDynamicInefficiencyRcd>().get("50ns", SiPixelDynamicInefficiency_);
	else
	es.get<SiPixelDynamicInefficiencyRcd>().get(SiPixelDynamicInefficiency_);
	pixelEfficiencies_.init_from_db(geom_, SiPixelDynamicInefficiency_);
	}
	}

	if (firstFinalizeEvent_) {
	const unsigned int bunchspace = PileupInfo_->getMix_bunchSpacing();
	_pixeldigialgo->init_DynIneffDB(iSetup, bunchspace);
	firstFinalizeEvent_ = false;
	}

Migrate most of MixingModule and PreMixingModule to EventSetup consumes #31697

Migrate most of MixingModule and PreMixingModule to EventSetup consumes #31697

Conversation

makortel commented Oct 7, 2020 • edited

PR description:

PR validation:

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

makortel commented Oct 7, 2020

cmsbuild commented Oct 7, 2020 • edited

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Apr 20, 2021

Comparison Summary

perrotta commented Apr 21, 2021

yuanchao commented Apr 23, 2021

srimanob commented Apr 24, 2021

civanch commented Apr 24, 2021

silviodonato commented Apr 26, 2021

makortel commented Apr 29, 2021

ggovi commented Apr 30, 2021

cmsbuild commented Apr 30, 2021

silviodonato commented Apr 30, 2021

makortel commented Oct 7, 2020 •

edited

cmsbuild commented Oct 7, 2020 •

edited