drop type specs in RecoEcal #31332

jeongeun · 2020-09-02T11:46:51Z

PR description:

Update the safer syntax for existing parameter :

drop type specifications where the original parameter exists.
move all parameter inside the clone

Instead of modifying parameters with full type specs, which can be interpreted as an insertion of a new parameter, it is a safer way to protect from parameter name mistakes and will also help in possible parameter migrations.
(The references were PR#30700, PR#30827, PR#30947,PR#31162,PR#31243)

In this PR, total 10 files updated.

RecoEcal/{EgammaClusterProducers} 10 files

PR validation:

Event Content comparison check was also done and there is no change with these updates.
Tested in CMSSW_11_2_X, the basic test all passed in the CMSSW PR instructions.

cmsbuild · 2020-09-02T11:47:13Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-09-02T11:53:54Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31332/18092

This PR adds an extra 16KB to repository

cmsbuild · 2020-09-02T11:54:14Z

A new Pull Request was created by @jeongeun (JeongEun Lee) for master.

It involves the following packages:

RecoEcal/EgammaClusterProducers

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper, @rchatter, @jainshilpi, @rovere, @argiro, @sobhatta, @lecriste, @afiqaize, @lgray, @varuns23 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

perrotta · 2020-09-02T11:56:39Z

please test

cmsbuild · 2020-09-02T11:57:04Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-09-01-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9062/console Started: 2020/09/02 13:59

perrotta · 2020-09-02T12:03:24Z

RecoEcal/EgammaClusterProducers/python/multi5x5SuperClusters_cfi.py

-multi5x5SuperClustersUncleaned.endcapClusterProducer = cms.string('multi5x5BasicClustersUncleaned')
-
+multi5x5SuperClustersUncleaned = multi5x5SuperClustersCleaned.clone(
+    endcapClusterProducer = cms.string('multi5x5BasicClustersUncleaned')


"string" can be dropped

No, forget it @jeongeun : the corresponding parameter is not listed in the cloned config

Triggered by this finding, I'm realizing that a Multi5x5SuperClusterProducer does not require a parameter called "endcapClusterProducer" (which would be needed by a HiSuperClusterProducer, instead).

If I'm not wrong, one should have replaced here instead

endcapClusterTag= cms.InputTag('multi5x5BasicClustersUncleaned', 'multi5x5EndcapBasicClusters'),

@afiqaize @SohamBhattacharya could you please have a look and tell us if there is a bug here?

cmsbuild · 2020-09-02T13:11:18Z

+1
Tested at: c1a05d7
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-30f180/9062/summary.html
CMSSW: CMSSW_11_2_X_2020-09-01-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-02T13:11:21Z

Comparison job queued.

cmsbuild · 2020-09-02T14:41:13Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-30f180/9062/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2609667
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2609633
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 34 files compared)
DQMHistoSizes: changed ( 10224.0 ): 0.004 KiB MessageLogger/Warnings
Checked 149 log files, 22 edm output root files, 35 DQM output files

perrotta · 2020-09-07T08:52:36Z

@afiqaize @SohamBhattacharya could you please check if there is actually a bug in the configuration of the multi5x5SuperClustersUncleaned module as implemented in RecoEcal/EgammaClusterProducers/python/multi5x5SuperClusters_cfi.py, and if we can profit of this PR to implement the fix suggested in #31332 (comment), if so?

afiqaize · 2020-09-07T10:14:51Z

iirc multi 5x5 is an old SC algo that we don't use anymore in pp reconstruction, so this fix should have no effect. is this is used anywhere at the moment? also, shouldn't it be an error if a module (Multi5x5SuperClusterProducer in this case) is passed a spurious parameter?

perrotta · 2020-09-07T10:39:37Z

Thank you @afiqaize

iirc multi 5x5 is an old SC algo that we don't use anymore in pp reconstruction,

If so, it is probably time to clean it up from the release and avoid maintaining unneeded code in it (but also read below)

so this fix should have no effect. is this is used anywhere at the moment?

Well, as far as I can see it is still kept in some AlCa and HI event contents:

Calibration/EcalAlCaRecoProducers/python/ALCARECOEcalCalIsolElectron_Output_cff.py
Calibration/HcalAlCaRecoProducers/python/ALCARECOHcalCalGammaJet_Output_cff.py
RecoHI/HiEgammaAlgos/python/RecoHiEgamma_EventContent_cff.py

Moreover, I see it entering the multi5x5ClusteringTask, which is included in the ecalClustersNoPFBoxTask, which ends up in the ecalClustersTask, which is one of the components of the globalrecoTask: therefore, unless I made some mistake in inspecting the configurations, it seems to me that the module is still included in the standard workflows, and its content saved at least in some Alca and HI output

also, shouldn't it be an error if a module (Multi5x5SuperClusterProducer in this case) is passed a spurious parameter?

No, because no fillDescriptions() method was ever implemented in the Multi5x5SuperClusterProducer, and therefore there is no parameter validation in it

afiqaize · 2020-09-07T10:50:23Z

Ok, I do remember it being used in HI (though not until what point) so this is not surprising. As for AlCa, maybe for validation purposes. I'll have to check with other EGM experts.

Nevertheless, iiuc what concerns this PR is only the existence of a spurious parameter, which should not affect any output. If this is correct I think we can go ahead with the fix without problems...

also, shouldn't it be an error if a module (Multi5x5SuperClusterProducer in this case) is passed a spurious parameter?

No, because no fillDescriptions() method was ever implemented in the Multi5x5SuperClusterProducer, and therefore there is no parameter validation in it

...perhaps even adding the validation to prevent similar problems in the future.

cmsbuild · 2020-09-09T05:57:19Z

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

perrotta · 2020-09-09T06:16:31Z

please test

cmsbuild · 2020-09-09T06:16:54Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-09-08-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9214/console Started: 2020/09/09 08:18

cmsbuild · 2020-09-09T07:29:28Z

+1
Tested at: 802701a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-30f180/9214/summary.html
CMSSW: CMSSW_11_2_X_2020-09-08-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-09T07:29:30Z

Comparison job queued.

cmsbuild · 2020-09-09T08:57:55Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-30f180/9214/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 46 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2609667
DQMHistoTests: Total failures: 18
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2609626
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 34 files compared)
DQMHistoSizes: changed ( 10224.0 ): -0.004 KiB MessageLogger/Warnings
Checked 149 log files, 22 edm output root files, 35 DQM output files

