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

Jump to bottom

[12_4_X] Improved Egamma PFID model selection consistency #38372

Merged
merged 1 commit into from Jun 21, 2022
Merged

[12_4_X] Improved Egamma PFID model selection consistency #38372

merged 1 commit into from Jun 21, 2022

Conversation

valsdav
Copy link
Contributor

@valsdav valsdav commented Jun 14, 2022

PR description:

This PR solves the issue #38175.
The crash happened because the model selection by "eta" requirement was different in the ElectronDNNEstimator and in the GsfElectronProducer (one was using electron.eta, the other superCluster.eta). Now the model index is directly passed from the DNNHelper evaluator to the caller code, ensuring the consistency in the number of outputs. (Following comment #38175 (comment))

Moreover the electron model selection is now performed correctly with SuperCluster.eta instead of Electron.eta.

PR Validation:

The PR has been validated with local tests.

Release notes:

Backport of #38356
This is urgently needed for the 12_4_0 release.

@valsdav
Improved PFID model selection consistency.
acc34c7 
The model index used to evaluate the candidate is now saved in the DNNHelper output and used in the producer
to select how many DNN outputs should be saved, without performing again the pt/eta binning.

Moreover the eta selection is now performed with SuperCluster.eta instead of Electron.eta.
@valsdav
Copy link
Contributor Author

valsdav commented Jun 14, 2022

Backport of #38356

@cmsbuild cmsbuild added this to the CMSSW_12_4_X milestone Jun 14, 2022
@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 14, 2022 •
edited

A new Pull Request was created by @valsdav (Davide Valsecchi) for CMSSW_12_4_X.

It involves the following packages:

  • RecoEgamma/EgammaElectronProducers (reconstruction)
  • RecoEgamma/EgammaPhotonProducers (reconstruction)
  • RecoEgamma/EgammaTools (reconstruction)
  • RecoEgamma/ElectronIdentification (reconstruction)
  • RecoEgamma/PhotonIdentification (reconstruction)

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

@jpata
Copy link
Contributor

jpata commented Jun 15, 2022

@cmsbuild please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49b007/25532/summary.html
COMMIT: acc34c7
CMSSW: CMSSW_12_4_X_2022-06-14-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38372/25532/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

  • 1365.01365.0_BuToJpsiK_13+BuToJpsiK_13INPUT+DIGIUP15+RECOUP15+HARVESTUP15/step2_BuToJpsiK_13+BuToJpsiK_13INPUT+DIGIUP15+RECOUP15+HARVESTUP15.log

Comparison Summary

There are some workflows for which there are errors in the baseline:
136.8562 step 3
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: 14 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3680359
  • DQMHistoTests: Total failures: 14
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3680323
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Jun 16, 2022

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49b007/25556/summary.html
COMMIT: acc34c7
CMSSW: CMSSW_12_4_X_2022-06-15-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38372/25556/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: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3680359
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3680329
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Jun 17, 2022

+reconstruction

@cmsbuild
Copy link
Contributor

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

  • small changes to the MVA output are expected (OK in the backport without a modifier since it's a bugfix)

I tend to agree with @jpata on this
On the other end, 12_4_0 will be used for MC production, and technically the following 12_4_1 will potentially provide slightly different results if this PR is merged in it. Is it something that can be deemed acceptable?

PPD: @rappoccio , Kaori. Jordan

@perrotta
Copy link
Contributor

+1

  • Since the plan is now to use 12_4_1 (to be built next week) also for the MC production, and no production was started yet with 12_4_0, there is no reason any more to hold this PR.
  • It is intended that 12_4_0 will not used for any reconstruction: SIM-DIGIS produced with it will be reconstructed with the forthcoming 12_4_1

@cmsbuild cmsbuild merged commit 53f6024 into cms-sw:CMSSW_12_4_X Jun 21, 2022
@cmsbuild cmsbuild mentioned this pull request Jun 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
backport-ok fully-signed orp-approved reconstruction-approved tests-approved
Projects
None yet
Milestone
CMSSW_12_4_X
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@valsdav @cmsbuild @jpata @perrotta