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

VID fix & core unit tests + Regression ValueMap Producer code cleanup. (74X) #11232

Merged

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Sep 12, 2015

Backport of #11229.

@ikrav please base your user recipes off of this PR.

@gpetruc @arizzi @davidlange6 Since this now contains a bugfix for the regression it is necessary.

@lgray
Copy link
Contributor Author

lgray commented Sep 12, 2015

@cmsbuild please test

@cmsbuild cmsbuild added this to the Next CMSSW_7_4_X milestone Sep 12, 2015
@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_7_4_X.

VID fix & core unit tests + Regression ValueMap Producer code cleanup. (74X)

It involves the following packages:

RecoEgamma/ElectronIdentification
RecoEgamma/PhotonIdentification

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@Sam-Harper 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano 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

@cmsbuild
Copy link
Contributor

@lgray
Copy link
Contributor Author

lgray commented Sep 14, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Sep 15, 2015

How essential is this one in 74X perspective?
We will start taking data at 3.8 T imminently and making substantive changes mid-way may make things more complicated for analyzers.

From what I noticed already, this PR modifies the miniAOD egamma object momenta.
What else is here that changes the miniAOD content?

@davidlange6 @gpetruc

@lgray
Copy link
Contributor Author

lgray commented Sep 15, 2015

The only super essential change is baef928. The rest of the changes are code clean up and analysis level issues.

There is one additional necessary variable calculation bugfix from @ikrav that could be integrated easily.

@lgray
Copy link
Contributor Author

lgray commented Sep 15, 2015

There are no other modifications to the MiniAOD content in this PR (as it is at present).

@slava77
Copy link
Contributor

slava77 commented Sep 15, 2015

so, I will try to sign off this one today.
@davidlange6 if we can put this in 74X, some prompt delay would be needed, otherwise it goes to some later "natural" time.

@slava77
Copy link
Contributor

slava77 commented Sep 16, 2015

+1

