Change CandidateBoostedDoubleSecondaryVertexComputer to use GBRForest (76X)

[lgray]

Change the TMVA evaluator in CandidateBoostedDoubleSecondaryVertexComputer to use GBRForest for internal storage rather than TMVA::Reader.

Purely technical, no changes expected/observed.

[lgray]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.

[cmsbuild]

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

Change CandidateBoostedDoubleSecondaryVertexComputer to use GBRForest

It involves the following packages:

RecoBTag/SecondaryVertex

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@ferencek, @acaudron, @pvmulder, @imarches 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.

[ferencek]

@lgray, note that this tagger is not run in the standard reco and is expected to undergo significant changes in the coming weeks. However, since you are updating the existing code, you can remove the mutex and its locking since this is now handled internally by the TMVAEvaluator.

[lgray]

@ferencek OK, thanks. Wasn't sure what I should do since it's not const threadsafe (and didn't feel like walking through the code). I'll update later.

[cmsbuild]

+1
Tested at: a03847d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11607/8558/summary.html

[cmsbuild]

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

[lgray]

@ferencek FYI here is where I found this particular instance of using TMVA::Reader. If it is changing in the future that's fine, so long as TMVA::Reader doesn't use memory in the future!

https://slava77sk.web.cern.ch/slava77sk/reco/cgi-bin/igprof-navigator/CMSSW_7_6_X_2015-09-22-1500-orig.251721.DoubleEG.10evts.MEM_LIVE/565

[lgray]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.

[cmsbuild]

Pull request #11607 was updated. @cmsbuild, @cvuosalo, @slava77 can you please check and sign again.

[lgray]

@cvuosalo @slava77 This one shaves 3 MB off allocated memory, may see small changes in RSS but nothing earth shattering.

[cmsbuild]

+1
Tested at: 5051b18
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11607/8572/summary.html

[cmsbuild]

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

[ferencek]

@lgray, when updated, the plan is to use GBRForest. However, this will require a transient instance of TMVA::Reader to be created but that should not be a problem. Later we can also think about putting required payloads in the GT but that usually has a longer turnaround time.

[slava77]

@ferencek
Will you be able to "rebase" the upcoming developments on top of this PR?
If not and there is unwanted interference, I'm not sure this PR should go in then.
(better be the former)

[ferencek]

@slava77, developments haven't started yet so there should be no interference.

[lgray]

@ferencek Transient instances of TMVA::Reader are fine for me.

[slava77]

@lgray @ferencek it looks like the pfBoostedDoubleSecondary* btags are not running (it may be the secondaryVertex_cff is not loaded at the right time so that the jetTools can pick it up; but maybe it's intentional)

[slava77]

+1

for #11607 a03847d

• changes in the code are as expected: switch to using GBRForest for MVA evaluation runtime
  • however, there is actually no runtime call in the current workflows; the CandidateBoostedDoubleSecondaryVertexComputer constructor is still called (the corresponding ESProducer is in the list)
• jenkins tests pass and comparisons with baseline show no differences (trivial: the taggers are not called)
• local test in CMSSW_7_6_0_pre6 show about 3MB decrease in allocated memory

[cmsbuild]

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

[ferencek]

@slava77, pfBoostedDoubleSecondary* are not included the standard reco (possibly will be in some future release) so the fact they don't run is intentional.

[davidlange6]

+1

[slava77]

Right, I ended up looking at a wrong PAT config and confused myself that
these taggers were meant to be added.

... Why load the ES producer then? Maybe some cleanup is in order on this
side (unused jettag ES).

On Oct 6, 2015 12:01 AM, "Dinko Ferencek" [email protected] wrote:

@slava77, pfBoostedDoubleSecondary* are not included the standard reco
(possibly will be in some future release) so the fact they don't run is
intentional.

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