Skip to content
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

Closed
wants to merge 7 commits into from

Conversation

Cvico
Copy link
Contributor

@Cvico Cvico commented Mar 5, 2024

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:

  1. The required era modifiers in Configuration/Eras/.
    • I've added two new era modifiers run3_muon_2022 and run3_muon_2023, which are called from the general Run3_Eras_cff.py.
    • I hope this is the optimal way to handle our use case.
  2. The call to the era modifier and the implementation of the new mvaTTH evaluation.
    • How the mva is evaluated changes a little bit with respect to the previous version: mainly the order in which the variables are parsed to TMVA and also we have added a new additional variable which is LepGood_pfRelIso03_all.
    • Which weights should be read in order to properly compute the mva score.

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.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44322/39349

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2024

A new Pull Request was created by @Cvico for master.

It involves the following packages:

  • Configuration/Eras (operations)
  • PhysicsTools/NanoAOD (xpog)

@davidlange6, @antoniovilela, @rappoccio, @vlimant, @fabiocos, @cmsbuild, @hqucms can you please review it and eventually sign? Thanks.
@makortel, @gpetruc, @AnnikaStein, @missirol, @fabiocos, @Martin-Grunewald this is something you requested to watch as well.
@antoniovilela, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@vlimant
Copy link
Contributor

vlimant commented Mar 6, 2024

I'd rather have the default be run3, and you revert to the old using ~run3

@Cvico
Copy link
Contributor Author

Cvico commented Mar 6, 2024

I'd rather have the default be run3, and you revert to the old using ~run3

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(
Copy link
Contributor

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

Copy link
Contributor Author

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.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44322/39359

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 6, 2024

Pull request #44322 was updated. @antoniovilela, @davidlange6, @vlimant, @fabiocos, @rappoccio, @cmsbuild, @hqucms can you please check and sign again.

@JanFSchulte
Copy link
Contributor

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.

@Cvico
Copy link
Contributor Author

Cvico commented Mar 6, 2024

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.

@Fedespring
Copy link
Contributor

Thanks @Cvico, this further change should happen basically now to get everything in place.

@Cvico
Copy link
Contributor Author

Cvico commented Mar 8, 2024

Hi, I've updated the name from mvaTTH to promptMVA as @JanFSchulte suggested, also in the electrons_cff.py config. I've checked it produces an output with two branches: Electron_promptMVA and Muon_promptMVA.

I've also included two changes: one in nano_eras_cff.py to include the run3_muon modifiers, and also I had to modify the muonMVALowPt to fetch the variables for the Run 2 training explicitly. The reason for this latter change is because it was previously working on TOP of the Run 2 mvaTTH configuration, but since we moved this to Run 3 by default, the mvaLowPT had to be explicitly modified with the Run 2 variables.

@cmsbuild
Copy link
Contributor

Pull request #44322 was updated. @jfernan2, @antoniovilela, @mandrenguyen, @vlimant, @rappoccio, @davidlange6, @hqucms, @cmsbuild, @fabiocos can you please check and sign again.

@jfernan2
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests RelVals RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cae026/38125/summary.html
COMMIT: bafee58
CMSSW: CMSSW_14_1_X_2024-03-13-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/44322/38125/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 2 errors in the following unit tests:

---> test TestConfigDP_7 had ERRORS
---> test TestConfigDP_8 had ERRORS

RelVals

  • 136.7611136.7611_RunJetHT2016EreMINIAOD/step2_RunJetHT2016EreMINIAOD.log
  • 136.8311136.8311_RunJetHT2017FreMINIAOD/step2_RunJetHT2017FreMINIAOD.log
  • 136.88811136.88811_RunJetHT2018DreMINIAODUL/step2_RunJetHT2018DreMINIAODUL.log
Expand to see more relval errors ...

RelVals-INPUT

  • 4.64.6_MinimumBias2010A/step2_MinimumBias2010A.log
  • 136.72411136.72411_RunJetHT2016B_reminiaodUL/step2_RunJetHT2016B_reminiaodUL.log
  • 136.72412136.72412_RunJetHT2016B_reminiaodUL/step2_RunJetHT2016B_reminiaodUL.log
Expand to see more relval errors ...

@Cvico
Copy link
Contributor Author

Cvico commented Mar 14, 2024

Hi, I fixed the import that failed. Apologies for that rookie mistake.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44322/39483

@cmsbuild
Copy link
Contributor

Pull request #44322 was updated. @mandrenguyen, @fabiocos, @antoniovilela, @cmsbuild, @rappoccio, @hqucms, @jfernan2, @davidlange6, @vlimant can you please check and sign again.

@Cvico
Copy link
Contributor Author

Cvico commented Mar 18, 2024

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?

@vlimant
Copy link
Contributor

vlimant commented Mar 18, 2024

hello, the only conflict is in muon_cff, and the change to BaseMVAValueMapProducer might actually help with this PR.
the "fun" will begin for the backport, if backport at all, since it is going to be different. I don't think I want just yet to backport #44392

@JanFSchulte
Copy link
Contributor

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.

@JanFSchulte
Copy link
Contributor

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

@Cvico
Copy link
Contributor Author

Cvico commented Mar 19, 2024

Apologies for the little experience on rebasing:
What should be the new base? I'm doing git cms-rebase-topic cvico:mvatth_run3_14_1_0_pre0 from a fresh 14_1_0_pre0 but it does not find any conflict...

@JanFSchulte
Copy link
Contributor

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.

@vlimant
Copy link
Contributor

vlimant commented Mar 20, 2024

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.
In the meatime, I'll backport #44392 too, to make it easier to get your feature in 14.0.

@vlimant
Copy link
Contributor

vlimant commented Mar 21, 2024

I made #44503 to simplify this.

@Cvico
Copy link
Contributor Author

Cvico commented Mar 21, 2024

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. In the meatime, I'll backport #44392 too, to make it easier to get your feature in 14.0.

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.

@vlimant
Copy link
Contributor

vlimant commented Mar 21, 2024

please close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants