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 for Legacy2016 Smearings #6

Merged
merged 3 commits into from May 31, 2018

Conversation

previsualconsent
Copy link
Contributor

Discussed in #4

@cmsbuild
Copy link

A new Pull Request was created by @previsualconsent (Peter Hansen) for branch master.

@cmsbuild, @smuzaffar, @gudrutis, @mrodozov can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

external issue cms-sw/cmsdist#4048

@previsualconsent previsualconsent mentioned this pull request May 29, 2018
@slava77
Copy link

slava77 commented May 29, 2018

@Sam-Harper
I just wanted to confirm that this is the only change we need to fix the smearing

@slava77
Copy link

slava77 commented May 29, 2018

I see that the file already present in CMSSW from #4 is changed in the same way in this PR.
The changes are in agreement with slide 6 in https://indico.cern.ch/event/697082/contributions/3018397/attachments/1656254/2651425/2018-05-25-EGM-ScalesSmearings.pdf (with more decimal points though).
So, this looks as expected.

The tests from cms-sw/cmsdist#4036 should be applicable to changes from this PR as well.
cms-sw/cmsdist#4036 (comment) shows that the jenkins tests pass with differences in wf 136.7611 (the 2016 re-miniAOD). All changes in this workflow are in slimmedPhotons and slimeedElectrons in userFloats, as expected.
So, I think that we can proceed with merging this PR and also the matching cmsdist PRs (after tests, if necessary to confirm the same changes)

@fabiocos
Copy link

+1

@fabiocos
Copy link

@smuzaffar we need a cmsdist entry for master corresponding to the PR, and a corresponding one in 9_4_X

@cmsbuild
Copy link

Pull request #6 was updated.

external issue cms-sw/cmsdist#4048

@slava77
Copy link

slava77 commented May 30, 2018

@previsualconsent @Sam-Harper
please comment on the apparently continuing updates of these external files.
Is this a work in progress and we should hold off the start of the 94X 2016 re-mini?

The last set of updates seems like a significant bugfix.
Is it certain that this is now the last of it?

@cmsbuild
Copy link

Pull request #6 was updated.

external issue cms-sw/cmsdist#4048

@Sam-Harper
Copy link
Contributor

Hi Slava, this was discovered today

This is a significant bug fix which meant that the MC didnt scale the uncertainties propagated to them. It slipped through my validation.

This should be the last of them.

@previsualconsent
Copy link
Contributor Author

Yes, I was unaware of a specific use case that is now covered by the latest two commits.

@slava77
Copy link

slava77 commented May 30, 2018

is another commit 5 mins ago the last one?

I think it's worth giving you ~8 hours to double/triple-check.

@Sam-Harper
Copy link
Contributor

agreed!

@smuzaffar
Copy link
Contributor

assign reconstruction
please sign it before we merge and update cmsdist

@cmsbuild
Copy link

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link

slava77 commented May 31, 2018

@Sam-Harper @previsualconsent
I just wanted to check with you that the final double/triple-checks are complete and you do not expect to make more changes.
Please confirm.
Thank you.

@Sam-Harper
Copy link
Contributor

We expect no more changes. The MC is correctly getting scale uncertainties with a central scale value of 1. Which means the changes we added worked.

@slava77
Copy link

slava77 commented May 31, 2018

+1

@smuzaffar @mrodozov
please merge this.
We can then run the standard tests after cms-sw/cmsdist#4036 (or an equivalent) is updated

@mrodozov mrodozov merged commit f8e79fe into cms-data:master May 31, 2018
fabiocos added a commit to fabiocos/cmsdist that referenced this pull request May 31, 2018
cmsbuild added a commit to cms-sw/cmsdist that referenced this pull request Jun 1, 2018
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

7 participants