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 naming for fwLite VID in 74X #11513

Merged
merged 1 commit into from Sep 29, 2015

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Sep 27, 2015

Fixes an issue with the naming of embedded objects for VID usage in FWLite.

To be clear: this PR changes the content of photons in MiniAOD during its creation. It is necessary for reMiniAOD campaign, need a release by Wednesday.

This can be patched at analysis level, but requires reconfiguration of the code and makes user recipes much more complicated.

It is included in regression/VID PRs for 76X/75X.

@lgray
Copy link
Contributor Author

lgray commented Sep 27, 2015

@cmsbuild please test

@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.

Fix naming for fwLite VID in 74X

It involves the following packages:

RecoEgamma/EgammaTools

@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

@lgray
Copy link
Contributor Author

lgray commented Sep 28, 2015

@cvuosalo At the request of Slava I updated the header of this PR to be a bit more descriptive and indicate its urgency.

@lgray
Copy link
Contributor Author

lgray commented Sep 28, 2015

@cvuosalo Apologies for spamming: just wanting to make sure this can be signed off relatively quickly?

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@lgray
Copy link
Contributor Author

lgray commented Sep 28, 2015

Already tested, but OK!

@lgray
Copy link
Contributor Author

lgray commented Sep 28, 2015

ah, other merge commits.

@cvuosalo
Copy link
Contributor

@lgray:
Extended tests are underway. I will approve this PR as soon as I can.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

+1

For #11513 a42ae5e

Revising naming of some embedded objects for VID usage in FWLite.

Jenkins tests against baseline CMSSW_7_4_X_2015-09-28-1100 show no significant differences. An extended test of workflow 4.53_RunPhoton2012B with 200 events against baseline CMSSW_7_4_X_2015-09-27-1100 shows no difference in product sizes for either RECO or Mini-AOD.

@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 (tests are also fine) and once validation in the development release cycle CMSSW_7_6_X is complete. This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@cvuosalo
Copy link
Contributor

@lgray:
Just for clarity, could you explain what exactly changes with this PR? How would the change in event content appear?

@lgray
Copy link
Contributor Author

lgray commented Sep 29, 2015

@cvuosalo
If you get a miniAOD photon from the event content and do:

photon.hasUserFloat("phoChargedIsolation") it will now return true.
Previously it would not, and this causes problems downstream in some ID use scenarios.
This is because the isolation variables were named chargedHadronIso, or similar, previously.

There should be little to no change in the miniAOD size since the strings change by a few bytes at most.

@slava77
Copy link
Contributor

slava77 commented Sep 29, 2015

I was expecting the branch size to change if user float names changed.
On Sep 29, 2015 3:54 AM, "Lindsey Gray" notifications@github.com wrote:

@cvuosalo https://github.com/cvuosalo
If you get a miniAOD photon from the event content and do:

photon.hasUserFloat("phoChargedIsolation") it will now return true.
Previously it would not, and this causes problems downstream in some ID
use scenarios.


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

@lgray
Copy link
Contributor Author

lgray commented Sep 29, 2015

@slava77 uncompressed size should change (by a few bytes), for compressed size this is in the noise.

@lgray
Copy link
Contributor Author

lgray commented Sep 29, 2015

@Slava7 especially in 74X since there are so many other embedded objects that start with pho.

@slava77
Copy link
Contributor

slava77 commented Sep 29, 2015

I checked Carl's output files used in the test, the compressed size actually changes:

  • slimmedPhotons change from 1843.09 to 1843.46 bytes/event
  • slimmedElectrons change from 2190.16 to 2190.24 bytes/event
    These are the only branches the differ.
    So, it looks like there is an expected effect from user float name and value changes

@lgray
Copy link
Contributor Author

lgray commented Sep 29, 2015

Good.

@lgray
Copy link
Contributor Author

lgray commented Sep 29, 2015

@gpetruc @davidlange6 Ready to go here.

@lgray
Copy link
Contributor Author

lgray commented Sep 29, 2015

@slava77 Names change, values do not.

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Sep 29, 2015
@cmsbuild cmsbuild merged commit 19c08cb into cms-sw:CMSSW_7_4_X Sep 29, 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