EGM-certified electron IDs for 2016 data: cut-based and MVA #16979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…few new cut classes
…lass files (including old ones) to make things work better for FWLite.
…was preventing the release from building in 81X+
|
A new Pull Request was created by @ikrav for CMSSW_9_0_X. It involves the following packages: RecoEgamma/EgammaTools @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
cvuosalo
reviewed
Dec 12, 2016
| from PhysicsTools.SelectorUtils.centralIDRegistry import central_id_registry | ||
|
|
||
| # Common functions and classes for ID definition are imported here: | ||
| from RecoEgamma.ElectronIdentification.Identification.cutBasedElectronID_tools import * |
There was a problem hiding this comment.
Still one import * in the new code -- please fix like you did the other one. Thanks.
|
On 12/12/16 12:33 PM, Carl Vuosalo wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In
RecoEgamma/ElectronIdentification/python/Identification/cutBasedElectronHLTPreselecition_Summer16_V1_cff.py
<#16979 (review)>:
> @@ -0,0 +1,99 @@
+from PhysicsTools.SelectorUtils.centralIDRegistry import central_id_registry
+
+# Common functions and classes for ID definition are imported here:
+from RecoEgamma.ElectronIdentification.Identification.cutBasedElectronID_tools import *
Still one |import *| in the new code -- please fix like you did the
other one. Thanks.
This import *
cleanup is needed primarily for imports of files which define actual
modules.
It is not needed and is rather tedious to maintain for includes of
functions tools and ingredient PSets.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16979 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbv6aDVibBdP3Q09j6wci4osQxX0sks5rHa-3gaJpZM4LK6e2>.
|
|
Well, anyways I fixed it already, the list of explicit imports this time is
not too long. The change is committed.
… On Dec 12, 2016, at 3:17 PM, Slava Krutelyov ***@***.***> wrote:
On 12/12/16 12:33 PM, Carl Vuosalo wrote:
> ***@***.**** commented on this pull request.
>
> ------------------------------------------------------------------------
>
> In
> RecoEgamma/ElectronIdentification/python/Identification/cutBasedElectronHLTPreselecition_Summer16_V1_cff.py
> <#16979 (review)>:
>
>> @@ -0,0 +1,99 @@
> +from PhysicsTools.SelectorUtils.centralIDRegistry import central_id_registry
> +
> +# Common functions and classes for ID definition are imported here:
> +from RecoEgamma.ElectronIdentification.Identification.cutBasedElectronID_tools import *
>
> Still one |import *| in the new code -- please fix like you did the
> other one. Thanks.
This import *
cleanup is needed primarily for imports of files which define actual
modules.
It is not needed and is rather tedious to maintain for includes of
functions tools and ingredient PSets.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#16979 (review)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AEdcbv6aDVibBdP3Q09j6wci4osQxX0sks5rHa-3gaJpZM4LK6e2>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
The error below is fixed, the change is committed. The cause was my typo when
changing very many variable names to comply CMS coding rules in the previous commit.
… On Dec 14, 2016, at 11:59 AM, cmsbuild ***@***.***> wrote:
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_0_X_2016-12-14-1100/src/RecoEgamma/ElectronIdentification/src/ElectronMVAEstimatorRun2Spring16HZZ.cc
:54:28: error: field 'conversionsLabelMiniAOD_' is uninitialized when used here [-Werror,-Wuninitialized]
conversionsLabelMiniAOD_(conversionsLabelMiniAOD_) {
|
cvuosalo
reviewed
Dec 15, 2016
| } | ||
|
|
||
| private: | ||
| const double _normalizedGsfChi2CutValueEB,_normalizedGsfChi2CutValueEE,_barrelCutOff; |
There was a problem hiding this comment.
Three more with prohibited leading underbars. Please fix them.
|
Pull request #16979 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
|
This is fixed and committed.
… On Dec 15, 2016, at 3:09 PM, Carl Vuosalo ***@***.***> wrote:
@cvuosalo commented on this pull request.
In RecoEgamma/ElectronIdentification/plugins/cuts/GsfEleNormalizedGsfChi2Cut.cc:
> + CutApplicatorBase(c),
+ _normalizedGsfChi2CutValueEB(c.getParameter<double>("normalizedGsfChi2CutValueEB")),
+ _normalizedGsfChi2CutValueEE(c.getParameter<double>("normalizedGsfChi2CutValueEE")),
+ _barrelCutOff(c.getParameter<double>("barrelCutOff")){
+ }
+
+ result_type operator()(const reco::GsfElectronPtr&) const override final;
+
+ double value(const reco::CandidatePtr& cand) const override final;
+
+ CandidateType candidateType() const override final {
+ return ELECTRON;
+ }
+
+private:
+ const double _normalizedGsfChi2CutValueEB,_normalizedGsfChi2CutValueEE,_barrelCutOff;
Three more with prohibited leading underbars. Please fix them.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
+1 For EGM, adding new certified electron IDs. The code changes are satisfactory, and Jenkins tests against baseline CMSSW_9_0_X_2016-12-15-2300 show no significant differences. An extended test for the previous version of this PR (#16839 (comment)), which differed only slightly from the current one, showed no problems, with no differences and only a very marginal increase in CPU time. |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar |
|
Does this means I may proceed with submitting pull requests to 80X and 81X
with identical content? - thanks
… On Dec 16, 2016, at 11:38 AM, cmsbuild ***@***.***> wrote:
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
this PR is not merged yet. |
|
Ok, I take it that I shouldn’t submit new PRs until this PR is merged.
(I sent the email because the previous message said it is fully signed off on).
Higgs people asked me not to do backport to 80X, so instead I will cherry-pick
the commits you asked me to introduce in this PR, and add them to my original
80X branch.
… On Dec 19, 2016, at 1:54 PM, Slava Krutelyov ***@***.***> wrote:
this PR is not merged yet.
You can make backports, but there is a small chance you'd need to edit here and in the backports later.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
+1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains EGM certified cut-based and MVA electron IDs tuned on 80X samples. They are intended to be used on 2016 data. The electron MVA contains the HZZ MVA used for ICHEP16 and the presently recommended general purpose MVA tuned for better performance given features of a more complete 2016 dataset.
This PR is a new version of the recent PR #16839 (about to be closed) that is rebased to CMSSW_9_0_X_2016-12-07-2300. It also contains a few minor changes addressing comments from reviewers.