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

Fix calculation of smeared MET on MiniAOD #36165

Closed

Conversation

michaelwassmer
Copy link
Contributor

PR description:

This pull request fixes the incorrect calculation of the nominal smeared missing transverse energy (MET). The underyling problem is the application of the JER correction factors. These factors are applied in PhysicsTools/PatUtils/interface/SmearedJetProducerT.h but later not considered when reverting to the uncorrected jets in JetMETCorrections/Type1MET/interface/PFJetMETcorrInputProducerT.h.
This results in the problem that the JER correction factors in the smeared MET calculation effectively cancels and therefore the smeared MET is basically the same as the regular type1-corrected MET. After the changes shown below, the nominal smeared MET distributions from MiniAOD are similar to the ones obtained from NanoAOD.

This pull request includes the following:

PhysicsTools/PatUtils/plugins/SmearedJetProducer.cc

  • adds a template function (SmearJet in SmearedJetProducer_namespace) to apply the JER
  • if the type of the jet is not pat::Jet it reproduces what has been done so far (using the scaleEnergy function to apply the JER factor to the jet)
  • if the jet is of type pat::Jet, it does the same but additionally adds the JER factor as a userFloat to the pat::Jet (needed to later remove JER from the jets in the corresponding MET module)

PhysicsTools/PatUtils/interface/SmearedJetProducerT.h

  • use the template function explained above

PhysicsTools/PatUtils/plugins/PATPFJetMETcorrInputProducer.cc

  • edit pat::Jet template specialization of the already existing RawJetExtractor class to consider a possible JER correction factor, which was added as a userFloat previously, during the jet correction removal (if a JER factor was applied, it is now removed by applying 1/factor)
  • pat::Jet template specialization, which retrieves a previously saved JER factor, of the template class/functor explained below

JetMETCorrections/Type1MET/interface/PFJetMETcorrInputProducerT.h

  • add a template class/functor to retrieve JER correction factors
  • in generic implementation it only applies 1 as a factor therefore doing nothing
  • in case of pat::Jet, see above

Finally, there is also a mistake when saving the JERup and JERdown varied smearedMET. The different contributions of the nominal smeared MET correction and the JERup and JERdown variations are not separated correctly from on another. After the changes shown below, the varied smeared MET distributions from MiniAOD are similar to the ones obtained from NanoAOD.

DataFormats/PatCandidates/src/MET.cc

  • to get the corrections/deltas for the JER variations of the smeared MET, the calculation now takes the up/down-varied smeared MET (px,py,...) and subtracts the contributions from the nominal smeared MET corrections (ref.dpx(),ref.dpy(),...) as well as the nominal type1-corrected MET (this->px(),this->py(),...)
  • the previous implementation is a mystery to me :D

PR validation:

  • 'runTheMatrix.py -l limited -i all' worked besides some workflows which had CMSDAS errors despite a working certificate
  • 'scram b runtest' worked

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36165/26718

  • This PR adds an extra 28KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36165/26719

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DataFormats/PatCandidates (reconstruction)
  • JetMETCorrections/Type1MET (reconstruction)
  • PhysicsTools/PatUtils (reconstruction)

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

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Nov 18, 2021

@michaelwassmer
please add a link to a presentation in a JME POG meeting with more details on the physics impact.

How old is this problem and would a backport be needed?

@slava77
Copy link
Contributor

slava77 commented Nov 18, 2021

@cmsbuild please test

@michaelwassmer
Copy link
Contributor Author

michaelwassmer commented Nov 18, 2021

Here is a link to a presentation in the JetMET meeting comparing original and fixed calculation: https://indico.cern.ch/event/1048658/contributions/4405700/attachments/2263616/3842703/SmearedMET_Michael_Wassmer.pdf
Here another link containing plots comparing to NanoAOD (slide 11): https://indico.cern.ch/event/1077259/contributions/4530656/attachments/2312550/3935829/METreport2.pdf
The problem is quite old I think, maybe @mcremone , @lathomas , @alkaloge , @kirschen , @camclean can comment on how old. A backport could be relevant for people who would like to use smeared MET in their analysis, either with legacy or ultra-legacy samples.

@cmsbuild
Copy link
Contributor

-1

Failed Tests: HeaderConsistency
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a446a/20609/summary.html
COMMIT: 295e635
CMSSW: CMSSW_12_2_X_2021-11-18-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36165/20609/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: 727 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3327156
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3327134
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Dec 20, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a446a/21407/summary.html
COMMIT: a1851de
CMSSW: CMSSW_12_3_X_2021-12-20-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36165/21407/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: 765 differences found in the comparisons
  • DQMHistoTests: Total files compared: 43
  • DQMHistoTests: Total histograms compared: 3461688
  • DQMHistoTests: Total failures: 12
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3461654
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
  • Checked 181 log files, 42 edm output root files, 43 DQM output files
  • TriggerResults: no differences found

@errai-
Copy link

errai- commented Dec 21, 2021

@michaelwassmer @jpata I was directed here from the newly introduced critical bugfix, as we have both edited the .cc file of pat::Jet. This fix seems to be a very welcome one, but I still needed to go through the whole code patch to check that there are no underlying conflicts.

What I'm slightly worried about here is that quite a lot of new code have been introduced for the seemingly simple problem of undoing and redoing the jer smearing. If I understood correctly, the main point is to be able to undo the smearing scale after jet.correctedP4("Uncorrected") is called in this code.

Has it been recognized that this issue could have been solved with lots of less new code by simply calling jet.correctedP4("Unscaled") instead of jet.correctedP4("Uncorrected") and leaving the new parts with jet.jerFactor() out? From 10_6_X onwards, the pat::Jet::scaleEnergy function appends any additional jet energy scales by the title "Unscaled" in to the beginning of the JEC level vector. This function is most of all utilized by the jet smearing routines.

In short: if smearing is applied to a pat::Jet using the scaleEnergy function, one can undo the JECs and JER smearing simultaneously by referring to the "JEC level" "Unscaled" instead of "Uncorrected". In addition, if any additional energy scales have been applied, referring to "Unscaled" in correctedP4() automatically refers to the point where all energy scales are undone. In this sense, using "Unscaled" and the already existing structures is a more universal solution than the one proposed here. The only "additional" check that would be needed, is checking if the "lowest" available correction level is "Unscaled" or "Uncorrected".

Tagging also @ahinzmann , who was involved in introducing the "Unscaled" correction level in the first place.

EDIT: reading the comments with a deepened thought, the issue is probably deeper and a universal behavior also for non-pat::Jet's might probably require the sorts of additions as introduced here. However, the question and standpoint remain: how much could this PR be shortened by the usage of jet.correctedP4("Unscaled"). The complexity and deep inheritance of the current CMSSW build are already a problem when bugtracking and development are considered. Hence I think it's strongly encouraged to vigilantly minimize the amount of new code introduced in any new PR's.

@michaelwassmer
Copy link
Contributor Author

Hi @errai-
I wasn't aware of this "Unscaled" correction level. It seems that it would become a much simpler fix and I don't mind trying it out. I found the code where the "Unscaled" is introduced, however I couldn't quickly find where the "Uncorrected" level is created. Would you mind pointing me to it? Nevertheless, do I understand correctly that with the "Unscaled" level it is possible to revert all applied energy scalings and with the "Uncorrected" only the registered JEC levels?

@errai-
Copy link

errai- commented Dec 21, 2021

Hi @errai- I wasn't aware of this "Unscaled" correction level. It seems that it would become a much simpler fix and I don't mind trying it out. I found the code where the "Unscaled" is introduced, however I couldn't quickly find where the "Uncorrected" level is created. Would you mind pointing me to it? Nevertheless, do I understand correctly that with the "Unscaled" level it is possible to revert all applied energy scalings and with the "Uncorrected" only the registered JEC levels?

Hi @michaelwassmer and thanks for the prompt reply! The level "Uncorrected" is introduced when the pat::Jet is created in association to the jet energy corrections. Actually I'm not exactly sure where the code for this is located (maybe @ahinzmann can help?). I think it's best to give a brief example: take a pat::Jet where the JEC level is set to L3Absolute. The jet corrections (e.g. for correctedP4()) are all called through the function jecFactor, which looks for the corrections within an ordered vector. Simplifying a bit, as the jet is created, the ordered return values of jecFactor look like this:

