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.

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

/validation/RecoEgamma/ Phase2 miniAOD modification v2 #33421

Merged
merged 3 commits into from Apr 27, 2021

Conversation

archiron
Copy link
Contributor

@archiron archiron commented Apr 13, 2021

PR description:

Implemented a workaround in the miniAOD validation to read the electron collections from two different collections (one for the barrel, one for the HGCAL). The values are adjusted for PHASE2 validation via an era modifier
Improved the binning in a few plots.

The modifications are made in Validation/RecoEGamma/plugins folder (ElectronMcMiniAODSignalValidator.cc/h) and into the Validation/RecoEGamma/python folder (ElectronMcSignalValidatorMiniAOD_cfi.py). We also modified ElectronMcSignalValidator_gedGsfElectrons_cfi.py, ElectronMcFakeValidator_gedGsfElectrons_cfi.py & ElectronMcSignalValidatorPt1000_gedGsfElectrons_cfi.py for binning.

We also made some corrections into DQMOffline/Egamma/python folder (electronDataDiscovery.py) by implementing a new function wich help us with local runs into Validation/RecoEgamma/tests folders (ElectronMcSignalPostValidation_cfg.py, ElectronMcSignalPostValidationMiniAOD_cfg.py, ElectronMcSignalValidationMiniAOD_cfg.py, ElectronMcSignalValidation_gedGsfElectrons_cfg.py, electronValidationCheck_Env.py).

miniAOD_corrections

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

for miniAOD improvement, the différences can be seen :
before : https://cms-egamma.web.cern.ch/validation/Electrons/Dev/index.php?action=/11_3_0_pre4_PHASE2_miniAOD3_DQM_dev/FullvsFull_CMSSW_11_3_0_pre4/RECO-miniAOD_ZEE_14#TOP
after : https://cms-egamma.web.cern.ch/validation/Electrons/Dev/index.php?action=/11_3_0_pre4_PHASE2_miniAOD6_DQM_dev/FullvsFull_CMSSW_11_3_0_pre4/RECO-miniAOD_ZEE_14#TOP

the binning improvement is only a regularization of the number of bins for histos with primary vertices (80 instead of 81). This avoid some drops into the curves of histograms.

PR validation:

compilation is OK, basics tests (scram b runtests or local tests) are OK too.
runTheMatrix.py -l limited -i all --ibeos tests are fine (38 37 36 27 17 4 1 1 1 tests passed, 0 0 0 0 0 0 0 0 0 failed
).
here is the run-log :
runall-report-step123-.log

Before submitting your pull requests, make sure you followed this checklist:

@beaudett

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33421/22076

  • This PR adds an extra 36KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33421/22077

  • This PR adds an extra 40KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @archiron (Chiron) for master.

It involves the following packages:

DQMOffline/EGamma
Validation/RecoEgamma

@andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @rvenditti can you please review it and eventually sign? Thanks.
@rovere, @lecriste, @rociovilar 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

@jfernan2
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-51693e/14212/summary.html
COMMIT: 8574892
CMSSW: CMSSW_11_3_X_2021-04-12-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33421/14212/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: 38
  • DQMHistoTests: Total histograms compared: 2865526
  • DQMHistoTests: Total failures: 135
  • DQMHistoTests: Total nulls: 152
  • DQMHistoTests: Total successes: 2865217
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -1.212 KiB( 37 files compared)
  • DQMHistoSizes: changed ( 10024.0,... ): -0.637 KiB EgammaV/ElectronMcSignalValidator
  • DQMHistoSizes: changed ( 10024.0,... ): -0.637 KiB EgammaV/ElectronMcSignalValidatorPt1000
  • DQMHistoSizes: changed ( 10024.0,... ): -0.004 KiB EgammaV/ElectronMcFakeValidator
  • DQMHistoSizes: changed ( 23234.0,... ): 4.805 KiB EgammaV/ElectronMcSignalValidatorMiniAOD
  • Checked 160 log files, 37 edm output root files, 38 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

@archiron could you please check in the following link if the changes in the DQM folders are what you expect?
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_11_3_X_2021-04-12-2300+51693e/42213/dqm-histo-comparison-summary.html
Thanks

@beaudett
Copy link
Contributor

Hi,
@archiron may have better chance, but I failed to find a file that contains miniAODs with all the histograms. Could you please point use to a file with miniAOD and for Phase2 ? Thanks

@archiron
Copy link
Contributor Author

perhaps whith:
runTheMatrix.py -n | grep D76 34634.0 2026D76+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal 34834.999 2026D76PU_PMXS1S2PR+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU
and 34634.0 seems to be a candidate.

@beaudett
Copy link
Contributor

and 34634.0 seems to be a candidate.

Right, but the statistics is way too low to judge.

@cmsbuild cmsbuild modified the milestones: CMSSW_11_3_X, CMSSW_12_0_X Apr 15, 2021
@jfernan2
Copy link
Contributor

jfernan2 commented Apr 26, 2021

What about running the wf privately setting the desired number of events you consider as enough stats?

@archiron
Copy link
Contributor Author

These plots have been made privately (but not with a standard workflow). Let me know if you want me to nevertheless run a fully standard workflow
standard plots for validation : https://cms-egamma.web.cern.ch/validation/Electrons/Releases/index.php?action=/11_3_0_pre4_PHASE2_DQM_std/FullvsFull_CMSSW_11_3_0_pre4/RECO-miniAOD_ZEE_14#TOP
plots made without correction : https://cms-egamma.web.cern.ch/validation/Electrons/Dev/index.php?action=/11_3_0_pre4_PHASE2_miniAOD3_DQM_dev/FullvsFull_CMSSW_11_3_0_pre4/RECO-miniAOD_ZEE_14#TOP
plots made with correction : https://cms-egamma.web.cern.ch/validation/Electrons/Dev/index.php?action=/11_3_0_pre4_PHASE2_miniAOD6_DQM_dev/FullvsFull_CMSSW_11_3_0_pre4/RECO-miniAOD_ZEE_14#TOP
all these plots are made with 9000 evts but 1000 evts may be sufficient.

@jfernan2
Copy link
Contributor

@archiron could you please check in the following link if the changes in the DQM folders are what you expect?
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_11_3_X_2021-04-12-2300+51693e/42213/dqm-histo-comparison-summary.html

Just to be clear: my question was the one above, I am not worried about stats as long as you confirm that seen changes are expected

@beaudett
Copy link
Contributor

Ho @jfernan2
according to @archiron there is only one Phase2 wf, and it has only one entry
https://tinyurl.com/ykyw42xh
If black is new and blue the reference, then there is one additional entry in the endcaps, while there was none before. In that case, it would be good. Now, this hint makes sense only if the GEN are identical.

@jfernan2
Copy link
Contributor

OK, but the PR is producing changes in wfs which are not phase 2, as seen on
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_11_3_X_2021-04-12-2300+51693e/42213/dqm-histo-comparison-summary.html
Do you only expect changes in Phase2 wfs?

@archiron
Copy link
Contributor Author

Oh, yes other changes are expected. Most of them are issue from the binning improvement in *recOfflineVertices histos i.e. histos with primary vertices (80 bins instead of 81). This avoid some drops into the curves of histograms such as :
nbins_corrections-a

@jfernan2
Copy link
Contributor

OK, I see, so then everything is understood unless you correct me. The full stats you privately analyzed confirm that, right?

@archiron
Copy link
Contributor Author

yes.

@jfernan2
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Apr 27, 2021

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants