cmssw icon indicating copy to clipboard operation
cmssw copied to clipboard

Implemented new modifiers for mvaTTH Run 3

Open Cvico opened this issue 11 months ago • 33 comments

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.

Cvico avatar Mar 05 '24 22:03 Cvico

cms-bot internal usage

cmsbuild avatar Mar 05 '24 22:03 cmsbuild

+code-checks

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

  • This PR adds an extra 28KB to repository

cmsbuild avatar Mar 05 '24 22:03 cmsbuild

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

cmsbuild avatar Mar 05 '24 22:03 cmsbuild

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

vlimant avatar Mar 06 '24 07:03 vlimant

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?

Cvico avatar Mar 06 '24 08:03 Cvico

+code-checks

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

  • This PR adds an extra 28KB to repository

cmsbuild avatar Mar 06 '24 11:03 cmsbuild

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

cmsbuild avatar Mar 06 '24 11:03 cmsbuild

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.

JanFSchulte avatar Mar 06 '24 14:03 JanFSchulte

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.

Cvico avatar Mar 06 '24 15:03 Cvico

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

Fedespring avatar Mar 06 '24 15:03 Fedespring

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.

Cvico avatar Mar 08 '24 09:03 Cvico

+code-checks

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

  • This PR adds an extra 36KB to repository

cmsbuild avatar Mar 08 '24 09:03 cmsbuild

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

cmsbuild avatar Mar 08 '24 09:03 cmsbuild

type egamma

RSalvatico avatar Mar 08 '24 11:03 RSalvatico

Hi, comments addressed. I also spotted an issue in the Era Run3 modifier: since run2_muon_2018 was not being explicitly removed there, it was loading the Run 2 weights anyway... Should I also put run2_muon_2017 in that exclusion file? I think now it should be only reading 2022 when the Era config is not run2_muon_2016, 2017 or 2018... But following all this modifiers is a little bit complicated.

Cvico avatar Mar 09 '24 17:03 Cvico

+code-checks

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

  • This PR adds an extra 36KB to repository

  • Found files with invalid states:

    • Configuration/Eras/python/Modifier_run3_muon_2023_cff.py:
      • Added: ba550c090f5a34bf49ecfda0430eb485aaa8d885
      • Deleted: f8ae9868255f06bf4f627677fe17b1950699d079
    • Configuration/Eras/python/Modifier_run3_muon_2022_cff.py:
      • Added: ba550c090f5a34bf49ecfda0430eb485aaa8d885
      • Deleted: f8ae9868255f06bf4f627677fe17b1950699d079

cmsbuild avatar Mar 09 '24 17:03 cmsbuild

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

cmsbuild avatar Mar 09 '24 17:03 cmsbuild

I also spotted an issue in the Era Run3 modifier: since run2_muon_2018 was not being explicitly removed there, it was loading the Run 2 weights anyway...

By quick look excluding run2_muon_2018 modifier from Run3 era would be consistent with how run2_muon201[67] modifiers are handled in Run2_201[78] eras. That change would seem to impact only two places.


First case is https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/NanoAOD/python/triggerObjects_cff.py#L390-L395

AFAICT the prefiringweight module is enabled only with https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/NanoAOD/python/triggerObjects_cff.py#L422-L424 and _run2_HLTconditions is specific to the eras Run2_201[678], so the change would be safe.


In the second case https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py#L42 the question is whether these values should apply to Run3 as well.