for #11232 baef928

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs once checked with relvals in the development release cycle of CMSSW (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@lgray
Copy link
Contributor Author

lgray commented Sep 18, 2015

@davidlange6 @gpetruc Just to make sure, this PR, #11282, and #11262 will make it in for the MiniAOD reprocessing?

@gpetruc
Copy link
Contributor

gpetruc commented Sep 18, 2015

Hi,
I hope they can go in a patch release as soon as possible, for prompt reco
too.

Giovanni

On Fri, Sep 18, 2015 at 9:49 AM, Lindsey Gray notifications@github.com
wrote:

@davidlange6 https://github.com/davidlange6 @gpetruc
https://github.com/gpetruc Just to make sure, this PR, #11282
#11282, and #11262
#11262 will make it in for the
MiniAOD reprocessing?


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

@davidlange6
Copy link
Contributor

how confident are we that there are not other small fixes needed?

On Sep 18, 2015, at 10:33 AM, Giovanni Petrucciani notifications@github.com wrote:

Hi,
I hope they can go in a patch release as soon as possible, for prompt reco
too.

Giovanni

On Fri, Sep 18, 2015 at 9:49 AM, Lindsey Gray notifications@github.com
wrote:

@davidlange6 https://github.com/davidlange6 @gpetruc
https://github.com/gpetruc Just to make sure, this PR, #11282
#11282, and #11262
#11262 will make it in for the
MiniAOD reprocessing?


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


Reply to this email directly or view it on GitHub.

@lgray
Copy link
Contributor Author

lgray commented Sep 18, 2015

At this point I am pretty confident. The evaluation code now matches what
was used in the training.

On Fri, Sep 18, 2015 at 10:35 AM, David Lange notifications@github.com
wrote:

how confident are we that there are not other small fixes needed?

On Sep 18, 2015, at 10:33 AM, Giovanni Petrucciani <
notifications@github.com> wrote:

Hi,
I hope they can go in a patch release as soon as possible, for prompt
reco
too.

Giovanni

On Fri, Sep 18, 2015 at 9:49 AM, Lindsey Gray notifications@github.com

wrote:

@davidlange6 https://github.com/davidlange6 @gpetruc
https://github.com/gpetruc Just to make sure, this PR, #11282
#11282, and #11262
#11262 will make it in for the
MiniAOD reprocessing?


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


Reply to this email directly or view it on GitHub.


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

@davidlange6
Copy link
Contributor

I mean in the miniAOD - we don’t want too many breakpoints.

On Sep 18, 2015, at 10:56 AM, Lindsey Gray notifications@github.com wrote:

At this point I am pretty confident. The evaluation code now matches what
was used in the training.

On Fri, Sep 18, 2015 at 10:35 AM, David Lange notifications@github.com
wrote:

how confident are we that there are not other small fixes needed?

On Sep 18, 2015, at 10:33 AM, Giovanni Petrucciani <
notifications@github.com> wrote:

Hi,
I hope they can go in a patch release as soon as possible, for prompt
reco
too.

Giovanni

On Fri, Sep 18, 2015 at 9:49 AM, Lindsey Gray notifications@github.com

wrote:

@davidlange6 https://github.com/davidlange6 @gpetruc
https://github.com/gpetruc Just to make sure, this PR, #11282
#11282, and #11262
#11262 will make it in for the
MiniAOD reprocessing?


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


Reply to this email directly or view it on GitHub.


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


Reply to this email directly or view it on GitHub.

@lgray
Copy link
Contributor Author

lgray commented Sep 18, 2015

Translation: the miniaod regressions and mva IDs are now fixed, considering
all three relevant PRs.

(Sent from my Nexus 6)
On Sep 18, 2015 11:42 AM, "David Lange" notifications@github.com wrote:

I mean in the miniAOD - we don’t want too many breakpoints.

On Sep 18, 2015, at 10:56 AM, Lindsey Gray notifications@github.com
wrote:

At this point I am pretty confident. The evaluation code now matches
what
was used in the training.

On Fri, Sep 18, 2015 at 10:35 AM, David Lange notifications@github.com

wrote:

how confident are we that there are not other small fixes needed?

On Sep 18, 2015, at 10:33 AM, Giovanni Petrucciani <
notifications@github.com> wrote:

Hi,
I hope they can go in a patch release as soon as possible, for
prompt
reco
too.

Giovanni

On Fri, Sep 18, 2015 at 9:49 AM, Lindsey Gray <
notifications@github.com>

wrote:

@davidlange6 https://github.com/davidlange6 @gpetruc
https://github.com/gpetruc Just to make sure, this PR, #11282
#11282, and #11262
#11262 will make it in for
the
MiniAOD reprocessing?


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


Reply to this email directly or view it on GitHub.


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


Reply to this email directly or view it on GitHub.


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

@gpetruc
Copy link
Contributor

gpetruc commented Sep 18, 2015

If we put this in before the prompt of the first collision run of 2015D we
have no breakpoints (beyond the already existing one between 2015C and
2015D), and no need of reprocessing the beginning of 2015D unless other
bugs are found.

There's a possible change in the type1 MET definition (a change in the jet
pt threshold), but that's useful only on MC or re-reco/re-miniaod of old
data, since for prompt reco data the type1 correction will have to be
recomputed anyway when new JEC residuals for 2015D will become available.

Ciao,

Giovanni

On Fri, Sep 18, 2015 at 11:42 AM, David Lange notifications@github.com
wrote:

I mean in the miniAOD - we don’t want too many breakpoints.

On Sep 18, 2015, at 10:56 AM, Lindsey Gray notifications@github.com
wrote:

At this point I am pretty confident. The evaluation code now matches
what
was used in the training.

On Fri, Sep 18, 2015 at 10:35 AM, David Lange notifications@github.com

wrote:

how confident are we that there are not other small fixes needed?

On Sep 18, 2015, at 10:33 AM, Giovanni Petrucciani <
notifications@github.com> wrote:

Hi,
I hope they can go in a patch release as soon as possible, for
prompt
reco
too.

Giovanni

On Fri, Sep 18, 2015 at 9:49 AM, Lindsey Gray <
notifications@github.com>

wrote:

@davidlange6 https://github.com/davidlange6 @gpetruc
https://github.com/gpetruc Just to make sure, this PR, #11282
#11282, and #11262
#11262 will make it in for
the
MiniAOD reprocessing?


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


Reply to this email directly or view it on GitHub.


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


Reply to this email directly or view it on GitHub.


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

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Sep 23, 2015
VID fix & core unit tests + Regression ValueMap Producer code cleanup. (74X)
@cmsbuild cmsbuild merged commit ed6ac77 into cms-sw:CMSSW_7_4_X Sep 23, 2015
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

5 participants