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

JER MET uncertainties and METSignifiance re-estimation #16435

Merged
merged 3 commits into from Nov 17, 2016

Conversation

mmarionncern
Copy link

This PR enables the re-computation of the MET significance for the corrected METs on PAT, and enable the JER uncertainty propagation to the MET when the jets are not smeared.

Packages touched:

  • PhysicsTools/PatAlgos
  • RecoMET/METAlgorithms

Format and content changes :

  • JER uncertainties for non-smeared not set to the central value anymore
  • central MET significance is not supposed to change when reprocessing AODs (i.e. in slimmedMET)

No real changes of performances/miniAOD size is expected. the MET significance changes only has an effect when the MET is recomputed on top of miniAOD, but the covariance matrix is not recomputed, which means that CPU ressources stays basically the same. The jet smearing used to compute the JER variation is small and does not reduce significantly the processing rate

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2016

A new Pull Request was created by @mmarionncern for CMSSW_8_1_X.

It involves the following packages:

PhysicsTools/PatAlgos
PhysicsTools/PatUtils
RecoMET/METAlgorithms

@cmsbuild, @cvuosalo, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@TaiSakuma, @gouskos, @imarches, @ahinzmann, @acaudron, @gpetruc, @rappoccio, @jdolen, @nhanvtran, @JyothsnaKomaragiri, @gkasieczka, @schoef, @ferencek, @mverzett, @mariadalfonso, @pvmulder this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@slava77
Copy link
Contributor

slava77 commented Nov 2, 2016

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16154/console

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2016

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2016

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 2, 2016

