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

New mva taggers backport from 71X to 53X #3795

Conversation

pvmulder
Copy link
Contributor

@pvmulder pvmulder commented May 9, 2014

Backport of the merged 71X pull requests:
#2622
#3384
#3664
#3733
but with changes in configuration files and CombinedSVComputer.cc to keep the standard CSV without a change

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2014

A new Pull Request was created by @pvmulder (Petra Van Mulders) for CMSSW_5_3_X.

New mva taggers backport from 71X to 53X

It involves the following packages:

DataFormats/BTauReco
PhysicsTools/MVAComputer
RecoBTag/SecondaryVertex
RecoVertex/AdaptiveVertexFinder

@nclopezo, @monttj, @cmsbuild, @anton-a, @thspeer, @slava77, @vadler, @Degano can you please review it and eventually sign? Thanks.
@ferencek, @cerati, @GiacomoSguazzoni, @rovere 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.
@smuzaffar you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@@ -21,3 +21,21 @@
extSVDeltaRToJet = cms.double(0.3)

)

secondaryVertexTagInfosV2 = cms.EDProducer("SecondaryVertexProducer",
Copy link
Contributor

Choose a reason for hiding this comment

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

@pvmulder To make things less error-prone, why not define this as

secondaryVertexTagInfosV2 = secondaryVertexTagInfos.clone()
secondaryVertexTagInfosV2.trackSelection.qualityClass = cms.string('any')

Just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ferencek fixed

@ferencek
Copy link
Contributor

ferencek commented May 9, 2014

@pvmulder OK, looks good. The old CVS does not change its 53X behavior. The same can now be applied to 6_2_X_SLHC. Thanks

trackP0Par, // track momentum along the jet axis, in the jet rest frame
trackP0ParRatio, // track momentum along the jet axis, in the jet rest frame, normalized to its energy"
trackChi2, // track fit chi2
trackNTotalHits, // number of valid total hits
trackNPixelHits, // number of valid pixel hits

chargedHadronEnergyFraction, // fraction of the jet energy coming from charged hadrons
Copy link
Contributor

Choose a reason for hiding this comment

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

@pvmulder This is a cosmetic detail and I did notice it before but why are all the descriptions no longer aligned. With GitHub's very strange choice of the visible line length, this issue becomes apparent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ferencek This is a problem in my editor that is not well-configured. In my editor things seems perfectly aligned, but not in other editors.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pvmulder Maybe try with some other editor just for this file and TaggingVariable.cc. Otherwise, I can also easily fix it.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2014

Pull request #3795 was updated. @nclopezo, @monttj, @cmsbuild, @anton-a, @thspeer, @slava77, @vadler, @Degano can you please check and sign again.

2 similar comments
@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2014

Pull request #3795 was updated. @nclopezo, @monttj, @cmsbuild, @anton-a, @thspeer, @slava77, @vadler, @Degano can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2014

Pull request #3795 was updated. @nclopezo, @monttj, @cmsbuild, @anton-a, @thspeer, @slava77, @vadler, @Degano can you please check and sign again.

@monttj
Copy link
Contributor

monttj commented May 10, 2014

+1

@cmsbuild
Copy link
Contributor

Pull request #3795 was updated. @nclopezo, @monttj, @cmsbuild, @thspeer, @StoyanStoynev, @slava77, @vadler, @Degano can you please check and sign again.

@vadler
Copy link

vadler commented May 22, 2014

+1
I don't see a change related to the update message #3795 (comment)

@StoyanStoynev
Copy link
Contributor

@pvmulder Could you clarify about configuration files - I see no changes introduced in RecoBTag/SecondaryVertex/python/combinedSecondaryVertexCommon_cfi.py , RecoBTag/SecondaryVertex/python/trackPseudoSelection_cfi.py , RecoBTag/SecondaryVertex/python/trackSelection_cfi.py , RecoBTag/SecondaryVertex/python/vertexTrackSelection_cfi.py and this deviates from 71X PRs. Is it part of preserving "the standard CSV" behavior?

@pvmulder
Copy link
Contributor Author

@StoyanStoynev
That is correct. There are no changes in the configuration files to preserve the standard CSV behavior.

@StoyanStoynev
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_5_3_X IBs unless changes or unless it breaks tests. @smuzaffar can you please take care of it?

@smuzaffar
Copy link
Contributor

+tested

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_5_3_X IBs unless changes (tests are also fine). @smuzaffar can you please take care of it?

davidlange6 added a commit that referenced this pull request Jun 3, 2014
…_71X_to_53X

New mva taggers backport from 71X to 53X
@davidlange6 davidlange6 merged commit cc0127b into cms-sw:CMSSW_5_3_X Jun 3, 2014
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

9 participants