LowPtElectrons: final energy regression and ID (back port of 32391)

[bainbrid]

PR description:

This PR:

• Provides support for UL re-MINIAOD of both "standard" and BParking data sets, using the modifiers bParking, run2_miniAOD_UL, and run2_miniAOD_devel.

• Leaves the RECO sequence, and RECO/AOD content, unchanged w.r.t. 10_6_25.

• Enables energy regression for certain modifier configurations (detailed below). The energy regression performance can be found on slides 6-7 here.

• Enables new electron ID models for certain modifier configurations (detailed below). The performance of the ID models 2020Nov28 and 2021May17 can be found on slides 4-5 here.

• Depends on PR LowPtElectrons: 2020Nov28 and 2021May17 models for 10_6_X cms-data/RecoEgamma-ElectronIdentification#19

A detailed summary of the modifier configurations provided in this PR can be found at this TWiki (see table). A brief summary is provided below.

1. No changes to the RECO sequence are applied.

2. No-change policy for UL re-MINIAOD of standard data sets, modifier (~bParking & run2_miniAOD_UL):

• the functionality provided by this logic is preserved w.r.t. release CMSSW_10_6_20, which was used as part of the Winter 2020 UL re-MINIAOD campaign for standard data sets. (No energy regression is performed; the 2020Sept15 ID model is evaluated.)

3. Changes to PAT for any future (re)MINIAOD campaign for standard data sets, modifier (~bParking & run2_miniAOD_devel):

• Energy regression is performed as part of the re-MINIAOD sequence.

• ID is evaluated as part of the re-MINIAOD sequence. A new ID weights file (2020Nov28) has been provided for use with a UL re-MINIAOD campaign. There is no appreciable extra CPU load (w.r.t. the original model), and the memory footprint will reduce (due to the smaller weights file). The 2020Nov28 model is available within PR LowPtElectrons: 2020Nov28 and 2021May17 models for 10_6_X cms-data/RecoEgamma-ElectronIdentification#19.

• Low-pT PAT electrons are selected based on the following conditions pT > 1.0 GeV and ID score > -0.25, which provides 90% signal efficiency, a 10% mistag rate, resulting in an increase to the MINIAOD footprint of ~1%.

4. Changes to PAT for any future (re)MINIAOD campaign for the BParking data set, modifier (bParking & run2_miniAOD_UL):

• Energy regression is performed as part of the re-MINIAOD sequence.

• ID is evaluated as part of the re-MINIAOD sequence. A new ID weights file (2021May17), has been provided for use with a UL re-MINIAOD campaign. There is no appreciable extra CPU load (w.r.t. the original model), and the memory footprint will reduce (due to the smaller weights file). The 2021May17 model is available within PR LowPtElectrons: 2020Nov28 and 2021May17 models for 10_6_X cms-data/RecoEgamma-ElectronIdentification#19.

• Low-pT PAT electrons are selected based on the following conditions pT > 1.0 GeV. Computing performance studies can be found below. The raising of the minimum electron pT threshold from 'open' (effectively ~0.5 GeV) to 1.0 GeV reduces the MINIAOD event size by ~20%, based on the 136.898 workflow (data ParkingBPH 2018B).

PR validation:

• Local checks performed for both the RECO and PAT steps, for all relevant modifier configurations
• The following workflows were considered: 136.88811, 1325.518, 136.898
• the 2020Nov28 and 2021May17 model performances in CMSSW were validated against our standalone test bench setup.

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

This PR is a back port of #32391

Forward port of additional functionality