(on a side note, completely irrelevant to this PR, this customization https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/NanoAOD/python/triggerObjects_cff.py#L372-L375 violates 6.16 in https://cms-sw.github.io/cms_coding_rules.html#6--packaging-rules-1)

makortel avatar Mar 11 '24 17:03 makortel

Should I also put run2_muon_2017 in that exclusion file?

In what exclusion file?

makortel avatar Mar 11 '24 17:03 makortel

Should I also put run2_muon_2017 in that exclusion file?

In what exclusion file?

I was refering to the Eras_Run3_cff.py file where I explicitly excluded the 2018_muon config. But I think inside the 2018_muon config one removes the 2017_muon config, so I don't see need for removing it again

Cvico avatar Mar 11 '24 19:03 Cvico

I also spotted an issue in the Era Run3 modifier: since run2_muon_2018 was not being explicitly removed there, it was loading the Run 2 weights anyway...

By quick look excluding run2_muon_2018 modifier from Run3 era would be consistent with how run2_muon201[67] modifiers are handled in Run2_201[78] eras. That change would seem to impact only two places.

First case is

https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/NanoAOD/python/triggerObjects_cff.py#L390-L395

AFAICT the prefiringweight module is enabled only with

https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/NanoAOD/python/triggerObjects_cff.py#L422-L424

and _run2_HLTconditions is specific to the eras Run2_201[678], so the change would be safe.

In the second case

https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py#L42

the question is whether these values should apply to Run3 as well.

(on a side note, completely irrelevant to this PR, this customization

https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/NanoAOD/python/triggerObjects_cff.py#L372-L375

violates 6.16 in https://cms-sw.github.io/cms_coding_rules.html#6--packaging-rules-1)

I think @JanFSchulte @Fedespring might comment better on whether these things are needed for Run 3

Cvico avatar Mar 11 '24 19:03 Cvico

Should I also put run2_muon_2017 in that exclusion file?

In what exclusion file?

I was refering to the Eras_Run3_cff.py file where I explicitly excluded the 2018_muon config. But I think inside the 2018_muon config one removes the 2017_muon config, so I don't see need for removing it again

Right. The run2_muon_2017 is already excluded in https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/Configuration/Eras/python/Era_Run2_2018_cff.py#L24 so excluding it again in something deriving from Run2_2018 would be confusing.

makortel avatar Mar 11 '24 19:03 makortel

I also spotted an issue in the Era Run3 modifier: since run2_muon_2018 was not being explicitly removed there, it was loading the Run 2 weights anyway...

By quick look excluding run2_muon_2018 modifier from Run3 era would be consistent with how run2_muon201[67] modifiers are handled in Run2_201[78] eras. That change would seem to impact only two places. First case is https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/NanoAOD/python/triggerObjects_cff.py#L390-L395

AFAICT the prefiringweight module is enabled only with https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/NanoAOD/python/triggerObjects_cff.py#L422-L424

and _run2_HLTconditions is specific to the eras Run2_201[678], so the change would be safe. In the second case https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py#L42

the question is whether these values should apply to Run3 as well. (on a side note, completely irrelevant to this PR, this customization https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/NanoAOD/python/triggerObjects_cff.py#L372-L375

violates 6.16 in https://cms-sw.github.io/cms_coding_rules.html#6--packaging-rules-1)

I think @JanFSchulte @Fedespring might comment better on whether these things are needed for Run 3

For the L1 prefiring corrections, we do not have any new weights for Run 3 (yet), those would be in the work in the L1 group. So excluding the relevant modifications from Run 3 would make sense, since the values are meaningless for Run 3 anyway. I'm not sure what the dummy values filled in that case are, though.

As for the effective areas for MiniISO, there is no retuning for Run 3, so we would need them to stay as they are for 2018 in all Run 3 Mini/Nano versions.

JanFSchulte avatar Mar 13 '24 12:03 JanFSchulte

NB #44392

vlimant avatar Mar 13 '24 13:03 vlimant

As for the effective areas for MiniISO, there is no retuning for Run 3, so we would need them to stay as they are for 2018 in all Run 3 Mini/Nano versions.

Then, given that the present state of this PR excludes the run2_muon_2018 modifier from the Run3 era, the https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py#L40-L42 needs a new line

run3_muon.toModify( process.patMuons, effectiveAreaVec = [0.0566, 0.0562, 0.0363, 0.0119, 0.0064])

or you could consolidate

(run2_muon_2017 | run2_muon_2018 | run3_muon).toModify( process.patMuons, effectiveAreaVec = [0.0566, 0.0562, 0.0363, 0.0119, 0.0064])

makortel avatar Mar 13 '24 14:03 makortel

As for the effective areas for MiniISO, there is no retuning for Run 3, so we would need them to stay as they are for 2018 in all Run 3 Mini/Nano versions.

Then, given that the present state of this PR excludes the run2_muon_2018 modifier from the Run3 era, the

https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py#L40-L42

needs a new line

run3_muon.toModify( process.patMuons, effectiveAreaVec = [0.0566, 0.0562, 0.0363, 0.0119, 0.0064])

or you could consolidate

(run2_muon_2017 | run2_muon_2018 | run3_muon).toModify( process.patMuons, effectiveAreaVec = [0.0566, 0.0562, 0.0363, 0.0119, 0.0064])

Fixed. I opted for the first option.

Cvico avatar Mar 13 '24 23:03 Cvico

+code-checks

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

  • This PR adds an extra 28KB to repository

  • Found files with invalid states:

    • Configuration/Eras/python/Modifier_run3_muon_2023_cff.py:
      • Added: ba550c090f5a34bf49ecfda0430eb485aaa8d885
      • Deleted: f8ae9868255f06bf4f627677fe17b1950699d079
    • Configuration/Eras/python/Modifier_run3_muon_2022_cff.py:
      • Added: ba550c090f5a34bf49ecfda0430eb485aaa8d885
      • Deleted: f8ae9868255f06bf4f627677fe17b1950699d079
  • There are other open Pull requests which might conflict with changes you have proposed:

    • File Configuration/Eras/python/Era_Run3_cff.py modified in PR(s): #44368
    • File PhysicsTools/NanoAOD/python/electrons_cff.py modified in PR(s): #44392
    • File PhysicsTools/NanoAOD/python/muons_cff.py modified in PR(s): #44392
    • File PhysicsTools/PatAlgos/python/slimming/miniAOD_tools.py modified in PR(s): #44316, #44335, #44383

cmsbuild avatar Mar 13 '24 23:03 cmsbuild

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

cmsbuild avatar Mar 13 '24 23:03 cmsbuild

please test

jfernan2 avatar Mar 14 '24 09:03 jfernan2

-1

Failed Tests: UnitTests RelVals RelVals-INPUT Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cae026/38125/summary.html COMMIT: bafee581a21ce3c2b0cab672d1b1f6bb838c08c5 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 ...

cmsbuild avatar Mar 14 '24 10:03 cmsbuild