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

EGM ID Update 80X #12740

Merged
merged 6 commits into from Dec 16, 2015
Merged

EGM ID Update 80X #12740

merged 6 commits into from Dec 16, 2015

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Dec 10, 2015

Forward port of #12738.

New photon ID added to 80X MiniAOD.

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/PatAlgos
RecoEgamma/EgammaTools
RecoEgamma/PhotonIdentification

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

Following commands in first line of a comment are recognized

  • +1|approve[d]|sign[ed]: L1/L2's to approve it
  • -1|reject[ed]: L1/L2's to reject it
  • assign <category>[,<category>[,...]]: L1/L2's to request signatures from other categories
  • unassign <category>[,<category>[,...]]: L1/L2's to remove signatures from other categories
  • hold: L1/all L2's/release manager to mark it as on hold
  • unhold: L1/user who put this PR on hold
  • merge: L1/release managers to merge this request
  • [@cmsbuild,] please test: L1/L2 and selected users to start jenkins tests
  • [@cmsbuild,] please test with cms-sw/cmsdist#<PR>: L1/L2 and selected users to start jenkins tests using externals from cmsdist

@lgray
Copy link
Contributor Author

lgray commented Dec 10, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

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

@lgray
Copy link
Contributor Author

lgray commented Dec 10, 2015

@ikrav

@cmsbuild
Copy link
Contributor


void PhoAnyPFIsoWithEAAndQuadScalingCut::getEventContent(const edm::EventBase& ev) {
ev.getByLabel(contentTags_[anyPFIsoWithEA_],_anyPFIsoMap);
ev.getByLabel(contentTags_[rhoString_],_rhoHandle);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to use getByToken() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary for the classes to operate in (py)FWLite and full CMSSW.
The consumes interface does not exist in edm::EventBase.

Copy link
Contributor

Choose a reason for hiding this comment

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

An explanatory comment to that effect would be helpful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You cannot call consumes in FWLite and thus getbytoken cannot work. Likewise the accessor's are not available in edm::EventBase and cannot be used in this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dr15Jones - can we can this behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidlange6 Are you asking if we can get rid of this behavior? That is you want us to invent a new mechanism in FWLite to allow equivalent consumes behavior?

@cvuosalo
Copy link
Contributor

+1

For #12740 28bb2aa

Adding a new photon ID.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_0_X_2015-12-09-1600 show no significant differences. Several other PRs were included by Jenkins in the tests, and they cause differences not related to this PR. An extended test of workflow 134.705_RunMET2015B with 200 events against baseline CMSSW_8_0_X_2015-12-10-1100 also shows no significant differences and no significant change in the size of event content for RECO or Mini-AOD.

@cvuosalo cvuosalo mentioned this pull request Dec 11, 2015
@lgray
Copy link
Contributor Author

lgray commented Dec 13, 2015

@monttj @vadler Please review this PR.

davidlange6 added a commit that referenced this pull request Dec 16, 2015
@davidlange6 davidlange6 merged commit 222d08c into cms-sw:CMSSW_8_0_X Dec 16, 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

6 participants