[10_2_X] Syncing Egamma ID code with 10_4_X

[guitargeek]

This PR originally wanted to backport the Egamma Fall17 V2 Photon IDs and other Egamma MVA code improvements to 10_2_X. Since the ID functionality was more urgent than the rest of the development, it was decided to backport the Photon IDs and the PhotonIDValueMapProducer speedup separately from the rest in this PR: #25372.

What remains here is the backport of the other developments, which are in fact useful for analyses: updated PhotonMVANtuplizer, reduced memory usage, speed and FWlite compatibility for the electron MVAs.

To make things simpler to review, the following backport strategy was applied:

• Sync all files in RecoEgamma/EgammaTools, ElectronIdentification and PhotonIdentification with master
• Second commit reverting some changes which were actually not desired

This PR builds on top of #25372 and should only be tested and reviewed when that one and it's required external (cms-sw/cmsdist#4518) is merged.

[cmsbuild]

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

[guitargeek]

Hi @slava77, so if I understood correctly 10_2_X will be used for the production of something? Would it not be awesome to backport #25092 as well, since it speeds up Photon ID?

[guitargeek]

Don't test yet, I made a mistake and a commit will follow...

[cmsbuild]

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

[guitargeek]

Ok, things should be ready to test now with cms-sw/cmsdist#4518!

[guitargeek]

Local matrix tests passed.

[guitargeek]

@slava77 can you please check if I did the right thing with the externals and then tell the bot to test? It would be cool to know that we have at least something that works to continue the discussion with the NanoAOD guys, who would need to get this integrated very soon.

[slava77]

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

[cmsbuild]

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

[cmsbuild]

+1
Tested at: 86b413b
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25313/31795/summary.html

[cmsbuild]

Comparison job queued.

[cmsbuild]

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

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 329 differences found in the comparisons
• DQMHistoTests: Total files compared: 31
• DQMHistoTests: Total histograms compared: 2986766
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2986574
• 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

[cmsbuild]

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

[perrotta]

@guitargeek (and also @fabiocos and @slava77 , as I would like to ear what do they think about)

I don't think we can re-create 10_4 in 10_2. All the features of the new release will be accessible once that release will become available, and you'll ask people to move the analysis to it then.
This PR touches 95 files, with almost 6000 lines change. The risk of importing something odd with a 6k line update to a production release is not negligible, both when you code it, and also when we'll have to review it.

I would personally only stick on what is really needed/relevant for the analyses that will be done now, which I can't believe is the whole that is included in this backport. We are already backporting the new IDs and the code speed-up with #25372 . In your list, I may imagine that perhaps only the update of the ntuplizer has some reason to be backported now. All other improvements are welcome, but they will all be available in 10_4 in the new year, and you can instruct people to move their analyses to it, then.

Do you agree?

[andrius-k]

please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32057/console Started: 2018/12/07 15:17

[cmsbuild]

-1

Tested at: 38f3ee4

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25313/32057/summary.html

I found follow errors while testing this PR

Failed tests: Build ClangBuild

• Build:

I found compilation error when building:

>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/HLTTauCertifier.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/METplusTrackMonitor.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/RazorMonitor.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/DQMGenericTnPClient.cc 
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/LepHTMonitor.cc: In function 'bool {anonymous}::isGood(const reco::GsfElectron&, const Point&, const Point&, const edm::Handle >&, bool, double, double, double, double, double, double, double)':
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/LepHTMonitor.cc:49:86: error: no matching function for call to 'ConversionTools::hasMatchedConversion(const reco::GsfElectron&, const edm::Handle >&, const Point&)'
       pass_conversion = !ConversionTools::hasMatchedConversion(el, convs, bs_position);
                                                                                      ^
In file included from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/LepHTMonitor.cc:19:0:
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/RecoEgamma/EgammaTools/src/ConversionTools.cc:148:6: note: candidate: static bool ConversionTools::hasMatchedConversion(const reco::GsfElectron&, const ConversionCollection&, const XYZPoint&, bool, float, float, unsigned int)
 bool ConversionTools::hasMatchedConversion(const reco::GsfElectron &ele,


• Clang:

I found compilation error while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 16 COMPILER='llvm compile'

>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/DQMGenericTnPClient.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/HLTInclusiveVBFClient.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/HTMonitor.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/HLTTauDQMOfflineSource.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/SealModule.cc 
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/DQMOffline/Trigger/plugins/LepHTMonitor.cc:49:26: error: no matching function for call to 'hasMatchedConversion'
      pass_conversion = !ConversionTools::hasMatchedConversion(el, convs, bs_position);
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_2_X_2018-12-07-1100/src/RecoEgamma/EgammaTools/src/ConversionTools.cc:148:23: note: candidate function not viable: no known conversion from 'const edm::Handle' (aka 'const Handle >') to 'const reco::ConversionCollection' (aka 'const vector') for 2nd argument
bool ConversionTools::hasMatchedConversion(const reco::GsfElectron &ele,
                      ^


[cmsbuild]

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

[fabiocos]

@guitargeek @slava77 @perrotta I think that the scope and target of these backports needs to be discussed at the ORP. Having a new physics functionality usable for analyses before the UL reprocessing becomes available is one thing, but synchronizing the code base for any technical improvement seems to me too much.

[guitargeek]

Hello @fabiocos, @slava77 and @perrotta, I'm sorry I think I interpreted the following comment from Andrea wrong:

May you want to get back to the "full backport", then this pull request need to be updated, or even rebased if #25372 is merged first: it can be reviewed once again by reco in that case

I agree that we should no backport everything and if I could decide I would just not do any further backports and be done with it. As you say the physics functionality is already there, and if people really require the other features they can just ask me and we would just come back to it.

[perrotta]

-1

• @guitargeek my comment wanted to say that if you needed to backport something else than the new ID and the speed-up, then you could have re-opened this PR: frankly that did not consider a 6k line backport into a production release. Sorry for not having been clear.
  . I think we can forget this, and only backport anything else if really needed and if it does not interact too heavily with the rest of the code

[guitargeek]

Yep ok I will close for good now!