Uncorrected=0.959391, L1FastJet=0.944361, L2Relative=1, L3Absolute=1

If one would like to swap e.g. to the level L1FastJet, one would need to multiply the current P4 by 0.944361. If one would like to move to L2Rel or L3Abs, the scale is simply 1. (as L3Absolute is a unitary dummy scaling). Here, the level Uncorrected (still) refers to the completely raw jet. The jer smearing is "postulated" onto a jet which already has the jet corrections. After the smearing, the (vector) of jecFactor return values will look like this:

Unscaled=0.936635, Uncorrected=0.959391, L1FastJet=0.944361, L2Relative=1, L3Absolute=1

The factor of the smearing factor is added into the beginning of this vector! This changes the meaning of "Uncorrected": it now refers to the jet with no jet corrections, but smearing is still applied. Calling "Unscaled" gives the completely uncorrected and unsmeared jet. Just as a reminder, it is also possible that scaleEnergy is called in other purposes than jet smearing. If one would call the function with a unitary dummy scale before smearing, one would end up with

Unscaled=0.936635, Unscaled=0.959391, Uncorrected=0.959391, L1FastJet=0.944361, L2Relative=1, L3Absolute=1

That is, there is a dummy "Unscaled" on the left-hand-side of "Uncorrected". However, calling correctedP4("Unscaled") will return the leftmost "Unscaled" case. Note: it is for now recommended to avoid any extra calls to scaleEnergy, as this is related to the bugfix. In MC at L3Abs/L2L3Rel a single call of scaleEnergy from within jet smearing is safe, though.

As an example and continuing with these guidelines, one could replace this code reapplying the jer scale on a raw jet by adding something like the following into the jetcorrextractor file:

if constexpr (std::is_same<T, pat::Jet>::value) {
  rawJetP4 *= rawJet.jecFactor("Uncorrected)/rawJet.jecFactor("Unscaled");
}

@camclean
Copy link
Contributor

@jpata @michaelwassmer I have confirmed with @kirschen that we would like this fix to be available for UL analyses and therefore should also have a backport.

@jpata
Copy link
Contributor

jpata commented Dec 22, 2021

@camclean thanks for the confirmation.

@michaelwassmer @errai- thanks for the discussion on the implementation. I'm not an expert on JME corrections code as such, so I leave it to JME to discuss thoroughly and propose the best implementation. Clearly, if something related ("Unscaled") already exists and we can avoid introducing more layers of complexity, it should be preferred. I could suggest to discuss this next year with all parties involved (at e.g. the JME meeting) before considering this PR for merging.

@errai-
Copy link

errai- commented Dec 22, 2021

@camclean thanks for the confirmation.

@michaelwassmer @errai- thanks for the discussion on the implementation. I'm not an expert on JME corrections code as such, so I leave it to JME to discuss thoroughly and propose the best implementation. Clearly, if something related ("Unscaled") already exists and we can avoid introducing more layers of complexity, it should be preferred. I could suggest to discuss this next year with all parties involved (at e.g. the JME meeting) before considering this PR for merging.

Thanks for the comment @jpata ! I gave @kirschen a quick ping about the topic, in case he hasn't started his holidays yet.

@jpata
Copy link
Contributor

jpata commented Jan 10, 2022

kind ping on this. will there be a discussion in JME about the implementation?

@ahinzmann
Copy link
Contributor

Just my 2 cents. I have no strong opinion whether to implement a dedicated interface for JER in pat::Jet, or keep the current (which uses "unscaled"). One way to address the bug may to replace correctedP4("uncorrected") by correctedP4(0) where necessary. This appears in several places in CMSSW and it's obvious, also how to keep MiniAOD and NanoAOD in sync:
https://github.com/cms-sw/cmssw/search?q=correctedP4

@jpata
Copy link
Contributor

jpata commented Jan 17, 2022

kind ping @michaelwassmer, has there been any discussion on the path forward in JME?

@michaelwassmer
Copy link
Contributor Author

Hi,
sorry for being so silent the last days. There hasn't been much of a discussion, but I think @errai- has suggested the most elegant solution. Whether to use correctedP4("unscaled") or correctedP4(0) is a matter of personal taste I guess. But these suggestions are both better than what I did earlier, since they use already existing methods. I would suggest that we go with "unscaled" for now. I think I can implement this in the next few days. Should I create an new PR for this or just keep working on this one?

@errai-
Copy link

errai- commented Jan 17, 2022

Hi, sorry for being so silent the last days. There hasn't been much of a discussion, but I think @errai- has suggested the most elegant solution. Whether to use correctedP4("unscaled") or correctedP4(0) is a matter of personal taste I guess. But these suggestions are both better than what I did earlier, since they use already existing methods. I would suggest that we go with "unscaled" for now. I think I can implement this in the next few days. Should I create an new PR for this or just keep working on this one?

Just as a quick comment: there might be a slight preference towards @ahinzmann 's correctedP4(0) for two reasons:

  • It's completely future-proof, if additional layears are to be added
  • If unsmeared jets are used, the lack of an 'Unscaled' layer does not produce issues

Admittedly, correctedP4(0) is not very readable/clean code, so that's the downside one would need to accept.

@michaelwassmer
Copy link
Contributor Author

Thanks @errai-
All right, then I would go with correctedP4(0) to obtain the completely uncorrected jet so I don't have to manually use the smearing factor to reverse the smearing correction. I completely agree with this.
However, I have one further question:
The JetMETCorrections/Type1MET/interface/PFJetMETcorrInputProducerT.h module propagates the Type1 MET corrections by getting the completely uncorrected jets (this is clear, see above) and then recalculates from them the

  • jets at L1 correction
  • and the jets at the desired correction level you provide in the corresponding python config (usually L1L2L3).

Then from the residuals of the L1 collection and the L1L2L3 collection it calculates the Type1 MET corrections.
So far so good. For the smeared MET, the only difference is that one needs to consider the smearing corrections on top of the L1L2L3 jet collection. The only problem I currently see is that

rawJetP4 *= rawJet.jecFactor("Uncorrected)/rawJet.jecFactor("Unscaled");

implicitly assumes that the only "difference" between jecFactor("Uncorrected") and jetFactor("Unscaled") (or jecFactor(0)) is the smearing correction. I guess this could be problematic if for any reason the scaleEnergy method was invoked in addition to the one time during the smearing procedure because then one would not be able to get the smearing factor from this ratio. Am I missing something?
If not, I think it could make sense to still have the JER correction interface to keep track of the JER factor or the JER would need to be registered as an additional JEC in the already exiting jecFactors interface. Does this make sense?

@errai-
Copy link

errai- commented Jan 19, 2022

@michaelwassmer all scaleEnergy calls go into the same 'box' with jet smearing, accumulating the total jet energy scale. That is, if one adds extra scales to jets e.g. for the purpose of systematic variations, one needs to consider them in MET calculus exactly in the same manner as the jet smearing scales. Thus for MET, I see no reason of separating smearing scales from other scaleEnergy call types. One could think of all the non-JEC scales as one single generic extra scale that needs to be considered. Does this answer your question, or is there some subtle detail I missed?

Finally, to underline the features of the code: the level "Uncorrected" always exists, but "Unscaled" only exists if scaleEnergy has been called e.g. in the purpose of jet smearing. Hence, jecFactor(0) is the safest and most universal call one can make, as it corresponds to "Uncorrected" in the cases where "Unscaled" is not available for some reason.

@michaelwassmer
Copy link
Contributor Author

I created a new PR (#36797) in which I incorporated the suggested changes. I thought it would probably be cleaner to have a new PR since this one is already quite long.

@jpata
Copy link
Contributor

jpata commented Jan 26, 2022

-reconstruction

@jpata
Copy link
Contributor

jpata commented Feb 22, 2022

@cmsbuild please close

@cmsbuild cmsbuild closed this Feb 22, 2022
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.

8 participants