Improve ElectronIDExternalProducer #46868

Dr15Jones · 2024-12-04T16:48:27Z

PR description:

made it a global module
updated helpers to have const member functions
reduced memory churn by setting values from ParameterSet during
construction
fixed a 10 year old bug for case where CutBasedElectronID is supposed to retrieve the BeamSpot

PR validation:

Code compiles. Running a trivial RECO workflow did succeed.

NOTE: this should be a purely technical change and is not expected to change results for the cases we use.

This WILL change behavior IF an old version is used, but it returns the code to the behavior it had before the bug was introduced at the time this code was updated to call consumes.

fixes #46866

- made it a global module - updated helpers to have const member functions - reduced memory churn by setting values from ParameterSet during construction

Properly do consumes for beam spot if needed. Switched to Event::get methods which will properly type check.

Dr15Jones · 2024-12-04T16:48:37Z

please test

cmsbuild · 2024-12-04T16:48:55Z

cms-bot internal usage

cmsbuild · 2024-12-04T16:50:27Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46868/42897

cmsbuild · 2024-12-04T16:50:54Z

A new Pull Request was created by @Dr15Jones for master.

It involves the following packages:

RecoEgamma/ElectronIdentification (reconstruction)

@jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@Prasant1993, @Sam-Harper, @a-kapoor, @afiqaize, @jainshilpi, @lgray, @missirol, @ram1123, @sameasy, @sobhatta, @valsdav, @varuns23 this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2024-12-04T16:53:19Z

There is an additional change I'd like to do. I would like to change CutBasedElectronID so it uses the strategy pattern for the versioning. That is, distinct value of version_ would be handled via polymorphism instead of large number of if statements. This would allow the parameters to be extracted at construction time instead of being called for each electron individually (which is time and memory intensive).

cmsbuild · 2024-12-04T18:43:38Z

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-896277/43245/summary.html
COMMIT: 2f9814e
CMSSW: CMSSW_15_0_X_2024-12-04-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46868/43245/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

2024.303001DAS Error
2024.300001DAS Error
2024.203001DAS Error

Expand to see more relval errors ...

2024.103001
2024.302001
2024.101001
2024.102001
2024.100001

Comparison Summary

There are some workflows for which there are errors in the baseline:
2024.202001 step 1
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

No significant changes to the logs found
Reco comparison results: 39 differences found in the comparisons
DQMHistoTests: Total files compared: 45
DQMHistoTests: Total histograms compared: 3393039
DQMHistoTests: Total failures: 1873
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3391146
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 44 files compared)
Checked 198 log files, 168 edm output root files, 45 DQM output files
TriggerResults: no differences found

jfernan2 · 2024-12-05T09:36:06Z

please test

cmsbuild · 2024-12-05T11:09:07Z

+1

Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-896277/43269/summary.html
COMMIT: 2f9814e
CMSSW: CMSSW_15_0_X_2024-12-04-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46868/43269/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
2022.000001 step 1
2022.002001 step 1
2023.002001 step 1
2024.000001 step 1
2024.101001 step 1
2024.202001 step 1
2024.303001 step 1
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

No significant changes to the logs found
Reco comparison results: 32 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 2957317
DQMHistoTests: Total failures: 1526
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2955771
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 174 log files, 144 edm output root files, 39 DQM output files
TriggerResults: found differences in 1 / 37 workflows

jfernan2 · 2024-12-05T15:14:25Z

There is an additional change I'd like to do. I would like to change CutBasedElectronID so it uses the strategy pattern for the versioning. That is, distinct value of version_ would be handled via polymorphism instead of large number of if statements. This would allow the parameters to be extracted at construction time instead of being called for each electron individually (which is time and memory intensive).

@Dr15Jones are you going to include that change in this PR? Thanks

Dr15Jones · 2024-12-05T15:16:48Z

are you going to include that change in this PR? Thanks

I'm working on the changes but they are extensive. I'll probably have the code done today but I'm still trying to figure out a good way to test.

Sam-Harper · 2024-12-05T16:11:40Z

Before too much effort is expended on this. My understanding is that this module died in Run 1. Is this being used somewhere? E/gamma has used VID since Run2 and this is deprecated to my understanding. Is there something I miss? Can we not just delete it?

Dr15Jones · 2024-12-05T17:01:23Z

I discovered this module by running a Run3 workflow. The configuration header had

# Source: /local/reps/CMSSW/CMSSW/Configuration/Applications/python/ConfigBuilder.py,v 
# with command line options: step3 -s RAW2DIGI,L1Reco,RECO,RECOSIM,SKIM:LogError+LogErrorMonitor,PAT,NANO,DQM:@standardDQM+@ExtraHLT+@miniAODDQM+@nanoAODDQM --conditions auto:phase1_2024_realistic --datatier RECO,MINIAODSIM,NANOAODSIM,DQMIO -n 10 --eventcontent RECOSIM,MINIAODSIM,NANOEDMAODSIM,DQM --geometry DB:Extended --era Run3_2024 --customise Validation/Performance/TimeMemorySummary.customiseWithTimeMemorySummary --filein file:step2.root --fileout file:step3.root

