-
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
Implemented new modifiers for mvaTTH Run 3 #44322
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44322/39349
|
A new Pull Request was created by @Cvico for master. It involves the following packages:
@davidlange6, @antoniovilela, @rappoccio, @vlimant, @fabiocos, @cmsbuild, @hqucms can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
I'd rather have the default be run3, and you revert to the old using |
Thanks @vlimant for the suggestion. I don't have a feeling on what's the best option here, but could you point me on how to properly do this? Do I have to change these lines, and call the run2 modifier instead of the run3 modifier? |
@@ -119,6 +119,30 @@ | |||
weightFile = "PhysicsTools/NanoAOD/data/mu_BDTG_2016.weights.xml", | |||
) | |||
|
|||
run3_muon.toModify( |
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.
in line 89, set muonMVATTH
with the run3 parameters, and there do run3_common).toModify(
with old paremters. which is just changing the fileinpath, the variable order and remove variables
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.
I see, I've updated the config to do this. I was not aware this ~run3
could be done. Let me know if it is what you expected.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44322/39359
|
Pull request #44322 was updated. @antoniovilela, @davidlange6, @vlimant, @fabiocos, @rappoccio, @cmsbuild, @hqucms can you please check and sign again. |
Hi @Cvico From the POG side we would like to take the opportunity to rename the variable from mvaTTH to mvaPrompt to unify the naming with what is used in the MUO-22-001 paper and to not have it so closely associated with just one analysis since we want this to be an official POG product going forward. Could you take care of that? It should probably be mentioned in the doc string what the previous name was. |
Hi Jan. From my side I personally like the idea of changing the name to mvaPrompt (more precisely, I don't have strong opinion against it). But since there's a mvaTTH for electrons, I think EGM conveners should be aware of the change as well. I can quickly contact them with the proposal. |
Thanks @Cvico, this further change should happen basically now to get everything in place. |
Hi, I've updated the name from I've also included two changes: one in |
Pull request #44322 was updated. @jfernan2, @antoniovilela, @mandrenguyen, @vlimant, @rappoccio, @davidlange6, @hqucms, @cmsbuild, @fabiocos can you please check and sign again. |
please test |
-1 Failed Tests: UnitTests RelVals RelVals-INPUT Unit TestsI found 2 errors in the following unit tests: ---> test TestConfigDP_7 had ERRORS ---> test TestConfigDP_8 had ERRORS RelVals
Expand to see more relval errors ...RelVals-INPUT
Expand to see more relval errors ...
|
Hi, I fixed the import that failed. Apologies for that rookie mistake. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44322/39483
|
Pull request #44322 was updated. @mandrenguyen, @fabiocos, @antoniovilela, @cmsbuild, @rappoccio, @hqucms, @jfernan2, @davidlange6, @vlimant can you please check and sign again. |
Hi, I see #44392 has been merged. How do I properly resolve the conflicts? Do I pull the commit and then redo my changes from there? |
hello, the only conflict is in muon_cff, and the change to BaseMVAValueMapProducer might actually help with this PR. |
If at all possible, we would really like to have this back-ported to 14_0_X so we can have the new training in the Nano for 2024 data and the 2022/2023 ReReco. |
Ah, and @Cvico, the cleanest solution would be to rebase your branch to the latest IB, following https://cms-sw.github.io/tutorial-resolve-conflicts.html |
Apologies for the little experience on rebasing: |
You need to rebase with a CMSSW build that has the PR that created the conflict already compiled in. There are twice-daily integration builds that reflect the current state of the repository. In this case, you need to start from a fresh checkout of CMSSW_14_1_X_2024-03-19-1100 and then rebase your branch on that. |
Hi @Cvico , if you have negative experience with rebasing, as this can get complicated (but a good experience for sft dev), I can take care of cherrypicking your changes and do the necessary in muons_cff in a new PR ; let me know how it goes on your end. |
I made #44503 to simplify this. |
Hi @vlimant, sorry for the late reply, yesterday I was busy with teaching. I'm not sure what's the best option here, I have no experience with it, I'm not sure if (for time sake) it would be better to fix in another PR. |
please close |
PR description:
Brief description
Dear reviewers,
This PR is to introduce the newest training of the Muon mvaTTH in CMSSW. The training has been performed following what was done already done for Run 2, but using Run 3 MC samples for training and validation.
Context of the PR
The goal for this new training is to be used as a substitute of the current mvaTTH only in Run 3 MC production. That is: for Run 2 we still need to keep the current mvaTTH implementation (can be found in these lines), and for Run 3 we want to make use of an era modifier to fetch the proper --new -- weights that have been computed.
What this PR includes
This change should only affect Run 3 production. For that, this PR includes:
Configuration/Eras/
.run3_muon_2022
andrun3_muon_2023
, which are called from the generalRun3_Eras_cff.py
.LepGood_pfRelIso03_all
.Validation
I've performed a local test running a simple
cmsRun
script to see that the mva is loaded and nanoAOD is in fact produced.Other comments
Please let me know if the proposal is reasonable, and if there's anything else I can do for testing. I'm adding the L2 muon convener @JanFSchulte so he can follow up the PR as well. Feel free to add any other muon convener (I don't have their github user names unfortunately).
Linked PRs
This is the link for the PR that includes the weights in CMS-data: pull16.