Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt to new range for BDT output for Run3 Saturated regression #37100

Merged
merged 2 commits into from
Mar 3, 2022
Merged

Adapt to new range for BDT output for Run3 Saturated regression #37100

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f685bd
change range for Run3 Saturated regression
swagata87 Feb 26, 2022
10660de
era run3 egamma
swagata87 Mar 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions RecoEgamma/EgammaTools/python/regressionModifier_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,5 +246,16 @@

(run2_egamma_2016 | run2_egamma_2017 | run2_egamma_2018).toReplaceWith(regressionModifier,regressionModifier106XUL)

from Configuration.Eras.Modifier_run3_common_cff import run3_common
run3_common.toModify(regressionModifier106XUL.eleRegs.ecalOnlyMean,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be a run3_egamma instead, just to follow the old patterns?

rangeMinHighEt = 0.2,
rangeMaxHighEt = 2.0
)

run3_common.toModify(regressionModifier106XUL.phoRegs.ecalOnlyMean,
rangeMinHighEt = 0.2,
rangeMaxHighEt = 2.0
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could something similar also work? (Please check before implementing, if you decide so)

Suggested change
(run2_egamma_2016 | run2_egamma_2017 | run2_egamma_2018).toReplaceWith(regressionModifier,regressionModifier106XUL)
from Configuration.Eras.Modifier_run3_common_cff import run3_common
run3_common.toModify(regressionModifier106XUL.eleRegs.ecalOnlyMean,
rangeMinHighEt = 0.2,
rangeMaxHighEt = 2.0
)
run3_common.toModify(regressionModifier106XUL.phoRegs.ecalOnlyMean,
rangeMinHighEt = 0.2,
rangeMaxHighEt = 2.0
)
regressionModifierRun2 = regressionModifier106XUL.clone()
from Configuration.Eras.Modifier_run2_egamma_2016_cff import run2_egamma_2016
from Configuration.Eras.Modifier_run2_egamma_2017_cff import run2_egamma_2017
from Configuration.Eras.Modifier_run2_egamma_2018_cff import run2_egamma_2018
(run2_egamma_2016 | run2_egamma_2017 | run2_egamma_2018).toReplaceWith(regressionModifier,regressionModifierRun2)
regressionModifierRun3 = regressionModifierRun2.clone(
eleRegs = dict(
ecalOnlyMean = dict(
rangeMinHighEt = 0.2,
rangeMaxHighEt = 2.0
)
),
phoRegs = dict(
ecalOnlyMean = dict(
rangeMinHighEt = 0.2,
rangeMaxHighEt = 2.0
)
)
)
from Configuration.Eras.Modifier_run3_common_cff import run3_common
run3_common.toReplaceWith(regressionModifier,regressionModifierRun3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's wrong with the original? except for the fact that there we should just have run3_common.toModify(regressionModifier.phoRegs.ecalOnlyMean

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original one, ie regressionModifier clones regressionModifier94X,

cmssw/RecoEgamma/EgammaTools/python/regressionModifier_cfi.py

Line 240 in 6558221

regressionModifier = regressionModifier94X.clone()

and regressionModifier94X is incompatible with Run3 as it uses EGRegressionModifierV2, while we need EGRegressionModifierV3.

cmssw/RecoEgamma/EgammaTools/python/regressionModifier_cfi.py

Lines 183 to 185 in 6558221

regressionModifier94X = \
cms.PSet( modifierName = cms.string('EGRegressionModifierV2'),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it's already replaced at that point in the line above
(run2_egamma_2016 | run2_egamma_2017 | run2_egamma_2018).toReplaceWith(regressionModifier,regressionModifier106XUL)

(run2_egamma_2018 is active in the Run3 era)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it's already replaced at that point in the line above
(run2_egamma_2016 | run2_egamma_2017 | run2_egamma_2018).toReplaceWith(regressionModifier,regressionModifier106XUL)

to be more obvious that line could be updated to be
(run2_egamma_2016 | run2_egamma_2017 | run2_egamma_2018 | run3_common).toReplaceWith ...

(my other comment about introducing a run3_egamma instead still applies; although that one is probably more for convenience of finding egamma-related changes)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for explaining, Slava. I gave it a try[1].
so the way I originally implemented it, and the way you suggested, both have an unintended effect (discussed here also #37102 (comment)), that new parameter values gets propagated to low pT electron regression too. (I checked it by doing edmConfigDump after implementing [1]).

So now I will try the way Andrea suggested. And I will also try to add run3_egamma as per your suggestion.

[1]

(run2_egamma_2016 | run2_egamma_2017 | run2_egamma_2018 | run3_common).toReplaceWith(regressionModifier,regressionModifier106XUL)

run3_common.toModify(regressionModifier.eleRegs.ecalOnlyMean,
      rangeMinHighEt = 0.2,
      rangeMaxHighEt = 2.0
)

run3_common.toModify(regressionModifier.phoRegs.ecalOnlyMean,
      rangeMinHighEt = 0.2,
      rangeMaxHighEt = 2.0
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both have an unintended effect (discussed here also #37102 (comment)), that new parameter values gets propagated to low pT electron regression too.

uhm, what about
(run2_egamma_2016 | run2_egamma_2017 | run2_egamma_2018 | run3_common).toReplaceWith(regressionModifier,regressionModifier106XUL.clone()) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slava77 I don't think it can work.
For Run3 the regressionModifier has to be further modified: it cannot be the same as for Run2.

from Configuration.ProcessModifiers.egamma_lowPt_exclusive_cff import egamma_lowPt_exclusive
egamma_lowPt_exclusive.toReplaceWith(regressionModifier,regressionModifier103XLowPtPho)