jfernan2 · 2024-12-05T18:41:56Z

FYI: @SanghyunKo @cochando

cmsbuild · 2024-12-07T14:40:27Z

Pull request #46868 was updated. @jfernan2, @mandrenguyen can you please check and sign again.

cmsbuild · 2024-12-07T15:34:23Z

-1

Failed Tests: RelVals RelVals-INPUT AddOn
Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-896277/43309/summary.html
COMMIT: 342e295
CMSSW: CMSSW_15_0_X_2024-12-07-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46868/43309/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

135.4135.4_ZEEFS_13/step1_ZEEFS_13.log
9.09.0_Higgs200ChargedTaus/step3_Higgs200ChargedTaus.log
1306.01306.0_SingleMuPt1_UP15/step3_SingleMuPt1_UP15.log

Expand to see more relval errors ...

RelVals-INPUT

159.01159.01_HydjetQ_reminiaodPbPb2022_INPUT/step2_HydjetQ_reminiaodPbPb2022_INPUT.log
2500.0242500.024_NANOmcUL18reMINI/step2_NANOmcUL18reMINI.log
2500.0232500.023_NANOmcUL17reMINI/step2_NANOmcUL17reMINI.log

Expand to see more relval errors ...

2500.022
2500.021
2024.003001
2024.100001

AddOn Tests

fastsim

UNKNOWN

fastsim1

UNKNOWN

fastsim2

UNKNOWN

Dr15Jones · 2024-12-07T17:08:32Z

The jobs are dying in python saying Validation.HGCalValidation.hgcalSimHitValidationEE_cfi is missing.

Dr15Jones · 2024-12-08T13:30:08Z

please test

cmsbuild · 2024-12-08T15:00:41Z

+1

Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-896277/43318/summary.html
COMMIT: 342e295
CMSSW: CMSSW_15_0_X_2024-12-08-0000/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46868/43318/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 2 lines to the logs
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 46
DQMHistoTests: Total histograms compared: 3510017
DQMHistoTests: Total failures: 417
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3509580
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
Checked 202 log files, 172 edm output root files, 46 DQM output files
TriggerResults: no differences found

jfernan2 · 2024-12-09T12:09:02Z

@lnurfikri89 as JetMET RECO contact, please comment on;

So on this, I think we need to understand what JetMET are using this ancient ID for and whether they could move to another ID

@cms-sw/jetmet-pog-l2

Dr15Jones · 2024-12-09T15:04:02Z

The comparisons from the test all look very good. The only differences are the standard HGCal inconsistencies seen in IB and PR tests.

jfernan2 · 2024-12-10T18:46:44Z

type egamma,jetmet
Waiting for JetMET and EGM conveners to comment

jfernan2 · 2024-12-10T18:47:14Z

assign jetmet-pog

cmsbuild · 2024-12-10T18:47:26Z

New categories assigned: jetmet-pog

@alkaloge,@kirschen,@miquork you have been requested to review this Pull request/Issue and eventually sign? Thanks

jfernan2 · 2024-12-10T18:47:26Z

assign egamma-pog

cmsbuild · 2024-12-10T18:47:33Z

New categories assigned: egamma-pog

@afiqaize,@RSalvatico you have been requested to review this Pull request/Issue and eventually sign? Thanks

nurfikri89 · 2024-12-11T11:59:27Z

@lnurfikri89 as JetMET RECO contact, please comment on;

So on this, I think we need to understand what JetMET are using this ancient ID for and whether they could move to another ID
@cms-sw/jetmet-pog-l2

@jfernan2 My apologies for taking a while to get back to you. I did not get a notification from github.

The JetPlusTrack collection (slimmedJPTJets) is not an official jet collection but it is included and maintained by @kodolova.

Hi @kodolova, there is a recommendation in this PR to remove ElectronIDExternalProducer [1], which produces very old electron IDs that are not supported by EGM. The JPTeidTight ID is consumed by JetPlusTrackProducer. Can you comment on this?

[1] #46868 (comment)

kodolova · 2024-12-11T13:05:28Z

Hi, We can use any other ElectronIDProducer. What is proposed to use?

SanghyunKo · 2024-12-11T15:08:53Z

@kodolova Hi, FYI, the default EGM cut-based IDs are produced using a VID module named egmGsfElectronIDs, and the names of each (Run 3) ID is listed in the cutBasedElectronID_Winter22_122X_V1_cff.py. So the name of the InputTag should be like egmGsfElectronIDs:cutBasedElectronID-RunIIIWinter22-V1-medium (or veto, loose, tight, depending on your needs).

kodolova · 2024-12-12T08:16:03Z

That's ok. I will try to change.

Dr15Jones added 2 commits December 4, 2024 09:02

Improved ElectronIDExternalProducer

5607b8a

- made it a global module - updated helpers to have const member functions - reduced memory churn by setting values from ParameterSet during construction

Fix retrieval of beam spot in CutBasedElectronID

2f9814e

Properly do consumes for beam spot if needed. Switched to Event::get methods which will properly type check.

cmsbuild added this to the CMSSW_15_0_X milestone Dec 4, 2024

cmsbuild added reconstruction-pending pending-signatures orp-pending tests-started code-checks-pending labels Dec 4, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 4, 2024

cmsbuild added tests-rejected and removed tests-started labels Dec 4, 2024

makortel mentioned this pull request Dec 4, 2024

Improve ElectronIDExternalProducer cms-sw/framework-team#1102

Open

cmsbuild added tests-started and removed tests-rejected labels Dec 5, 2024

cmsbuild added tests-approved and removed tests-started labels Dec 5, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 7, 2024

cmsbuild added tests-rejected and removed tests-started labels Dec 7, 2024

cmsbuild added tests-started and removed tests-rejected labels Dec 8, 2024

cmsbuild added tests-approved and removed tests-started labels Dec 8, 2024

SanghyunKo mentioned this pull request Dec 10, 2024

Logic bug in CutBasedElectronID #46865

Open

cmsbuild added the jetmet-pog-pending label Dec 10, 2024

cmsbuild added egamma-pog-pending egamma jetmet labels Dec 10, 2024

jfernan2 mentioned this pull request Dec 13, 2024

Change ElectronID to the latest in python configuration for JetPlusTrack algoritm #46934

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve ElectronIDExternalProducer #46868

Improve ElectronIDExternalProducer #46868

Dr15Jones commented Dec 4, 2024 •

edited

Loading

Dr15Jones commented Dec 4, 2024

cmsbuild commented Dec 4, 2024 •

edited

Loading

cmsbuild commented Dec 4, 2024

cmsbuild commented Dec 4, 2024

Dr15Jones commented Dec 4, 2024

cmsbuild commented Dec 4, 2024

jfernan2 commented Dec 5, 2024

cmsbuild commented Dec 5, 2024

jfernan2 commented Dec 5, 2024

Dr15Jones commented Dec 5, 2024

Sam-Harper commented Dec 5, 2024

Dr15Jones commented Dec 5, 2024

jfernan2 commented Dec 5, 2024

cmsbuild commented Dec 7, 2024

cmsbuild commented Dec 7, 2024

Dr15Jones commented Dec 7, 2024

Dr15Jones commented Dec 8, 2024

cmsbuild commented Dec 8, 2024

jfernan2 commented Dec 9, 2024

Dr15Jones commented Dec 9, 2024

jfernan2 commented Dec 10, 2024 •

edited

Loading

jfernan2 commented Dec 10, 2024

cmsbuild commented Dec 10, 2024

jfernan2 commented Dec 10, 2024

cmsbuild commented Dec 10, 2024

nurfikri89 commented Dec 11, 2024

kodolova commented Dec 11, 2024

SanghyunKo commented Dec 11, 2024

kodolova commented Dec 12, 2024

Improve ElectronIDExternalProducer #46868

Are you sure you want to change the base?

Improve ElectronIDExternalProducer #46868

Conversation

Dr15Jones commented Dec 4, 2024 • edited Loading

PR description:

PR validation:

Dr15Jones commented Dec 4, 2024

cmsbuild commented Dec 4, 2024 • edited Loading

cmsbuild commented Dec 4, 2024

cmsbuild commented Dec 4, 2024

Dr15Jones commented Dec 4, 2024

cmsbuild commented Dec 4, 2024

RelVals-INPUT

Comparison Summary

jfernan2 commented Dec 5, 2024

cmsbuild commented Dec 5, 2024

Comparison Summary

jfernan2 commented Dec 5, 2024

Dr15Jones commented Dec 5, 2024

Sam-Harper commented Dec 5, 2024

Dr15Jones commented Dec 5, 2024

jfernan2 commented Dec 5, 2024

cmsbuild commented Dec 7, 2024

cmsbuild commented Dec 7, 2024

RelVals

RelVals-INPUT

AddOn Tests

Dr15Jones commented Dec 7, 2024

Dr15Jones commented Dec 8, 2024

cmsbuild commented Dec 8, 2024

Comparison Summary

jfernan2 commented Dec 9, 2024

Dr15Jones commented Dec 9, 2024

jfernan2 commented Dec 10, 2024 • edited Loading

jfernan2 commented Dec 10, 2024

cmsbuild commented Dec 10, 2024

jfernan2 commented Dec 10, 2024

cmsbuild commented Dec 10, 2024

nurfikri89 commented Dec 11, 2024

kodolova commented Dec 11, 2024

SanghyunKo commented Dec 11, 2024

kodolova commented Dec 12, 2024

Dr15Jones commented Dec 4, 2024 •

edited

Loading

cmsbuild commented Dec 4, 2024 •

edited

Loading

jfernan2 commented Dec 10, 2024 •

edited

Loading