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

[10_2_X] Fall17 V2 Photon ID and PhotonIDValueMapProducer speedup #25372

Merged
merged 13 commits into from Dec 12, 2018
Merged

[10_2_X] Fall17 V2 Photon ID and PhotonIDValueMapProducer speedup #25372

merged 13 commits into from Dec 12, 2018

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Nov 29, 2018

Primarily, this is the backport of #25293 plus changes to make Photon MVA V2 work before #24131 and adapting to new weights files names.

Additionally, since the NanoAOD developers would really appreciate the PhotonIDValueMapProducer speedup from #25092 to be backported to 10_2_X. Here, I suggest to simply synchronize the PhotonIDValueMapProducer (and required new header files) with master to get the speedup.

Validation plots for all photon ID value maps:
https://rembserj.web.cern.ch/rembserj/plots/CMSSW_PRs/25372/

  • target: CMSSW_10_2_6 plus this PR and the external PR
  • reference: CMSSW_10_4_X_2018-12-03-1100 and CMSSW_10_2_6

Validation workflow:

  • Dump all relevant ID value maps to ROOT file with this configuration [1], running on DY+Jets
  • Compare ROOT file from reference and target with this script [2]

Depends on cms-sw/cmsdist#4518.

[1] https://github.com/guitargeek/cmssw/blob/f76b6a2c9425d5ad5df91df5ef04272d0ab40e39/RecoEgamma/EgammaTools/test/testPhotonIDs_cfg.py
[2] https://github.com/guitargeek/cmssw/blob/f76b6a2c9425d5ad5df91df5ef04272d0ab40e39/RecoEgamma/EgammaTools/test/photon_validation_10_2_6.py

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @guitargeek (Jonas Rembser) for CMSSW_10_2_X.

It involves the following packages:

PhysicsTools/PatAlgos
RecoEgamma/PhotonIdentification

@perrotta, @monttj, @cmsbuild, @fgolf, @slava77, @peruzzim can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @jainshilpi, @rappoccio, @HeinerTholen, @varuns23, @seemasharmafnal, @mmarionncern, @imarches, @ahinzmann, @smoortga, @acaudron, @lgray, @jdolen, @drkovalskyi, @ferencek, @Sam-Harper, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @clelange, @JyothsnaKomaragiri, @mverzett, @gpetruc, @mariadalfonso, @pvmulder this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@peruzzim
Copy link
Contributor

@guitargeek I leave to reconstruction and release managers the discussion on how to do the backport technically.

From the NanoAOD production perspective, the part that is on top of #25347 should be completely transparent.
Looking at commits it seems that this PR builds on top of that one at fde7b8e, does not just depend on it.

Thanks

@guitargeek
Copy link
Contributor Author

Hi Marco, yep it will be discussed today in the RECO meeting. But can you please explain again what you mean with "the part that is on top of #25347 should be completely transparent"? Thanks!

@guitargeek
Copy link
Contributor Author

Hi @slava77 and @perrotta, so did we agree in the end on the minimal backport solution actually? For 10_2_X, that would mean #25347 and this PR which I'd kindly ask to get tested.

About the minimal backport of V2 IDs to 9_4_X: I was not entirely correct with my assessment this afternoon that it would only require new config files. In reality, the variables are slightly different, as the V2 ID takes the absolute value of track-cluster matching variables [2] while V1 does not [1]. That means an additional if here [3].

I suppose I'll just quickly set up the minimal backport to 9_4_X and make a PR, so it can be further discussed there.

Jonas

[1] https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/ElectronIdentification/data/ElectronMVAEstimatorRun2Fall17V1Variables.txt#L36
[2] https://github.com/cms-sw/cmssw/blob/master/RecoEgamma/ElectronIdentification/data/ElectronMVAEstimatorRun2Variables.txt#L36
[3] https://github.com/cms-sw/cmssw/blob/CMSSW_9_4_X/RecoEgamma/ElectronIdentification/src/ElectronMVAEstimatorRun2Fall17.cc#L357

@guitargeek
Copy link
Contributor Author

guitargeek commented Dec 1, 2018

Ok @perrotta, @slava77, @Sam-Harper and @michelif, again about the 9_4_X backport. I have the minimal backport set up now [1].

In the end, it was not so trivial as expected, since we also need a new cut class for the cut ID and for the electron MVA, I need also to produce the BDT output before the logistic transformation now. Please tell me which option you prefer and I will validate it and make the backport PR:

  • Option 1: make minimal changes necessary to get V2 IDs [1].
  • Option 2: backport all Egamma ID developments up to the FWlite support [2].

[1] https://github.com/guitargeek/cmssw/tree/EgammaID_9_4_12_minimal
[2] https://github.com/guitargeek/cmssw/tree/EgammaID_FWlite_949

@perrotta
Copy link
Contributor

perrotta commented Dec 1, 2018

@cmsbuild please test with cms-sw/cmsdist#4518

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2018

The tests are being triggered in jenkins.
Using externals from cms-sw/cmsdist#4518
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31931/console

@perrotta
Copy link
Contributor

perrotta commented Dec 1, 2018

@guitargeek about the backport to 94X, I think we agreed on the "minimal support", and I would concentrate on it by now. The "full backport" can be arranged for a later stage, if needed.

About currently open pull request for 10_2_X: since we agreed about backporting both V2 IDs and the code speed up, this is the pull request which contains them. At this point, #25313 is superseeded, and #25347 is only a subsample of this one, and therefore redundant. I think you can close both them, now.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

Pull request #25372 was updated. @perrotta, @andrius-k, @kmaeshima, @schneiml, @cmsbuild, @jfernan2, @fgolf, @slava77, @peruzzim can you please check and sign again.

@guitargeek
Copy link
Contributor Author

Okay @peruzzim , I tested some workflows with MiniAOD locally and they pass. It should be safe now to test with #25444 cms-sw/cmsdist#4518. Sorry for the initial problems with this.

@peruzzim
Copy link
Contributor

please test with #25444 cms-sw/cmsdist#4518

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 11, 2018

The tests are being triggered in jenkins.
Using externals from cms-sw/cmsdist#4518
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32112/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25372/32112/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 3007299
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3007106
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 129 log files, 14 edm output root files, 31 DQM output files

@peruzzim
Copy link
Contributor

+xpog

for the change to PhysicsTools/NanoAOD/python/photons_cff.py that is just a file naming change

@andrius-k
Copy link

+1

@perrotta
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit ccc1a22 into cms-sw:CMSSW_10_2_X Dec 12, 2018
@guitargeek guitargeek deleted the PhotonIDValueMapProducer_10_2_X branch December 12, 2018 10:40
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

8 participants