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

Updates to the L1 prefiring weight producer #32728

Merged
merged 4 commits into from Feb 4, 2021

Conversation

lathomas
Copy link
Contributor

PR description:

This PR provides updates the L1 prefiring weight producer.
This is a revised version of #32542 that includes some update and fixes conflicts with recent commits.

  1. Protection against muon jets ( PhysicsTools/PatUtils/plugins/L1ECALPrefiringWeightProducer.cc )
    In the L1 prefiring weight producer, a customized protection is added to veto jets mostly made of muons that have very little chance to prefire (since prefiring is related to the jet EM energy).
    The default setting considers only jets with a muon energy fraction <0.5. A value of -1 can be used to switch off the protection.
    Physics wise, the change has often a negligible impact, except for (very) precise measurements including muons in the final states (e.g. Wmass) or final states involving high pt forward muons (e.g. Z'->mumu).

  2. Link to new UL2017 maps and enabling/disabling the muon jet protection. (PhysicsTools/NanoAOD/python/triggerObjects_cff.py)

  • New prefiring maps for Ultra Legacy 2017 are now available and merged in:
    Adding UL17 prefiring maps for photons and jet cms-data/PhysicsTools-PatUtils#1
  • The updated UL2017 maps and the muon jet protection are used by default for next campaigns.
  • Earlier configurations can be retrieved through modifiers.
  • I still have issue figuring out how to properly configure the producer for the modifier "run2_nanoAOD_106Xv1" (since different maps need to be used for 2016 and 2017).

PR validation:

Validation of the muon protection (setting JetMaxMuonFraction = 0.5) was performed on a QCD and a high mass Z->mumu samples.
QCD is untouched, prefiring weights increase as expected in ZMuMu.
prefQCDHT500To700
prefPRzmumum400to800

The new maps were presented at this meeting:
https://indico.cern.ch/event/985369/#3-ul2017-prefiring-maps-tbc

if this PR is a backport please specify the original PR and why you need to backport that PR:

Not a backport. I understand a backport is needed for 10_6_X. Do we also need one for 11_2? Thanks !

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32728/20871

  • This PR adds an extra 20KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/NanoAOD
PhysicsTools/PatUtils

@perrotta, @gouskos, @cmsbuild, @fgolf, @slava77, @jpata, @mariadalfonso can you please review it and eventually sign? Thanks.
@jdamgov, @emilbols, @gouskos, @swertz, @jdolen, @ahinzmann, @smoortga, @schoef, @rappoccio, @mariadalfonso, @JyothsnaKomaragiri, @nhanvtran, @gkasieczka, @clelange, @hatakeyamak, @ferencek, @gpetruc, @andrzejnovak, @peruzzim, @seemasharmafnal, @mmarionncern this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@@ -219,7 +219,22 @@
)

from PhysicsTools.PatUtils.L1ECALPrefiringWeightProducer_cff import prefiringweight
run2_HLTconditions_2016.toModify(prefiringweight, DataEra = cms.string("2016BtoH"))
from PhysicsTools.NanoAOD.nano_eras_cff import *
from PhysicsTools.NanoAOD.common_cff import *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add the run2_HLTconditions_2016 and run2_HLTconditions_2017 in the general eras cff ?
and move these lines in the first part of the file ?

Comment on lines +225 to +227
run2_jme_2016.toModify( prefiringweight, DataEra = cms.string("2016BtoH"))
#Next line is for UL2017 maps
run2_jme_2017.toModify( prefiringweight, DataEra = cms.string("UL2017BtoF"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the 2018 (UL) the default is what is in the fillDescription i.e.
desc.addstd::string("DataEra", "2017BtoF");
is this correct ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But there's no prefiring in 2018 so that shouldn't be used. Currently the prefiring weights are not even computed/saved for 2018.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.
indeed themc106Xul17_doc.html#L1PreFiringWeight
is not stored in the 2018
mc106Xul18_doc.html

run2_nanoAOD_94X2016.toModify( prefiringweight, JetMaxMuonFraction = cms.double(-1.) )
#For first UL reNANOAOD (run2_nanoAOD_106Xv1), same thing
run2_nanoAOD_106Xv1.toModify( prefiringweight, JetMaxMuonFraction = cms.double(-1.) )
#One still needs to correct the maps for run2_nanoAOD_106Xv1: it should be "2016BtoH" for 2016 and "2017BtoF" for 2017, not sure how to do that...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we are in master, we can safely update the nano when reading the miniUL v1.
I suggest to remove this customisation run2_nanoAOD_106Xv1 and only restore the pre-UL.
in this way we will have the correct maps and what you haven the comments is solved.

When we backport to 10_6 for which we have an ongoing production on mini of type 106Xv1 we will see how to do this

@mrodozov
Copy link
Contributor

mrodozov commented Jan 26, 2021

note you still need cms-sw/cmsdist#6551 for the cms-data/PhysicsTools-PatUtils#1 changes.
cms-data/PhysicsTools-PatUtils#1 will not be in the release until 6551 and this PR are merged

@mrodozov
Copy link
Contributor

test parameters:

@perrotta
Copy link
Contributor

perrotta commented Feb 1, 2021

@lathomas @mariadalfonso what is the status of this PR? Is it planned to move forward?

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32728/20946

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 1, 2021

Pull request #32728 was updated. @perrotta, @gouskos, @cmsbuild, @fgolf, @slava77, @jpata, @mariadalfonso can you please check and sign again.

@perrotta
Copy link
Contributor

perrotta commented Feb 1, 2021

@cmsbuild please test

@mariadalfonso
Copy link
Contributor

please abort

(it seems that yesterday tests are stuck)

@mariadalfonso
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23483a/12655/summary.html
COMMIT: bba2ce3
CMSSW: CMSSW_11_3_X_2021-02-02-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/32728/12655/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 14 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2752926
  • DQMHistoTests: Total failures: 16
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2752887
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@mariadalfonso
Copy link
Contributor

mariadalfonso commented Feb 3, 2021

+xpog

small changes in the l1PreFiringEventWeightTable as expected

all_OldVSNew_TTbar13nanoEDM106Xv1in2017wf1325p81c_nanoaodFlatTable_l1PreFiringEventWeightTable__DQM_obj_floats__L1PreFiringWeight_Dn_500

@@ -135,7 +137,8 @@ void L1ECALPrefiringWeightProducer::produce(edm::StreamID, edm::Event& iEvent, c
continue;
if (fabs(eta_jet) > 3.)
continue;

if (jetMaxMuonFraction_ > 0 && jet.muonEnergyFraction() > jetMaxMuonFraction_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks a bit counterintuitive to me that a max muon fraction of -1 can mean "all muons," in fact.

I would have rather set such a max fraction to 1 for it (let say "999." or so in the config", so that the first part of the condition can be removed).

On the other hand, this escamotage can avoid uselessly computing the jet.muonEnergyFraction(), and as such it improves the performance of the code.

Still I would have preferred something like

Suggested change
if (jetMaxMuonFraction_ > 0 && jet.muonEnergyFraction() > jetMaxMuonFraction_)
if (jetMaxMuonFraction_ < 1 && jet.muonEnergyFraction() > jetMaxMuonFraction_)

This being just a matter of taste, I won't consider it relevant for granting the reco signature to this PR

@perrotta
Copy link
Contributor

perrotta commented Feb 3, 2021

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 3, 2021

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 1121332 into cms-sw:master Feb 4, 2021
@mariadalfonso
Copy link
Contributor

mariadalfonso commented Feb 25, 2021

@lathomas
can we have a backport in 11_2 (should be identical to master)

ping

@mariadalfonso
Copy link
Contributor

mariadalfonso commented Mar 29, 2021

@lathomas
can you prepare the backport to 10_6_X ?

note that we need to safeguard the current ongoing v8 production in 10_6_X
(run2_nanoAOD_106Xv1 & ~run2_nanoAOD_devel).toModify(xyzTable , xyxVariable = None)

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.

None yet

6 participants