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 JES uncertainties on T1(smear) MET with split JER (indentation) #256

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

pieterdavid
Copy link
Contributor

Since the loop over JES uncertainty sources was put inside the one over JER bins in #244, the JES variations are applied six times instead of once if JER uncertainty splitting is enabled.
I just removed the indentation, but I guess this also allows for some of the line splitting to be reverted - let me know if there's some script/guideline for this.

@pieterdavid
Copy link
Contributor Author

pieterdavid commented Nov 2, 2020

If I may take this opportunity to ask a question about #244: I noticed that all variations are different beween T1 and T1Smear, except for the unclustered energy, which is still a shift applied to the unsmeared (corrected) T1MET. Is this supposed to be also included when using T1Smear (and is it consistent then)? Thanks in advance!

Update: this is mostly answered in #259 - I'll follow the discussion there.

@pieterdavid
Copy link
Contributor Author

ping. Could someone have a look? I may be wrong, but I think that this is a fairly straightforward bugfix

@zdemirag
Copy link

Adding @alpakpinar about the unclustered uncertainty, there is an other PR and discussion on going as well.

@gouskos
Copy link
Contributor

gouskos commented Nov 23, 2020

Hi @alefisico @camclean
(a) The PR looks good to me. Do you have any tests/validation that we can try before merging?
(b) Can you please follow-up on this:

If I may take this opportunity to ask a question about #244: I noticed that all variations are different beween T1 and T1Smear, except for the unclustered energy, which is still a shift applied to the unsmeared (corrected) T1MET. Is this supposed to be also included when using T1Smear (and is it consistent then)?

Thank you!

@alefisico
Copy link

Adding @alpakpinar about the unclustered uncertainty, there is an other PR and discussion on going as well.

sorry are you talking about #253? I think that is only for FatJet no?

@alefisico
Copy link

Hi @alefisico @camclean
(a) The PR looks good to me. Do you have any tests/validation that we can try before merging?
(b) Can you please follow-up on this:

If I may take this opportunity to ask a question about #244: I noticed that all variations are different beween T1 and T1Smear, except for the unclustered energy, which is still a shift applied to the unsmeared (corrected) T1MET. Is this supposed to be also included when using T1Smear (and is it consistent then)?

Thank you!

For (a), from what I understand in the PR, I think the variables itself should not change correct? Maybe @pieterdavid can plot some of the variables with the fix and without the fix?

For (b) including MET conveners @alkaloge @mcremone.

@alkaloge
Copy link

alkaloge commented Nov 23, 2020

Hi,

I am not sure what " I noticed that all variations are different beween T1 and T1Smear, except for the unclustered energy" describes/means actually. @pieterdavid can you clarify ?

PS. Just for the record, given that MET does not have a reco contact, we (MET) have not touched any part of the code for a long as I am aware of, meaning that I can maybe comment on what is there, but we (MET) haven't really implemented any part of the code

@alefisico
Copy link

@alkaloge yes, still most of the changes are done by users. We (JME/JMAR) are working on some validation and a more "controlled" way of implement changes. Hopefully we got something in place soon.

@pieterdavid
Copy link
Contributor Author

For (a), from what I understand in the PR, I think the variables itself should not change correct? Maybe @pieterdavid can plot some of the variables with the fix and without the fix?

@alefisico Attached log file shows the jesTotal(Up,Down) variation with splitJER on and off for 10 events (DY MC). With the code in the master branch the values differ between splitJER=True and splitJER=False, with the PR they are the same (and the same as in the master branch with splitJER=False).

I am not sure what " I noticed that all variations are different beween T1 and T1Smear, except for the unclustered energy" describes/means actually. @pieterdavid can you clarify ?

@alkaloge It's the same question as in #259 : the current code makes MET_T1_* and MET_T1Smear_* branches for all variations (since #244), but only MET_pt_unclustEn(Up,Down) and MET_phi_unclustEn(Up,Down), which are calculated with by adding the MetUnclustEnUpDelta(X,Y) values to the nominal MET_T1 - should these (a) also be used as a variation on the smeared Type-1 MET (which seems a bit strange) (b) not be used then or (c) instead be calculated based on the MET_T1Smear (which seems more consistent, but requires a rename to MET_T1_MetUnclustEn..., and the addition of MET_T1Smear_MetUnclustEn)?

@alkaloge
Copy link

thanks, @pieterdavid now I see the problem. Indeed, I would go with c) (although a) vs c) should be close, but maybe not identical). Please keep in mind that in general MET does not really encourage people to use SmearMET as I don't think we have many use cases nor enough feedback on T1+Smear data/mc comparison.

@pieterdavid
Copy link
Contributor Author

Thanks @alkaloge ! They are close indeed, I implemented the change to check, and pushed it as a separate PR #266

@pieterdavid
Copy link
Contributor Author

I think I answered all the questions and comments two weeks ago, but please let me know if anything is missing.

@gouskos gouskos merged commit 7dd1380 into cms-nanoAOD:master Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants