Added the switch for the inclusion of PS energy #37491

ram1123 · 2022-04-07T14:29:06Z

PR description:

EGamma regression can be trained with and without PS energy taking into consideration. So, we added a switch for this while performing the evaluation in the CMSSW.

This PR is relevant ONLY for HLT supercluster regression, and that it does not effect the offline egamma regression.

PR validation:

We run the code with the current version of update and it worked as expected.

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

cmsbuild · 2022-04-07T14:41:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37491/29193

This PR adds an extra 16KB to repository

cmsbuild · 2022-04-07T14:43:20Z

A new Pull Request was created by @ram1123 (Ramkrishna Sharma) for master.

It involves the following packages:

RecoEcal/EgammaClusterAlgos (reconstruction)

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

tvami · 2022-04-07T15:08:26Z

Hi @ram1123 this is relevant for Run-3 only right? If yes, then I believe it should be under the Run-3 modifier.

thomreis · 2022-04-07T15:10:42Z

RecoEcal/EgammaClusterAlgos/interface/SCEnergyCorrectorSemiParm.h

@@ -146,6 +147,7 @@ template <edm::Transition esTransition>
 void SCEnergyCorrectorSemiParm::setTokens(const edm::ParameterSet& iConfig, edm::ConsumesCollector cc) {
  isHLT_ = iConfig.getParameter<bool>("isHLT");
  isPhaseII_ = iConfig.getParameter<bool>("isPhaseII");
+  regTrainedWithPS_ = iConfig.getParameter<bool>("regTrainedWithPS");


This parameter should be added to the fillPSetDescription as well.
Why is it set in this function and not in the constructor itself? It is not needed to set any token.

swagata87 · 2022-04-07T15:22:08Z

Hi @ram1123 this is relevant for Run-3 only right? If yes, then I believe it should be under the Run-3 modifier.

Aren't modifiers relevant only for offline reco?
Ram's PR is specific to HLT supercluster regression.

francescobrivio · 2022-04-07T16:18:03Z

From private conversation with @swagata87 and EGM experts we understood that this change should go together with update of the EGM conditions in the HLT GT.
Note that the official request to update the GT hasn't come yet (@saumyaphor4252 please do it asap!), so we might need a couple of days. I'll bring this up at the ORP tomorrow. FYI @perrotta @qliphy

francescobrivio · 2022-04-07T16:20:25Z

urgent

with the caveat described in Added the switch for the inclusion of PS energy #37491 (comment)

francescobrivio

After more discussion with EGM experts and @cms-sw/alca-l2 we came up with a tentative plan:

include this PR in the release with false flag (since the conditions are not there yet)
we cut the 12_3_0 release
we (alca) do the FullTrackValidation (where we can change the flag by hand and thus really test the new conditions)
we (alca) will do a PR with the new HLT GT and changing the flag to true

The only drawback is that 12_3_0 won't have the latest HLT GT, but that can always be added later, and this is a good way to include already in 12_3_0 all the c++ changes needed.

francescobrivio · 2022-04-07T17:10:46Z

RecoEcal/EgammaClusterAlgos/src/SCEnergyCorrectorSemiParm.cc

@@ -51,6 +51,7 @@ SCEnergyCorrectorSemiParm::SCEnergyCorrectorSemiParm()
      caloGeom_(nullptr),
      isHLT_(false),
      isPhaseII_(false),
+      regTrainedWithPS_(true),


@ram1123 please change this to false. As far as I understood this should restore things to the way they are now.
Once we have the new conditions validate we (@cms-sw/alca-l2) we'll take care of changing it to true when we update the GT.

swagata87 · 2022-04-07T17:52:27Z

@ram1123 somewhere in PR description, please make it clear that this PR is relevant ONLY for HLT supercluster regression, and that it does not effect the offline egamma regression.

cmsbuild · 2022-04-07T23:18:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37491/29206

This PR adds an extra 16KB to repository

cmsbuild · 2022-04-07T23:18:39Z

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

qliphy · 2022-04-08T00:49:09Z

please test

cmsbuild · 2022-04-08T05:45:28Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-63d4f5/23752/summary.html
COMMIT: e70ade1
CMSSW: CMSSW_12_4_X_2022-04-07-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37491/23752/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: 48
DQMHistoTests: Total histograms compared: 3593039
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3593015
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 200 log files, 45 edm output root files, 48 DQM output files
TriggerResults: no differences found

clacaputo · 2022-04-08T10:24:06Z

assign hlt

cmsbuild · 2022-04-08T10:24:28Z

New categories assigned: hlt

@missirol,@Martin-Grunewald you have been requested to review this Pull request/Issue and eventually sign? Thanks

clacaputo · 2022-04-08T10:26:01Z

+reconstruction

PR relevant only for HLT supercluster regression
boolean set to false, see Added the switch for the inclusion of PS energy #37491 (review)

missirol · 2022-04-08T10:38:50Z

+hlt

cmsbuild · 2022-04-08T10:39:12Z

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 · 2022-04-08T11:01:15Z

+1

A new option added, and the default value does not change previous behaviour

Added the switch for the inclusion of PS energy

99de122

cmsbuild added this to the CMSSW_12_4_X milestone Apr 7, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 7, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 7, 2022

thomreis reviewed Apr 7, 2022

View reviewed changes

ram1123 mentioned this pull request Apr 7, 2022

[12_3_X] Added the switch for the inclusion of PS energy #37494

Merged

cmsbuild added the urgent label Apr 7, 2022

francescobrivio suggested changes Apr 7, 2022

View reviewed changes

Added the parameter to fillPSetDescription & set it to false

e70ade1

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 7, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 7, 2022

cmsbuild added tests-started and removed tests-pending labels Apr 8, 2022

cmsbuild added tests-approved and removed tests-started labels Apr 8, 2022

cmsbuild added the hlt-pending label Apr 8, 2022

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Apr 8, 2022

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Apr 8, 2022

cmsbuild added orp-approved and removed orp-pending labels Apr 8, 2022

cmsbuild merged commit deaa652 into cms-sw:master Apr 8, 2022

missirol mentioned this pull request Apr 13, 2022

Add PPS and EGM tags to dataRun3 GTs #37557

Merged

tvami mentioned this pull request Apr 15, 2022

Updating EGM HLT supercluster regression tags for Run 3 in MC #37588

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the switch for the inclusion of PS energy #37491

Added the switch for the inclusion of PS energy #37491

ram1123 commented Apr 7, 2022 •

edited

cmsbuild commented Apr 7, 2022

cmsbuild commented Apr 7, 2022

tvami commented Apr 7, 2022

thomreis Apr 7, 2022

swagata87 commented Apr 7, 2022

francescobrivio commented Apr 7, 2022 •

edited

francescobrivio commented Apr 7, 2022

francescobrivio left a comment

francescobrivio Apr 7, 2022

swagata87 commented Apr 7, 2022

cmsbuild commented Apr 7, 2022

cmsbuild commented Apr 7, 2022

qliphy commented Apr 8, 2022

cmsbuild commented Apr 8, 2022

clacaputo commented Apr 8, 2022

cmsbuild commented Apr 8, 2022

clacaputo commented Apr 8, 2022

missirol commented Apr 8, 2022

cmsbuild commented Apr 8, 2022

perrotta commented Apr 8, 2022

Added the switch for the inclusion of PS energy #37491

Added the switch for the inclusion of PS energy #37491

Conversation

ram1123 commented Apr 7, 2022 • edited

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 Apr 7, 2022

cmsbuild commented Apr 7, 2022

tvami commented Apr 7, 2022

thomreis Apr 7, 2022

Choose a reason for hiding this comment

swagata87 commented Apr 7, 2022

francescobrivio commented Apr 7, 2022 • edited

francescobrivio commented Apr 7, 2022

francescobrivio left a comment

Choose a reason for hiding this comment

francescobrivio Apr 7, 2022

Choose a reason for hiding this comment

swagata87 commented Apr 7, 2022

cmsbuild commented Apr 7, 2022

cmsbuild commented Apr 7, 2022

qliphy commented Apr 8, 2022

cmsbuild commented Apr 8, 2022

Comparison Summary

clacaputo commented Apr 8, 2022

cmsbuild commented Apr 8, 2022

clacaputo commented Apr 8, 2022

missirol commented Apr 8, 2022

cmsbuild commented Apr 8, 2022

perrotta commented Apr 8, 2022

ram1123 commented Apr 7, 2022 •

edited

francescobrivio commented Apr 7, 2022 •

edited