perrotta · 2020-09-09T09:22:20Z

Reco comparison results: 46 differences found in the comparisons

As expected, having fixed the configuration for the multi5x5SuperClustersUncleaned [1] one observes (tiny) changes in 5x5SuperClusters in the test outputs.

Apparently, there are more worklows in which those multi 5x5 are used with respect to what guessed by EGamma (see #31332 (comment)): it is therefore urgent to clean them out from pp reco sequences if they are really not needed.

Since now there is the correct endcapClusterTag input, I think this PR can be accepted as such.
I'd let EGamma (@afiqaize @SohamBhattacharya), ECal DPG (@rchatter), HI (@mandrenguyen) to inspect the validation outputs and evaluate the effect of the differences.

[1] endcapClusterTag moved from 'multi5x5BasicClustersCleaned:multi5x5EndcapBasicClusters' to 'multi5x5BasicClustersUncleaned:multi5x5EndcapBasicClusters'

perrotta · 2020-09-09T13:53:29Z

+1

Dropped type specification for parameters in cloned configs, as planned
While doing so, a bug was spotted in the configuration of the multi5x5SuperClustersUncleaned and fixed. As a result, some tiny regression in those 5x5 clusters is visible: it would be nice to have this merged already in pre6, so that the effect can be evaluated and validated on relvals
No difference anywhere else in reco outputs, as expected

cmsbuild · 2020-09-09T13:53:53Z

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-09-09T15:34:32Z

+1

jeongeun added 2 commits September 2, 2020 13:40

drop type specification where original parameter exist (RecoEcal)

e6f4450

fix minor

c1a05d7

cmsbuild added this to the CMSSW_11_2_X milestone Sep 2, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 2, 2020

cmsbuild removed the code-checks-pending label Sep 2, 2020

cmsbuild added the code-checks-approved label Sep 2, 2020

cmsbuild added tests-started and removed tests-pending labels Sep 2, 2020

perrotta reviewed Sep 2, 2020

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Sep 2, 2020

cmsbuild added comparison-available and removed comparison-pending labels Sep 2, 2020

cmsbuild added tests-started and removed tests-pending labels Sep 9, 2020

cmsbuild added tests-approved and removed tests-started labels Sep 9, 2020

cmsbuild added comparison-available and removed comparison-pending labels Sep 9, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Sep 9, 2020

cmsbuild added orp-approved and removed orp-pending labels Sep 9, 2020

cmsbuild merged commit 769b4a7 into cms-sw:master Sep 9, 2020

perrotta mentioned this pull request Sep 10, 2020

Check usage of multi5x5 ECAL modules, and remove them from the reco sequences when not needed #31422

Open

This was referenced Sep 21, 2020

drop type specs in RecoMET #31523

Merged

drop type specs in RecoBTag #31538

Merged

This was referenced Oct 11, 2020

drop type specs in RecoTauTag #31748

Merged

drop type specs in RecoVertex RecoPixelVertexing #31784

Merged

jeongeun mentioned this pull request Oct 21, 2020

drop type specs in RecoHGCal #31881

Merged

jeongeun mentioned this pull request Nov 5, 2020

drop type specs in RecoHI {HiEgammaAlgos,HiJetAlgos,HiMuonAlgos} #32031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop type specs in RecoEcal #31332

drop type specs in RecoEcal #31332

jeongeun commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

perrotta commented Sep 2, 2020

cmsbuild commented Sep 2, 2020 •

edited

perrotta Sep 2, 2020

perrotta Sep 2, 2020

perrotta Sep 2, 2020

cmsbuild commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

perrotta commented Sep 7, 2020

afiqaize commented Sep 7, 2020

perrotta commented Sep 7, 2020

afiqaize commented Sep 7, 2020

cmsbuild commented Sep 9, 2020

perrotta commented Sep 9, 2020

cmsbuild commented Sep 9, 2020 •

edited

cmsbuild commented Sep 9, 2020

cmsbuild commented Sep 9, 2020

cmsbuild commented Sep 9, 2020

perrotta commented Sep 9, 2020

perrotta commented Sep 9, 2020

cmsbuild commented Sep 9, 2020

silviodonato commented Sep 9, 2020

drop type specs in RecoEcal #31332

drop type specs in RecoEcal #31332

Conversation

jeongeun commented Sep 2, 2020

PR description:

PR validation:

cmsbuild commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

perrotta commented Sep 2, 2020

cmsbuild commented Sep 2, 2020 • edited

perrotta Sep 2, 2020

Choose a reason for hiding this comment

perrotta Sep 2, 2020

Choose a reason for hiding this comment

perrotta Sep 2, 2020

Choose a reason for hiding this comment

cmsbuild commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

cmsbuild commented Sep 2, 2020

perrotta commented Sep 7, 2020

afiqaize commented Sep 7, 2020

perrotta commented Sep 7, 2020

afiqaize commented Sep 7, 2020

cmsbuild commented Sep 9, 2020

perrotta commented Sep 9, 2020

cmsbuild commented Sep 9, 2020 • edited

cmsbuild commented Sep 9, 2020

cmsbuild commented Sep 9, 2020

cmsbuild commented Sep 9, 2020

perrotta commented Sep 9, 2020

perrotta commented Sep 9, 2020

cmsbuild commented Sep 9, 2020

silviodonato commented Sep 9, 2020

cmsbuild commented Sep 2, 2020 •

edited

cmsbuild commented Sep 9, 2020 •

edited