Migrate VID BDTs to GlobalCache + GBRForest (76X) #10618

lgray · 2015-08-06T23:38:48Z

Forward port of #10556.

…roducer

cmsbuild · 2015-08-06T23:41:15Z

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_7_6_X.

Migrate VID BDTs to GlobalCache + GBRForest (76X)

It involves the following packages:

RecoEgamma/EgammaTools
RecoEgamma/ElectronIdentification
RecoEgamma/PhotonIdentification

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

lgray · 2015-08-06T23:43:24Z

@cmsbuild please test

cmsbuild · 2015-08-06T23:46:10Z

The tests are being triggered in jenkins.

cmsbuild · 2015-08-07T00:51:52Z

+1
Tested at: e87c12e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-10618/7035/summary.html

cmsbuild · 2015-08-07T01:58:40Z

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

slava77 · 2015-08-07T17:18:00Z

RecoEgamma/ElectronIdentification/plugins/ElectronMVAEstimatorRun2Phys14NonTrig.cc

-         << " EoP " << _allMVAVars.EoP
-         << " IoEmIoP " << _allMVAVars.IoEmIoP
-         << " eleEoPout " << _allMVAVars.eleEoPout
+	      << " fbrem "    << vars[11]  //_allMVAVars.fbrem


looking at this somewhat crazy implementation, I'm thinking if it were better to move to TMVAEvaluator ?
How many lines of code and places need to change if one is to insert a new variable.

TMVA evaluator isn't threadsafe.

-L

(Sent from my Nexus 6)
On Aug 7, 2015 19:18, "Slava Krutelyov" notifications@github.com wrote:

In
RecoEgamma/ElectronIdentification/plugins/ElectronMVAEstimatorRun2Phys14NonTrig.cc
#10618 (comment):

<< " mykfhits " << _allMVAVars.kfhits

<< " gsfchi2 " << _allMVAVars.gsfchi2

<< " deta " << _allMVAVars.deta

<< " dphi " << _allMVAVars.dphi

<< " detacalo " << _allMVAVars.detacalo

<< " see " << _allMVAVars.see

<< " spp " << _allMVAVars.spp

<< " etawidth " << _allMVAVars.etawidth

<< " phiwidth " << _allMVAVars.phiwidth

<< " OneMinusE1x5E5x5 " << _allMVAVars.OneMinusE1x5E5x5

<< " R9 " << _allMVAVars.R9

<< " HoE " << _allMVAVars.HoE

<< " EoP " << _allMVAVars.EoP

<< " IoEmIoP " << _allMVAVars.IoEmIoP

<< " eleEoPout " << _allMVAVars.eleEoPout

<< " fbrem " << vars[11] //_allMVAVars.fbrem

looking at this somewhat crazy implementation, I'm thinking if it were
better to move to TMVAEvaluator ?
How many lines of code and places need to change if one is to insert a new
variable.

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/10618/files#r36539926.

Take a look at #10543 .. maybe we can merge to something that fits both?

well, my concern here was mostly manual indexing. Maybe define enums instead?
The current row of vars[magic number] looks pretty bad

This for a debug statement. I don't use magic numbers in the rest of the
code.

-L

(Sent from my Nexus 6)
On Aug 7, 2015 19:31, "Slava Krutelyov" notifications@github.com wrote:

In
RecoEgamma/ElectronIdentification/plugins/ElectronMVAEstimatorRun2Phys14NonTrig.cc
#10618 (comment):

<< " mykfhits " << _allMVAVars.kfhits

<< " gsfchi2 " << _allMVAVars.gsfchi2

<< " deta " << _allMVAVars.deta

<< " dphi " << _allMVAVars.dphi

<< " detacalo " << _allMVAVars.detacalo

<< " see " << _allMVAVars.see

<< " spp " << _allMVAVars.spp

<< " etawidth " << _allMVAVars.etawidth

<< " phiwidth " << _allMVAVars.phiwidth

<< " OneMinusE1x5E5x5 " << _allMVAVars.OneMinusE1x5E5x5

<< " R9 " << _allMVAVars.R9

<< " HoE " << _allMVAVars.HoE

<< " EoP " << _allMVAVars.EoP

<< " IoEmIoP " << _allMVAVars.IoEmIoP

<< " eleEoPout " << _allMVAVars.eleEoPout

<< " fbrem " << vars[11] //_allMVAVars.fbrem

well, my concern here was mostly manual indexing. Maybe define enums
instead?
The current row of vars[magic number] looks pretty bad

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/10618/files#r36541165.

