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

70X met uncertainty tool fixes #4301

Merged

Conversation

schoef
Copy link
Contributor

@schoef schoef commented Jun 18, 2014

Fixed several issues regarding the MET uncertainty tool in PAT

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @schoef for CMSSW_7_0_X.

70X met uncertainty tool fixes

It involves the following packages:

JetMETCorrections/Type1MET
PhysicsTools/PatUtils

@nclopezo, @vadler, @cmsbuild, @Degano, @monttj can you please review it and eventually sign? Thanks.
@TaiSakuma, @nhanvtran this is something you requested to watch as well.
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.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

-1

Tested at: d950177
I ran the usual tests and I found errors in the following unit tests:

---> test runtestPhysicsToolsPatAlgos had ERRORS

you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4301/203/summary.html

@cmsbuild
Copy link
Contributor

@monttj
Copy link
Contributor

monttj commented Jun 30, 2014

@schoef and @TaiSakuma - trying to understand the changes. Isn't this PR in conflict with this #4045?

@schoef
Copy link
Contributor Author

schoef commented Jul 1, 2014

Yes it is.

Tai, I think you, Matthieu and I should chat.
Matthieu and I are trying to get in several changes in 70X/71X/72X.

On Mon, Jun 30, 2014 at 11:53 PM, Tae Jeong Kim notifications@github.com
wrote:

@schoef https://github.com/schoef and @TaiSakuma
https://github.com/TaiSakuma - trying to understand the changes. Isn't
this PR in conflict with this #4045
#4045?


Reply to this email directly or view it on GitHub
#4301 (comment).

@TaiSakuma
Copy link
Contributor

@schoef, sure we can chat.

This PR confilicts with the commit TaiSakuma@9dce9b2.

If we want to make the same change for 70X/71X/72X, you can revert this commit.

I can do the refactoring later after you are done with your change.

@ghost
Copy link

ghost commented Jul 14, 2014

ping
What's the status of this PR?

@schoef
Copy link
Contributor Author

schoef commented Jul 15, 2014

Hi,
the failure of the PR triggered some discussion.
I think we need a little more time to discuss amongst ourselves.
-r

On Mon, Jul 14, 2014 at 10:35 AM, Alessandro Degano <
notifications@github.com> wrote:

ping
What's the status of this PR?


Reply to this email directly or view it on GitHub
#4301 (comment).

@monttj
Copy link
Contributor

monttj commented Jul 15, 2014

+1
It has been delayed due to possible conflict with 72X.
But now it is understood that lots of new features will only appear in 72X.
And among MET developers in AT will have a discussion near future for this.

@mmarionncern
Copy link

All pat MET names have been changed and updated in the default pat configuration and in the miniAOD tool.

@cmsbuild
Copy link
Contributor

Pull request #4301 was updated. @nclopezo, @vadler, @cmsbuild, @Degano, @monttj can you please check and sign again.

@cmsbuild
Copy link
Contributor

@monttj
Copy link
Contributor

monttj commented Jul 30, 2014

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_0_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

davidlange6 added a commit that referenced this pull request Aug 26, 2014
@davidlange6 davidlange6 merged commit de717f5 into cms-sw:CMSSW_7_0_X Aug 26, 2014
@slava77
Copy link
Contributor

slava77 commented Aug 27, 2014

@schoef
In case you haven't seen, this PR was reverted by #5071.
So, we will need to have a new iteration to integrate it back with appropriate corrections. (a new PR will be needed)

@slava77
Copy link
Contributor

slava77 commented Aug 27, 2014

@mmarionncern mmarionncern deleted the 70X-METUncertaintyTool-20140710 branch November 14, 2016 08:04
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