@@ -298,5 +307,7 @@ class SmearedJetProducerT : public edm::stream::EDProducer<> {
std::mt19937 m_random_generator;

GreaterByPt<T> jetPtComparator;

int _nomVar;
Copy link
Contributor

Choose a reason for hiding this comment

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

leading underscore is frowned upon.

Please change to m_nomVar to be more in style with all other member data

@@ -20,16 +20,18 @@ Description: [one line class summary]

metsig::METSignificance::METSignificance(const edm::ParameterSet& iConfig) {

edm::ParameterSet cfgParams = iConfig.getParameter<edm::ParameterSet>("parameters");
// if(iConfig.exists("parameters")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this commented out "if" is also the reason for the unnecessary re-indentation of the code in metsig::METSignificance::METSignificance

Copy link
Author

Choose a reason for hiding this comment

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

not needed anymore, forgot to remove it during the development period, going to be removed

@@ -234,7 +243,7 @@ class SmearedJetProducerT : public edm::stream::EDProducer<> {
}

double dPt = jet.pt() - genJet->pt();
smearFactor = 1 + (jer_sf - 1.) * dPt / jet.pt();
smearFactor = 1 + _nomVar*(jer_sf - 1.) * dPt / jet.pt();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably missing something.
What is the purpose of this "101" shift?
For +/-101 we get the same NOMINAL jet_sf value and you will smear by the full +/- |jet_sf -1|
while the systematics on the JER can be smaller or larger

Looking at
https://twiki.cern.ch/twiki/bin/view/CMS/JetResolution#JER_Scaling_factors_and_Uncertai
for eta 0 to 0.5 JER SF is 1.122 +-0.026 or "jer_sf - 1 = 0.122 +-0.026".

So, smearing by +-0.122 in this case is not meaningful, but maybe there is some other use case outside the nominal definition of JER.
Please clarify.

Copy link
Author

Choose a reason for hiding this comment

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

@slava77 Here the total semaring factor is basically smear by 1-0.122 or 1+0.122. This is the same logic as the previous code, but the scale factor is used instead of the uncertainties associated to the scale factor

@slava77
Copy link
Contributor

slava77 commented Nov 4, 2016

@mmarionncern
looking at jenkins tests I see that changes are only in MC in MetUnc_JetRes plots. I suppose, this is expected.

Changes in MET are somewhat large
e.g. 25202 now has deltas up to 8 and there is apparently an overflow as well for dMET>10.
wf25202_metunc_jetresdown

Is the "101" variation (vary by 100% of "JER SF -1") now used for these plots?

@zdemirag
Copy link
Contributor

zdemirag commented Nov 4, 2016

Hi Slava,

Yes it is expected that the MET uncertainty for JER changes.

When we did our validation we saw that these very large tails are coming from events with MET ~ couple of GeV range (mostly event < 1 GeV).

This is from Z(mm) relval:

pastedgraphic-1

We can explicitly check the relval you posted as well and identify the tail events if needed.

@slava77
Copy link
Contributor

slava77 commented Nov 4, 2016

On 11/4/16 12:22 PM, Zeynep Demiragli wrote:

Hi Slava,

Yes it is expected that the MET uncertainty for JER changes.

Is it from using the new "101" variation type or is it from other
changes in this PR?

As I mentioned inline in the code, the meaning of "101" variation does
not appear to be what JER systematics prescription should do for a
physics analysis.
But maybe there is a different definition here.

When we did our validation we saw that these very large tails are coming
from events with MET ~ couple of GeV range (mostly event < 1 GeV).

This is from Z(mm) relval:

pastedgraphic-1
https://cloud.githubusercontent.com/assets/5181922/20019356/365ff036-a2cc-11e6-8f2a-991a52325a80.png

We can explicitly check the relval you posted as well and identify the
tail events if needed.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16435 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbjDlXSyIHi25i6qTE1G172wPIOKNks5q64YOgaJpZM4Knc7q.

@zdemirag
Copy link
Contributor

zdemirag commented Nov 4, 2016

This is indeed using the new 101 definition. For the Type1 MET we defined the JER uncertainty as the JER variation itself. For the Smeared MET JER variation is the variation on the JER SF uncertainty.

@slava77
Copy link
Contributor

slava77 commented Nov 5, 2016

On 11/4/16 1:30 PM, Zeynep Demiragli wrote:

This is indeed using the new 101 definition. For the Type1 MET we
defined the JER uncertainty as the JER variation itself. For the Smeared
MET JER variation is the variation on the JER SF uncertainty.

I understand the logic.
Thanks for the explanation.

Since the deltas looked too large IMO,
I printed out the values

e.g
SmearedPATJetProducer:shiftedPatJetResUp,
SmearedPATJetProducer:shiftedPatJetResDown all go through the same jet
and have a value or jer_sf which is equal to the central value of
jer_sf, not of 1+uncertainty.

genJet: 84.109 1.62808 2.9893 recJet: 105.08 1.60761 3.00653 jer_sf 1.115

From your description, I would expect to see here 1.03
(eta bin 1.3–1.7 JER in 80X is 1.115 +-0.03)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16435 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbsm9G3LrS743_9flwG2Y0HW0VipUks5q65XigaJpZM4Knc7q.

@slava77
Copy link
Contributor

slava77 commented Nov 14, 2016

On 11/14/16 2:04 AM, mmarionncern wrote:

Hi Slava, sorry for the late answer :

            isn't JER scaling the standard central value for the jets?

I am not sure I understand the comment, but if I take the statement
literally, yes, it is basically doing that when we have access ot the
genJet.

Just to clarify: I expect that jets used in analysis for their central
values of energy have their energy shifted according to the JER
prescription.

The deltas that you propose in this PR suggest that they should serve
only analyses that do not use JER prescription and instead use jets
corrected only for JEC.
JER is a standard prescription: why would it be ignored?

@schoef ?

            Do we need to add uncertainties to the unified framework
            for the cases with

              * jets not corrected by residual corrections
              * jets not corrected by L123 (e.g. provide raw jets
                scaled by 1/JEC) ?

I don't think so, people are not supposed to use raw jets. They can
rederive such uncertainties by modifying the level of JEC correction in
the MET tool, but I would let it as it is for experts only rather than
making them accessible to everybody with an extra probability to use the
wrong uncertainties.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16435 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbnjRX5NE08mmdR1k02b717_WaYqxks5q-DJJgaJpZM4Knc7q.

@schoef
Copy link
Contributor

schoef commented Nov 14, 2016

@slava77

I think we agree that the prescription makes sense: In the smeared case, propagate the JER smearing uncertainty and in the unsmeared case take a suitable difference to the smeared case as a proxy to the data/MC mismatch.

To answer your question, JER SF are on a much different timescale than JEC and have a much smaller impact for most analysis. Hence, algorithmically, it makes sense to implement the "101" uncertainty for the unsmeared MET. We've had plenty of use cases in the past.

You are right about the lack of documentation though, we'll prepare material and (at least) discuss it during the next MET meeting. It should eventually go to the supporting AN for the MET paper.

Robert

@slava77
Copy link
Contributor

slava77 commented Nov 14, 2016

On 11/14/16 4:15 AM, schoef wrote:

@slava77 https://github.com/slava77

I think we agree that the prescription makes sense: In the smeared case,
propagate the JER smearing uncertainty and in the unsmeared case take a
suitable difference to the smeared case as a proxy to the data/MC mismatch.

To answer your question, JER SF are on a much different timescale than
JEC and have a much smaller impact for most analysis. Hence,
algorithmically, it makes sense to implement the "101" uncertainty for
the unsmeared MET. We've had plenty of use cases in the past.

Hi Robert

Thank you for the clarification.

Have we really had experience with a large fraction of phase space of
jets having JER SF below 1?
I'm asking because the "101" prescription assumes the mean is 1.0 and
gives equal weights to over and under measurements.

Looking at JME-13-004 (fig 36 or Fig 38 for JER and Fig 44 for JEC
uncertainty), for central jets (say, eta~0)

At pt~30 GeV JER is about 20%.
The "101" prescription will "smear" this by 12%, for a total of 0.2*0.12
= 2.4%
Total JEC uncertainty in the barrel is 1.8%

For 100 GeV JER is about 10%
The "101" prescription will "smear" these jets by 1.2%
Total JEC uncertainty in the barrel is 0.9%

In both cases the "101" prescription is larger than the JEC uncertainty
on event by event basis.
I guess the point is that on a large sample, the JER "101" uncertainty
averages out to a much smaller number due to its stochastic nature.

Clearly, the averaging doesn't work for events (from background) which
are in the tails with a significant fraction of jets over or under-measured.
(Isn't it the case for all MC-driven analyses?)
I suppose, these analyses have to apply JER SF and use the normal
prescription.

Did I get the motivation and caveats for the "101" prescription more or
less correctly with the above?

    --slava

You are right about the lack of documentation though, we'll prepare
material and (at least) discuss it during the next MET meeting. It
should eventually go to the supporting AN for the MET paper.

Robert


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16435 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbuGHc9CxiFbgkV_VAoD1KzBGRrWpks5q-FDGgaJpZM4Knc7q.

@slava77
Copy link
Contributor

slava77 commented Nov 14, 2016

+1

for #16435 27b0a91

  • code changes are in line with the description and the follow up discussion/clarification and is to be used to support existing analysis use cases. (despite, IMHO, not very well conceived logic behind the implementation).
  • jenkins tests pass and comparisons with baseline show differences in JetRes{Up,Down} as expected

@slava77
Copy link
Contributor

slava77 commented Nov 14, 2016

@cmsbuild please test

unclear if error in 80X PR #16174 is specific to that PR

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 14, 2016

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16343/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Nov 15, 2016

Regarding 80X PR test problems: looks like the same issue is not appearing in 81X.
Looks like 81X is different enough.

@slava77
Copy link
Contributor

slava77 commented Nov 16, 2016

@davidlange6
due to (future) need to port this back to 80X and also this affecting only miniAOD, I think it's better to have this in 81X.
I suppose, we wait until the 90X version is merged and goes through a few IBs,
that's in case we have a short term plan for an 810+ release.

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