Retract that. OK I'll add some enums later.

-L

(Sent from my Nexus 6)
On Aug 7, 2015 19:33, "Lindsey Gray" lindsey.gray@gmail.com wrote:

This for a debug statement. I don't use magic numbers in the rest of the
code.

-L

(Sent from my Nexus 6)
On Aug 7, 2015 19:31, "Slava Krutelyov" notifications@github.com wrote:

In
RecoEgamma/ElectronIdentification/plugins/ElectronMVAEstimatorRun2Phys14NonTrig.cc
#10618 (comment):

<< " mykfhits " << _allMVAVars.kfhits

<< " gsfchi2 " << _allMVAVars.gsfchi2

<< " deta " << _allMVAVars.deta

<< " dphi " << _allMVAVars.dphi

<< " detacalo " << _allMVAVars.detacalo

<< " see " << _allMVAVars.see

<< " spp " << _allMVAVars.spp

<< " etawidth " << _allMVAVars.etawidth

<< " phiwidth " << _allMVAVars.phiwidth

<< " OneMinusE1x5E5x5 " << _allMVAVars.OneMinusE1x5E5x5

<< " R9 " << _allMVAVars.R9

<< " HoE " << _allMVAVars.HoE

<< " EoP " << _allMVAVars.EoP

<< " IoEmIoP " << _allMVAVars.IoEmIoP

<< " eleEoPout " << _allMVAVars.eleEoPout

<< " fbrem " << vars[11] //_allMVAVars.fbrem

well, my concern here was mostly manual indexing. Maybe define enums
instead?
The current row of vars[magic number] looks pretty bad

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/10618/files#r36541165.

lgray · 2015-08-07T17:22:00Z

Or, I should say it is threadsafe but you pay for the mutex. Likewise tmva evaluator doesn't yet support gbrforest which is another point of this pr.

lgray · 2015-08-07T17:23:45Z

Ah, didn't see it that pr. Would rather bring that in as an upgrade later.

Dr15Jones · 2015-08-07T17:28:56Z

Don't we also pay a heavy memory tax for TMVA since we switched to ROOT 6?

lgray · 2015-08-07T17:29:02Z

And indeed, changing TMVA evaluatour to be reentrant may require some work. It is pretty stateful right now.

Would definitely prefer to bring as an update in a later PR. Switching now would degrade memory use and thread efficiency in threaded mode.

davidlange6 · 2015-08-07T17:31:48Z

nothing in tmva is different in root6…

On Aug 7, 2015, at 7:28 PM, Chris Jones notifications@github.com wrote:

Don't we also pay a heavy memory tax for TMVA since we switched to ROOT 6?

—
Reply to this email directly or view it on GitHub.

Dr15Jones · 2015-08-07T17:35:10Z

I thought TMVA used the internal ROOT 'TFormula' which under the hood used CINT in ROOT5 and now cling in ROOT 6. So TMVA itself didn't change but I thought what it depends on did.

slava77 · 2015-08-07T18:30:13Z

RecoEgamma/ElectronIdentification/plugins/ElectronMVAEstimatorRun2Phys14NonTrig.cc

+                                        allMVAVars.etawidth,
+                                        allMVAVars.phiwidth,
+                                        allMVAVars.HoE,
+                                        allMVAVars.kfchi2,


this ~20-line copy-paste with only one line change is yet another poorly maintainable piece.

slava77 · 2015-08-07T18:33:12Z

RecoEgamma/PhotonIdentification/interface/PhotonMVAEstimatorRun2Phys14NonTrig.h

  // Call this function once per event to retrieve all needed
  // event content pices
-  void getEventContent(const edm::Event& iEvent) override;
+  // DEPRECATED
+  // void getEventContent(const edm::Event& iEvent) const override;


deprecated and commented out means it's not needed anymore and can be removed.
What about other "DEPRECATED" that are still used??

It's final in the base class, can't override it anymore.

(Sent from my Nexus 6)
On Aug 7, 2015 8:33 PM, "Slava Krutelyov" notifications@github.com wrote:

In
RecoEgamma/PhotonIdentification/interface/PhotonMVAEstimatorRun2Phys14NonTrig.h
#10618 (comment):

// Call this function once per event to retrieve all needed
// event content pices

void getEventContent(const edm::Event& iEvent) override;

// DEPRECATED

// void getEventContent(const edm::Event& iEvent) const override;

