-
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 HZZ electron MVA ID #37429
Adding HZZ electron MVA ID #37429
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37429/29122
|
A new Pull Request was created by @asculac (Ana Sculac) for master. It involves the following packages:
@cmsbuild, @mariadalfonso, @gouskos, @fgolf can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
-1 Failed Tests: RelVals RelVals-INPUT The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: RelVals
RelVals-INPUT
|
The failure in on Run3 2021 scenarios 11634.914, 11634.0, 11634.7, 11634.911, 12434.0, 11834.0 |
run2_egamma_2016.toModify(slimmedElectronsWithUserData.userFloats, | ||
mvaHZZIdIso = cms.InputTag("electronMVAValueMapProducer:ElectronMVAEstimatorRun2Summer16ULIdIsoValues"), | ||
) | ||
run2_egamma_2017.toModify(slimmedElectronsWithUserData.userFloats, | ||
mvaHZZIdIso = cms.InputTag("electronMVAValueMapProducer:ElectronMVAEstimatorRun2Summer17ULIdIsoValues") , | ||
) | ||
run2_egamma_2018.toModify(slimmedElectronsWithUserData.userFloats, | ||
mvaHZZIdIso = cms.InputTag("electronMVAValueMapProducer:ElectronMVAEstimatorRun2Summer18ULIdIsoValues"), | ||
) |
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.
These modifiers act also on the pre-UL.
mvaFall17V2Iso = cms.InputTag("electronMVAValueMapProducer:ElectronMVAEstimatorRun2Fall17IsoV2Values"), | ||
mvaFall17V2noIso = cms.InputTag("electronMVAValueMapProducer:ElectronMVAEstimatorRun2Fall17NoIsoV2Values"), |
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.
We should rename these ID to be generic.
In Run3 we will soon move out of the "Fall17V2" training .
So in the nano we should have one variable generica name and will be dinamically filled with the right ID for the Run2 ("Fall17V2" training) and Run3 (the one to come)
mvaFall17V2Iso --> mvaIso
mvaFall17V2noIso --> mvanoIso
The docs will point to the right one.
At the analysis level this will help the combined analysis of the Run2 and Run3 dataset, that is our main goal for next Run2 nano production.
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.
Hi @mariadalfonso ,
yes I agree.
@asculac you can implement it in this PR. Or if you feel this is out of scope of this PR, then one of us can do it in a follow-up PR. But indeed, the naming of mva egamma ID branches could be improved. (The cutbased ID branch names already looks good.)
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.
It looks like a very good idea; I would then suggest to agree on consistent naming;
mvaIso
mvanoIso (or better mvaNoIso)?
mvaHZZIso (instead of mvaHZZIdIsoValue)
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.
@swagata87 sure, I will add it to this PR it is not a problem
@mariadalfonso thank you for pointing everything out, I will check it and fix it
@asculac |
Hi @mariadalfonso , I apologise for the delay, unfortunately I didn't have time to look into details why our modules are not run when calling the VID producer. I will try to solve the problem in next week to avoid further delay with this PR |
@asculac any update ? |
Yes, our ID is not embedded in PAT object and this is causing the error. I am working currently on implementation. |
@asculac cmssw/PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py Lines 293 to 304 in 0cd6c4f
i.e, add the following:
After that we can test your PR again to check if the issue is solved |
Run3-specific failures ring a bell.. here: cmssw/PhysicsTools/NanoAOD/python/electrons_cff.py Lines 536 to 540 in 0cd6c4f
and just in the next block, i.e. here: cmssw/PhysicsTools/NanoAOD/python/electrons_cff.py Lines 542 to 555 in 0cd6c4f
does this make sense? |
type egamma |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37429/30437
|
it looks like the rebase was not correct - a lot of unrelated commits were included. |
Pull request #37429 was updated. @SiewYan, @gouskos, @bbilin, @pmandrik, @ianna, @Saptaparna, @Martin-Grunewald, @ggovi, @alberto-sanchez, @cecilecaillol, @perrotta, @civanch, @yuanchao, @makortel, @ahmad3213, @cmsbuild, @missirol, @GurpreetSinghChahal, @davidlange6, @smuzaffar, @Dr15Jones, @epalencia, @cvuosalo, @emanueleusai, @mdhildreth, @AdrianoDee, @jfernan2, @kskovpen, @slava77, @jpata, @qliphy, @micsucmed, @fabiocos, @francescobrivio, @malbouis, @mkirsano, @jordan-martins, @rekovic, @clacaputo, @alja, @srimanob, @fgolf, @mariadalfonso, @tvami, @rvenditti can you please check and sign again. |
-1
|
@mariadalfonso Is it okay if I close this PR and open another clean one with the same implementation? To avoid messing up with rebase again from my side |
ok, fine with me. |
PR description:
PR validation:
@namapane pls follow