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

New E/gamma pat::Electron and pat::Photon modifiers #17138

Merged
merged 4 commits into from
Jan 13, 2017

Conversation

Sam-Harper
Copy link
Contributor

I wish to embed VID & HEEP V7.0 information into pat::Electrons (and by extension pat::Photons).

This information is stored as a set of ValueMaps. Currently there is
EGExtraInfoModifierFromFloatValueMaps
EGExtraInfoModifierFromIntValueMaps

in RecoEgamma/EgammaTools which do exactly what I need for floats and ints, however I need it for other data types. I diffed EGExtraInfoModifierFromIntValueMaps and EGExtraInfoModifierFromFloatValueMaps to ensure that the only differences were due to types.

As such I propose replacing EGExtraInfoModifierFromIntValueMaps and EGExtraInfoModifierFromFloatValueMaps with a templated class EGExtraInfoModifierFromValueMaps which can read an arbitrary type of ValueMap and write that value as another arbitrary type to the pat::Electron,pat::Photon. The option to write it as an arbitrary type is necessary to for the bool and uints as pat::PATObject has no way to store these values. I could have forced them to always store as an int but I dislike that as it violates the principle of least surprise and also if in the future pat::PATObject gains the ability to store bools and uints, this makes it easy to adapt to that case while retaining backwards compatibility.

Note, EGExtraInfoModifierFromIntValueMaps and EGExtraInfoModifierFromIntValueMaps are part of the miniAOD sequence however I see no changes when the new class. As it simply stores the value in pat::PATObject::userFloat and pat::PATObject::userInt, it should either clearly work or not.

Now I hope I have named the files correctly and they are both in the correct location, I wasnt sure exactly what CMS filenaming policy is here for templates classes.

So far E/gamma has been informed about my request but as its the start of the year, I havent heard back from them yet.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Sam-Harper for CMSSW_9_0_X.

It involves the following packages:

RecoEgamma/EgammaTools

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@rafaellopesdesa, @lgray this is something you requested to watch as well.
@davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@slava77
Copy link
Contributor

slava77 commented Jan 10, 2017

however I need it for other data types
which other data types will you need and why can't they instead be the float and int which are already supported?
I doubt we will extend the userInt/userFloat to have also userBool,userShort,userUint etc.

@slava77
Copy link
Contributor

slava77 commented Jan 10, 2017

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 10, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/17230/console Started: 2017/01/10 15:01

@Sam-Harper
Copy link
Contributor Author

Hi Slava,

I need to be able to read value maps of uint, bool and vid::CutFlowResult.

The class by default attempts to add the object to userData however when its writing an int or float, it uses userFloat or userInt through specialisations for those types.

Thus I can store my bool as a userInt, I read the edm::ValueMap and then write it to the pat::Electron as a userInt.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Jan 13, 2017

+1

for #17138 8ce0049

  • code changes are in line with the description
  • jenkins tests pass and comparisons with baseline show no differences as expected
  • paranoia check: copy of jenkins outputs for 136.731 and branch-to-branch comparison shows no changes in values based on compressed size

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

@davidlange6 davidlange6 merged commit 47f689a into cms-sw:CMSSW_9_0_X Jan 13, 2017
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.

4 participants