-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Adding MVA ID to nanoAOD code #39355
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39355/32053
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39355/32058
|
A new Pull Request was created by @cramonal for master. It involves the following packages:
@cmsbuild, @swertz, @vlimant can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Thanks @cramonal , a couple of things:
|
Thanks for the prompt review @swertz.
I agree with this comment. Can you point me to the era modifier needed for processing miniv2? |
Before I can answer that, could you clarify whether that new mvaID will be supported also for Run 2 UL (which would clearly be good; but means it needs to be re-run on Run 2 miniV2 samples) or whether it is only made available for Run 3? |
We would like this to be supported for Run 2 UL besides Run 3. |
In that case you would need to insert the on-the-fly evaluation for these modifiers:
While by default, for anything more recent (>=123X) it would be taken from mini. |
@cramonal , any update on this? Is it clear to you how to proceed with the modifiers? |
Hi, kind ping again @cramonal , or perhaps @cms-sw/muon-pog-l2 - what is the plan for this? |
Hi, we are working on this. Will update in the following days |
a60ca3c
to
8cc1144
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This time the rebase worked @cramonal , but some small changes are still needed.
@@ -45,6 +71,10 @@ | |||
), | |||
) | |||
|
|||
(run2_miniAOD_80XLegacy | run2_nanoAOD_92X | run2_nanoAOD_94X2016 | run2_nanoAOD_94XMiniAODv1 | run2_nanoAOD_94XMiniAODv2\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other cases below should just be run2_nanoAOD_106Xv2 | run3_nanoAOD_122
mvaLowPtId = Var("passed('LowPtMvaLoose')+passed('LowPtMvaMedium')","uint8", doc="Low Pt Mva ID from miniAOD selector (1=LowPtMvaLoose, 2=LowPtMvaMedium)"), | ||
miniIsoId = Var("passed('MiniIsoLoose')+passed('MiniIsoMedium')+passed('MiniIsoTight')+passed('MiniIsoVeryTight')","uint8",doc="MiniIso ID from miniAOD selector (1=MiniIsoLoose, 2=MiniIsoMedium, 3=MiniIsoTight, 4=MiniIsoVeryTight)"), | ||
mvaIDMuon = Var("mvaIDValue()",float,doc="MVA-based ID score ",precision=6), | ||
mvaIDMuon_WP = Var("passed('MvaIDwpMedium')+passed('MvaIDwpTight')","uint8",doc="MVA-based ID selector WPs (1=MVAIDwpMedium,2=MVAIDwpTight)"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't we agree to remove the WP variable?
bdfca86
to
e86bd08
Compare
Dear @swertz, Sorry for the noise I was finalizing the changes. Everything should be in now. Please note that we want to keep the WP branch but we want this to be computed in nanoAOD for all the cases. Also I have renamed the branches in order to avoid having the expresion "Muon_*" twice in the name of the branch. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39355/33054
|
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-774eca/29036/summary.html Comparison SummarySummary:
NANO Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
Nano size comparison Summary:
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This PR propagates the new muon identification approach using a MVA introduced in this PR to the nanoAOD:
This MVA has been presented in several Muon POG meetings: https://indico.cern.ch/event/1034615/#4-update-on-mva-id
This is already available in miniAOD, but in order to simplify the usage of this ID at analysis level it would be of interest to have this also available in nanoAOD.
In the output there are 3 new branches:
Muon_mvaIDMuon -> contains the MVA score reading it from the miniAOD.
Muon_mvaIDMuon_WP -> contains the miniAOD selectors with the definition of the proposed Working Points.
Muon_mvaIDMuon_nano -> contains the MVA score, recalculating it when running the nanoAOD.
PR validation:
We check the output of the new branch and Working Points running runTheMatrix and everything looks fine.
We execute the basic tests suggested in the CMSSW PR instructions