• Most for the aforementioned changes are already in master (merged PR: LowPtElectrons: final energy regression and ID #32391)
• However, a few recent changes have also been "forward ported" to a new PR to master: LowPtElectrons: support for UL re-MINIAOD and BParking UL RE-RECO #33725

[cmsbuild]

A new Pull Request was created by @bainbrid for CMSSW_10_6_X.

It involves the following packages:

PhysicsTools/PatAlgos
RecoEgamma/Configuration
RecoEgamma/EgammaElectronProducers

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@emilbols, @gouskos, @jainshilpi, @hatakeyamak, @rappoccio, @mbluj, @varuns23, @seemasharmafnal, @mmarionncern, @ahinzmann, @lgray, @jdolen, @ferencek, @Sam-Harper, @rovere, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @mariadalfonso, @clelange, @swozniewski, @JyothsnaKomaragiri, @sobhatta, @lecriste, @afiqaize, @gpetruc, @andrzejnovak, @ram1123 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

[bainbrid]

@crovelli @cavalari @jainshilpi @lfinco

[bainbrid]

@slava77 do I need to squash the commits?

[slava77]

@slava77 do I need to squash the commits?

It's welcome but not required.

[slava77]

test parameters:

• pull_request = LowPtElectrons: 2020Nov28 and 2021May17 models for 10_6_X cms-data/RecoEgamma-ElectronIdentification#19

[slava77]

@cmsbuild please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5690ec/14752/summary.html
COMMIT: 4911c44
CMSSW: CMSSW_10_6_X_2021-04-30-1100/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33589/14752/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: 102 differences found in the comparisons
• DQMHistoTests: Total files compared: 35
• DQMHistoTests: Total histograms compared: 3215544
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3215209
• DQMHistoTests: Total skipped: 334
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
• Checked 143 log files, 29 edm output root files, 35 DQM output files
• TriggerResults: no differences found

[slava77]

Reco comparison results: 102 differences found in the comparisons

all differences appear to be in 136.88811 (2018 data UL re-miniAOD). Changes in this workflow are not allowed/expected in this release. It looks like something is not protected by _devel or bParking

[cmsbuild]

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

[bainbrid]

Reco comparison results: 102 differences found in the comparisons

all differences appear to be in 136.88811 (2018 data UL re-miniAOD). Changes in this workflow are not allowed/expected in this release. It looks like something is not protected by _devel or bParking

Sorry, there was a typo.

Fixed here 683bf86.

Tested locally with 136.88811 (and can reproduce exactly the ID distribution now...)

[slava77]

@cmsbuild please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5690ec/14843/summary.html
COMMIT: 683bf86
CMSSW: CMSSW_10_6_X_2021-05-04-1100/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33589/14843/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: 4 differences found in the comparisons
• DQMHistoTests: Total files compared: 35
• DQMHistoTests: Total histograms compared: 3215556
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3215220
• DQMHistoTests: Total skipped: 334
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
• Checked 143 log files, 29 edm output root files, 35 DQM output files
• TriggerResults: no differences found

[slava77]

there are quite a few divergent points from what's in #32391 / master version. Backports should be maximally verbatim.

[slava77]

this part is reshuffled compared to #32391 https://github.com/cms-sw/cmssw/pull/32391/files#diff-301347daec1998142b2c5167fcec9c40bacb33d2622ae294648e9be73a34ee40L74-R86
please backport verbatim wherever possible

[bainbrid]

Modified to be verbatim w.r.t. master, caveat changes to handle modifiers (and their combinations) according to the TWiki linked in the PR description.

[bainbrid]

Here I've reverted back to the order originally used in this PR, as fitst more naturally with the existing code in 10_6_X and also the most recent changes I've made to deal with issues found during testing. I can copy this style to master as part of an upcoming PR (to master) that includes missing items / "forward ports".

[slava77]

is this part really needed: it seems more straightforward to keep the older version (the cut in L9/10) to stay unchanged.

Suggested change

	# Modifier for run2_miniAOD_UL (order is important: overrides bParking)
	from Configuration.ProcessModifiers.run2_miniAOD_UL_cff import run2_miniAOD_UL
	run2_miniAOD_UL.toModify(selectedPatLowPtElectrons,cut = "pt>1. && electronID('ID')>1.5")

[bainbrid]

Agree, reverted as suggested.

[slava77]

is this comment actually useful?
is it supposed to really refer to what's stored, or more about the module running (or somehow to the fact that the cut is now changed to "" in this PR?

[bainbrid]

Comment removed. (Was related to the cut that was changed to "", now also reverted.)

[slava77]

changes in this file seem unnecessary.
run2_miniAOD_devel will highly unlikely be applied by itself without either bParking or run2_miniAOD_UL already present.

please remove the commit editing this file if possible

[bainbrid]

'run2_miniAOD_devel' is required, as used in the following combination: (~bParking & run2_miniAOD_devel)
This modifier combination is defined in the TWiki linked in the PR description, which is "Recommended for future UL re-miniAOD of non-BParking data sets."

[slava77]

'run2_miniAOD_devel' is required, as used in the following combination: (~bParking & run2_miniAOD_devel)
This modifier combination is defined in the TWiki linked in the PR description, which is "Recommended for future UL re-miniAOD of non-BParking data sets."

but the point is that run2_miniAOD_devel is needed only where one has to extend the functionality beyond what is already provided by run2_miniAOD_UL

[bainbrid]

Please see my reply here: https://github.com/cms-sw/cmssw/pull/33589/files#r641937310

[slava77]

it would be nice(r) not to edit unrelated lines in a backport

[bainbrid]

Oops, reverted.

[slava77]

Suggested change

	run2_miniAOD_devel.toReplaceWith(lowPtGsfElectrons,_lowPtGsfElectrons)
	(bParking | run2_miniAOD_devel).toReplaceWith(lowPtGsfElectrons,_lowPtGsfElectrons)

[bainbrid]

Done.

[slava77]

Suggested change

bParking.toReplaceWith(lowPtGsfElectrons,_lowPtGsfElectrons)

see above (bParking | run2_miniAOD_devel).toReplaceWith

[bainbrid]

Done.

[slava77]

Suggested change

	bParking.toModify(_lowPtGsfElectrons,
	previousGsfElectronsTag = cms.InputTag("lowPtGsfElectronsPreRegression"),
	)
	bParking.toModify(_lowPtGsfElectrons,
	previousGsfElectronsTag = "lowPtGsfElectronsPreRegression"
	)

also, please move this up, before _lowPtGsfElectrons is used in toReplaceWith calls

[bainbrid]

Done.

[slava77]

this line better be moved before all the .toReplaceWith(lowPtGsfElectrons calls above

[bainbrid]

I don't think I need this line anymore?

Please can you check the updated version of this file to be sure it's organised correctly according to your suggestion.

[slava77]

I'd like to propose a less circular implementation with more similarity to what is done in the master version:

• define lowPtGsfElectronsPreRegression_cfi.py which would contain all of the above except for the module name changed to lowPtGsfElectronsPreRegression
• keep the rest of this file content in lowPtGsfElectrons_cfi.py
  • start with lowPtGsfElectrons = lowPtGsfElectronsPreRegression.clone()

[bainbrid]

Done.

I also updated these lines to read as 'lowPtGsfElectronsPreRegression.preselection' - I hope this is correct.

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5690ec/15677/summary.html
COMMIT: eb4bdf5
CMSSW: CMSSW_10_6_X_2021-06-04-2300/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33589/15677/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: 2 differences found in the comparisons
• DQMHistoTests: Total files compared: 35
• DQMHistoTests: Total histograms compared: 3215543
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3215207
• DQMHistoTests: Total skipped: 334
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
• Checked 143 log files, 29 edm output root files, 35 DQM output files
• TriggerResults: no differences found

[cmsbuild]

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

[bainbrid]

@slava77 in response to this:

I've just reviewed again all combinations of modifier logic required in 10_6_20 and this PR, and I have this:

None					Autumn18
bParking (only)				Autumn18
(bParking & ~run2_miniAOD_UL)		Autumn18
(~bParking & run2_miniAOD_UL)		2020Sept15
(~bParking & run2_miniAOD_devel) 	2020Nov28
(bParking & run2_miniAOD_UL) 		2021May17

So I believe your change (in conjunction with the remaining cfi) does indeed give the above.

The original logic (bParking | run2_miniAOD_UL) prior to this commit 2a63f28 used the 2020Sept15 model, but the bParking logic (alone) is supposed to use the obsolete Autumn18 model, as we are no longer changing RECO. Your change (should!) fix this.

Explicitly we could write in the cfi:

default -->				Autumn18
(~bParking & run2_miniAOD_UL)	-->	2020Sept15
(~bParking & run2_miniAOD_devel) -->	2020Nov28
(bParking & run2_miniAOD_UL) -->	2021May17

but the existing logic is effectively the same.

[slava77]

@cmsbuild please test

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5690ec/15680/summary.html
COMMIT: 2a63f28
CMSSW: CMSSW_10_6_X_2021-06-05-1100/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33589/15680/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: 4 differences found in the comparisons
• DQMHistoTests: Total files compared: 35
• DQMHistoTests: Total histograms compared: 3215543
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3215207
• DQMHistoTests: Total skipped: 334
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
• Checked 143 log files, 29 edm output root files, 35 DQM output files
• TriggerResults: no differences found

[slava77]

+reconstruction

for #33589 2a63f28

• code changes are in line with the PR description and the follow up review
• jenkins tests pass and comparisons with the baseline show no (relevant) differences
  • (no auto comparisons) manual checks for 136.898 and 1304.181 with this PR compared to that from tests of [Backport] Adding Xi-tracks to lostTracks to 10_6_X #33962 show that there are (finally) no changes in reco quantities
• local tests with run2_miniAOD_UL running on reco made in 10_6_25 appear to work OK

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_6_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_0_X is complete. 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)

[qliphy]

+1