deprecated and commented out means it's not needed anymore and can be
removed.
What about other "DEPRECATED" that are still used??

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/10618/files#r36547535.

remove it here, then?

slava77 · 2015-08-07T20:07:04Z

+1

for #10618 e87c12e

code changes for the migration to GBRForest delivers the desired change;
there is quite a bit of code replication pairing variables, names, and generic array with the variable values. This is expected to be updated separately, as discussed partly here partly on skype
jenkins tests complete and comparisons with the baseline show no difference (as intended from this code migration/refactoring)
local tests to check memory use were done in CMSSW_7_6_X_2015-08-06-2300 /test area sign577/ on 200 events from 251721 (DoubleMuon-Run2015B-251721 44C77595*.root file):
- the net reduction in memory (on event 10) is 45 MB:
  - about 7.5 MB in the electron MVAValueMapProducer
  - about 37 MB in the two photon MVAValueMapProducers

cmsbuild · 2015-08-07T20:11:18Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_6_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

davidlange6 · 2015-08-08T07:00:09Z

+1

Migrate VID BDTs to GlobalCache + GBRForest (76X)

lgray added 4 commits August 7, 2015 01:38

use a global cache for MVA Estimators

6c14dec

fix buildfile errors and revert to older version of GBR forest

11f5d26

make sure to use objects from global cache everywhere in MVAValueMapP…

d7cba74

…roducer

remove unused variable per clang warnings

e87c12e

cmsbuild added this to the Next CMSSW_7_6_X milestone Aug 6, 2015

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 6, 2015

cmsbuild added tests-started and removed tests-pending labels Aug 6, 2015

cmsbuild added tests-approved and removed tests-started labels Aug 7, 2015

lgray mentioned this pull request Aug 7, 2015

EGM IDs for MiniAOD Reprocessing (76X) #10619

Merged

cmsbuild added comparison-available and removed comparison-pending labels Aug 7, 2015

slava77 reviewed Aug 7, 2015
View reviewed changes

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Aug 7, 2015

cmsbuild added orp-approved and removed orp-pending labels Aug 8, 2015

cmsbuild added a commit that referenced this pull request Aug 8, 2015

Merge pull request #10618 from lgray/vid_mvas_global_cache_76X

8010f82

Migrate VID BDTs to GlobalCache + GBRForest (76X)

cmsbuild merged commit 8010f82 into cms-sw:CMSSW_7_6_X Aug 8, 2015

This was referenced Aug 10, 2015

Migrate VID BDTs to GlobalCache + GBRForest (75X) #10557

Merged

EGM IDs for MiniAOD Reprocessing (75X) #10599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate VID BDTs to GlobalCache + GBRForest (76X) #10618

Migrate VID BDTs to GlobalCache + GBRForest (76X) #10618

lgray commented Aug 6, 2015

cmsbuild commented Aug 6, 2015

lgray commented Aug 6, 2015

cmsbuild commented Aug 6, 2015

cmsbuild commented Aug 7, 2015

cmsbuild commented Aug 7, 2015

slava77 Aug 7, 2015

lgray Aug 7, 2015

slava77 Aug 7, 2015

slava77 Aug 7, 2015

lgray Aug 7, 2015

lgray Aug 7, 2015

lgray commented Aug 7, 2015

lgray commented Aug 7, 2015

Dr15Jones commented Aug 7, 2015

lgray commented Aug 7, 2015

davidlange6 commented Aug 7, 2015

Dr15Jones commented Aug 7, 2015

slava77 Aug 7, 2015

slava77 Aug 7, 2015

lgray Aug 7, 2015

slava77 Aug 7, 2015

slava77 commented Aug 7, 2015

cmsbuild commented Aug 7, 2015

davidlange6 commented Aug 8, 2015

Migrate VID BDTs to GlobalCache + GBRForest (76X) #10618

Migrate VID BDTs to GlobalCache + GBRForest (76X) #10618

Conversation

lgray commented Aug 6, 2015

cmsbuild commented Aug 6, 2015

lgray commented Aug 6, 2015

cmsbuild commented Aug 6, 2015

cmsbuild commented Aug 7, 2015

cmsbuild commented Aug 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgray commented Aug 7, 2015

lgray commented Aug 7, 2015

Dr15Jones commented Aug 7, 2015

lgray commented Aug 7, 2015

davidlange6 commented Aug 7, 2015

Dr15Jones commented Aug 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented Aug 7, 2015

cmsbuild commented Aug 7, 2015

davidlange6 commented Aug 8, 2015