-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add HZZ electron ID for Run3 #43430
Add HZZ electron ID for Run3 #43430
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43430/37964
|
A new Pull Request was created by @apetkovi1 (Andro Petkovic) for master. It involves the following packages:
@mandrenguyen, @vlimant, @cmsbuild, @simonepigazzini, @jfernan2 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@apetkovi1 I just closed #43369 |
enable nano |
please test with cms-data/RecoEgamma-ElectronIdentification#28 |
-1 Failed Tests: RelVals RelVals-INPUT RelVals-NANO 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
RelVals-NANO |
Hi @a-kapoor, @Prasant1993 I am not sure why this fails - locally I was able to run runTheMatrix. Locally I do not have these logs (the ones where we can see error here). |
Hi @apetkovi1 Can you check if you are able to run https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f7b9c7/36162/runTheMatrix-results/25202.0_TTbar_13/cmdLog |
Hi @a-kapoor I have updated runTheMatrix output dir with outpouts from these new tests here. When using your commands I recreate some of these errors (check step1,2,3,4,5_TTbar_13.log). Any idea what could cause them ? EDIT: I resolved error in step5_TTbar_13.log by removing this line https://github.com/apetkovi1/cmssw/blob/a75a3d6f1a4b8be6a2945792e9e688297ea304f3/PhysicsTools/NanoAOD/python/electrons_cff.py#L188. Also, one thing that bugs me is that before I added this line there was not a single line of code in electrons_cff.py that corresponds to HZZ working point. However, I remember that in previous nanoAOD, HZZ working point was stored. EDIT: However, now i ran runTheMatrix while removing that line from electrons_cff.py. It seems that it is necessarry, since now wpHZZ has not been added to nano. In conclusion, this line is causing an issue https://github.com/apetkovi1/cmssw/blob/a75a3d6f1a4b8be6a2945792e9e688297ea304f3/PhysicsTools/NanoAOD/python/electrons_cff.py#L188, but if I remove it, wpHZZ is not included in nano after I do runTheMatrix. Still unclear for me how to resolve this. |
Hi @apetkovi1, I am just trying to understand here a bit more. About this HZZ ID in the past in Run2 UL16, Ul17 and UL18, I see you have only one variable (float) defined in the NanoAOD here : (run2_egamma_2016).toModify( But for Run3 case, you have 2 variables here. One is float and the other is int.
Do you really need these two in Run3 ? The above workflow is a Run2 workflow. So it doesn't find the second int variable for Run2. Try to test again. |
Hi @Prasant1993 @a-kapoor, I added what you said but the message stays the same (not sure what could cause this since wpHZZ is sucesfully added to nano when running runTheMatrix, proably some conflict with Run2 since wpHZZ was not added to nano back then). Now, if I would remove WPHZZ part from electrons_cff, working point would be added to miniAOD but not to nanoAOD ? (I still have HZZ working point defined in RecoEgamma/ElectronIdentification/python/FWLite.py) |
@apetkovi1, can you paste the error that you have got after did the necessary change for Run2 : "mvaIso_WPHZZ = None" ? |
This is my command:
Click to read the logNANO,ENDJOB %MSG %MSG %MSG FastJet release 3.4.1M. Cacciari, G.P. Salam and G. SoyezA software package for jet finding and analysis at collidershttp://fastjet.frPlease cite EPJC72(2012)1896 [arXiv:1111.6097] if you use this packagefor scientific work and optionally PLB641(2006)57 [hep-ph/0512210].FastJet is provided without warranty under the GNU GPL v2 or higher.It uses T. Chan's closest pair algorithm, S. Fortune's Voronoi codeand 3rd party plugin jet algorithms. See COPYING file for details.#-------------------------------------------------------------------------- %MSG
|
-1 Failed Tests: UnitTests RelVals RelVals-INPUT RelVals-NANO Unit TestsI found 1 errors in the following unit tests: ---> test runtestRecoEgammaElectronIdentification had ERRORS RelVals
Expand to see more relval errors ...RelVals-INPUT
Expand to see more relval errors ...
RelVals-NANO |
I see now in error log that test fails to find MVA weights, is the PR tested together with this one ? |
Hi @vlimant Please issue the test command with the PR cms-data/RecoEgamma-ElectronIdentification#28 The weight files PR is not merged yet. |
please test with cms-data/RecoEgamma-ElectronIdentification#28 |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f7b9c7/36801/summary.html 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: Comparison SummarySummary:
NANO Comparison SummarySummary:
Nano size comparison Summary:
|
Hi @vlimant
Which has now changed to
This is now the default. The differences in this https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisonsNANO/CMSSW_14_0_X_2024-01-10-2300+f7b9c7/60552/validateJR.html make sense since the new ID are not extremely different, but just a "Run 3" fine-tuned version. The "Run 2" version was used for Run 3 as well till Nanov13 since the new trained ID was not available at that point. It makes sense here that the new ID for ttbar tells us there is more background: Even for Mini, It makes sense that the size of userfloat and userints change by "1": Overall, the changes are expected. Tests are all passed, and it would be awesome if this could go ahead. Let me know. Tagging @RSalvatico @apetkovi1 @Prasant1993 |
got it |
+1 |
+1 |
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. @sextonkennedy, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
This PR is to add HZZ electron ID for Run 3. It has been tested with runTheMatrix (logs are in (1,2,3,4,5)). Also it has been tested with 2016 workflow since previously Run2 tests caused failure (log is in (6)) Additionally it has been tested privately to check if the sig. and bkg. efficiency correspond to the ones I got when testing during the training phase. Comparison is in (7). Essentially I ran testElectronMVARun3 and calculated sig and bkg efficiency in every region. Now, as you can see, testing with testElectronMVARun3 gave slightly better performance (higher sig and lower bkg eff) then test during the training phase. This is because I ran testElectronMVARun3 on a dataset which also partially includes the one which I used for training. This is because during the training phase I made random test/train splitting and did not save the test dataset. This is why I do not have completely unbiased testing sample when I use testElectronMVARun3. However, when we take into account the fact that part of this dataset was used for training, the results are expected. Please test this PR together with this one. ID presentation is in (8).
(1) step1_TTbar_14TeV+2023.log
(2) step2_TTbar_14TeV+2023.log
(3) step3_TTbar_14TeV+2023.log
(4) step4_TTbar_14TeV+2023.log
(5) step5_TTbar_14TeV+2023.log
(6) step5_TTbar_13.log
(7) MVAcomparison.pdf
(8) ElectronID_EGamma_v2.pdf