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

cMVAv2 negative/positive configuration files #13014

Conversation

jpata
Copy link
Contributor

@jpata jpata commented Jan 20, 2016

Adds configuration files for negative and positive cMVAv2 b-tagger needed for scale factor measurements. Does not affect reco sequences or event content.

Replaces #13007.
scram b runtests was successful. No more merge conflicts.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @jpata (Joosep Pata) for CMSSW_8_0_X.

It involves the following packages:

PhysicsTools/PatAlgos
RecoBTag/Combined

@cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please review it and eventually sign? Thanks.
@rappoccio, @imarches, @ahinzmann, @acaudron, @mmarionncern, @jdolen, @nhanvtran, @schoef, @ferencek, @gpetruc, @mariadalfonso, @pvmulder, @TaiSakuma this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@cvuosalo
Copy link
Contributor

@jpata: Please add to the PR description an explanation of the purpose of this PR and what differences it will create in event content and reconstructed quantities.

@jpata
Copy link
Contributor Author

jpata commented Jan 21, 2016

I have added some description. Please let me know if you need anything else from my side.

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/10666/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@@ -11,6 +11,16 @@
from RecoBTag.Combined.pfNegativeCombinedMVABJetTags_cfi import *
from RecoBTag.Combined.pfPositiveCombinedMVABJetTags_cfi import *

# CombinedMVA V2
#combined MVA V2
from RecoBTag.Combined.combinedMVAV2Computer_cfi import *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you should only add JetTag producers. All the computers need to go into RecoBTag/Combined/python/combinedMVA_EventSetup_cff.py (see #12848 for reference).

Also, don't forget to add this PR to https://twiki.cern.ch/twiki/bin/view/CMS/BTagSoftwarePullRequests Thanks

@cvuosalo
Copy link
Contributor

@jpata: Please note this PR now has merge conflicts.

@slava77
Copy link
Contributor

slava77 commented Jan 28, 2016

@pvmulder @jpata
should this PR go to the backburner?
There was no feedback in this thread for almost a week now.

@ghost
Copy link

ghost commented Jan 28, 2016

@slava77

@jpata mentioned today (offline) that he should be able to solve the conflicts today

Conflicts:
	RecoBTag/Combined/python/combinedMVA_cff.py

removed couts
@cmsbuild
Copy link
Contributor

Pull request #13014 was updated. @cvuosalo, @monttj, @cmsbuild, @slava77, @vadler, @davidlange6 can you please check and sign again.

@jpata
Copy link
Contributor Author

jpata commented Jan 29, 2016

rebased to latest IB, implemented @ferencek-s comments

@jpata
Copy link
Contributor Author

jpata commented Jan 29, 2016

scram b runtests passed for me. could you please comment?

@slava77
Copy link
Contributor

slava77 commented Jan 29, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

+1

For #13014 478e98f

Adding configuration files for negative and positive cMVAv2 b taggers.There should be no change in monitored quantities.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_0_X_2016-01-28-2300 show no significant differences, as expected.

@jpata
Copy link
Contributor Author

jpata commented Feb 1, 2016

any updates needed on my side?

davidlange6 added a commit that referenced this pull request Feb 1, 2016
…016-01-19-1100

cMVAv2 negative/positive configuration files
@davidlange6 davidlange6 merged commit 555de60 into cms-sw:CMSSW_8_0_X Feb 1, 2016
cmsbuild added a commit that referenced this pull request Feb 1, 2016
@jpata jpata deleted the cmvav2_negpos_from-CMSSW_8_0_X_2016-01-19-1100 branch July 31, 2016 10:38
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

6 participants