renamed functions to access PAT isolation variables to avoid conflict wi...

[gzevi]

The functions to access the pat::Photon userIsolation were identically named to the reco::Photon ones.
This was confusing, requiring the users to type reco::Photon::FunctionName() whenever they wanted to access the reco::Photon isolation values.
Now the accessors have been renamed with "pat" in front, to avoid the conflict.

[cmsbuild]

A new Pull Request was created by @gzevi for CMSSW_7_4_X.

renamed functions to access PAT isolation variables to avoid conflict wi...

It involves the following packages:

DataFormats/PatCandidates

@cmsbuild, @vadler, @nclopezo, @monttj can you please review it and eventually sign? Thanks.
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]

+1
Tested at: f2155ae
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7761/2690/summary.html

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7761/2690/summary.html

[monttj]

I am sorry for late response.
But this breaks the consistency with other PAT objects like electrons and muons.
I think it would have been also possible either of to modify reco member function because all isolation function name is different anyway for all objects in reco or to make both values the same?
What do you think?

[gzevi]

I believe we are too late to modify Reco objects in 74X.
I can implement your second suggestion, and change pat::Electron and pat::Muon. This will however brake some analyzers' code, since their old functions on electrons and muons will no longer work.
As a third alternative, we can force the pat::Photon functions to return the reco::Photon isolation values.
What do you think?

[lgray]

@monttj I think it would be better in this case to modify the pat::Object or pat::Lepton class such that these names are changed globally and then simply inform analysis users this higher level change is taking place.

I agree with @gzevi that it is too late to make a change to the core reco objects.

@slava77 any comment here?

[monttj]

@gzevi @lgray
Okay, if it is too late to change reco object, I would be in favour of forcing the pat::Photon functions to return the reco::Photon isolation values than changing the name in PAT since the isolation function is one of the most often used variable in analyses specially for muons and electrons and changing it would introduce a confusion for a long (we would have to answer the same question for a year.)

Besides, for photon, using the new ID/Isolation framework developed by Lindsey, it would be foreseen that the isolation can be calculated on the fly in miniAOD or updated in PAT so the isolation in reco would not be optimal at the end?

[gzevi]

@monttj @lgray
Isn't @monttj's proposal, forcing the pat::Photon functions to return the identically named reco::Photon functions, the same as removing these functions altogether from pat::Photon?
I am very much in favor of this (either forcing or removing), as it will further reduce confusion.
I didn't propose it in the first place, since it also reduces flexibility in the PAT objects.

[lgray]

This is more a call for @monttj at this point, but I am also in favor of
streamlining the stock isolation variables available in pat (so this
proposal is also fine in that respect)

-L
On Mar 14, 2015 7:18 AM, "gzevi" notifications@github.com wrote:

@monttj https://github.com/monttj @lgray https://github.com/lgray
Isn't @monttj https://github.com/monttj's proposal, forcing the
pat::Photon functions to return the identically named reco::Photon
functions, the same as removing these functions altogether from pat::Photon?
I am very much in favor of this (either forcing or removing), as it will
further reduce confusion.
I didn't propose it in the first place, since it also reduces flexibility
in the PAT objects.

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

[monttj]

Hello all, @gzevi @lgray

I thought we already have the same setup for isolation in RECO and PAT area following the POG recommendation for Run 1 using following this configuration.
https://github.com/cms-sw/cmssw/blob/CMSSW_7_5_X/RecoParticleFlow/PFProducer/python/photonPFIsolationValues_cff.py

The idea was to change the parameters in the PAT configuration when the recommendation from POG is updated and if it is too late to implement it in RECO. So in this way, we always keep the PAT value the latest and recommended one for end users.

It looks it is not the case for photons. RECO and PAT values are filled in different way, I guess.
I think we should use the same framework and it should be configurable as well.
Then we can keep both RECO and PAT the same for now and later should be able to configure the value to follow the latest development.

I think the decision as to which framework is used for Run 2 needs to come from POG.

I am fully agree that having the same function name is less confusing.
Please keep the same function name if it can be configured in PAT with the framework POG recommended.

Regards,
Taejeong

[gzevi]

Just to be clear, before I implement it, the proposal of @monttj and @lgray would be to change the following functions in pat::Photon

float chargedHadronIso() const { return userIsolation(pat::PfChargedHadronIso); }
float neutralHadronIso() const { return userIsolation(pat::PfNeutralHadronIso); }
float photonIso() const { return userIsolation(pat::PfGammaIso); }

to

float chargedHadronIso() const { return reco::Photon::chargedHadronIso(); }
float neutralHadronIso() const { return reco::Photon::neutralHadronIso(); }
float photonIso() const { return reco::Photon::photonIso(); }

Is this correct?

Thank you

[monttj]

There is puChargedHadronIso() as well.
If we should use RECO value than PAT, it would be a quick and temporary solution.

I was thinking that it returns the reco value in a configurable way like it is done in RECO.
One example is that Muon RECO and PAT objects use the same configuration file in this location.
https://github.com/cms-sw/cmssw/tree/CMSSW_7_5_X/RecoMuon/MuonIsolation/python

But for photons, it seems that there are several ways to get the isolation value.
I thought this configuration which is implemented in PAT
https://github.com/cms-sw/cmssw/blob/CMSSW_7_5_X/RecoParticleFlow/PFProducer/python/photonPFIsolationValues_cff.py
was the POG recommendation.

If it is not the case (if RECO objects are the POG recommendation),
the best would be we need to implement the RECO configuration in PAT so that we can change the value later.

Taejeong

[gzevi]

puChargedHadronIso() is not an issue, since it does not have the same name as the RECO quantity, so it does not hinder analysis. It's just an extra (configurable) variable in the ntuple.

There are 2 separate issues here:

1. PAT isolation variables are outdated. This is true in general, for photons and electrons and muons.
2. Three of the PAT isolation variables for photons are named identically to the RECO quantities.

Issue (1) just means we carry some not-useful information around. It's not the end of the world. If you would like to update all PAT isolation variables to be correct, in a configurable way, that would be great, but not urgent.

Issue (2) is urgent. The not-useful information is preventing users to access the useful one. This is why my original proposal was to rename it.

It is an issue of code maintenance: if there is no one responsible for these variables, shouldn't we at least make sure they do not interfere with analysis? If there is someone responsible for keeping them up to date, then they should proceed on the route you described: study the two isolation codes (Reco and Pat) and try to obtain the identical Reco value from the Pat code.

What do you think?

[lgray]

Hi Gio, Taejeong,

Considering 1) I personally think it would be good to start off the run
with some known-to-work stock variables (like the standard pf isolation) as
what is in pat. During run one the isolation recipes evolved with time as
people came to understand their data better, but that require having some
basic isolation in place to start with so people could mess with the
objects. Back then it was detector based isolation, now we have pf
isolation to start from. I would favor starting from something robust and
simple instead of something that was cutting edge at one point in time (and
since then things have advanced).

2. I favor clarity in this case.

Basically we should take the quickest route to getting something usable to
the analysis users.

-L

On Sat, Mar 21, 2015 at 6:18 AM, gzevi notifications@github.com wrote:

puChargedHadronIso() is not an issue, since it does not have the same name
as the RECO quantity, so it does not hinder analysis. It's just an extra
(configurable) variable in the ntuple.

There are 2 separate issues here:

1. PAT isolation variables are outdated. This is true in general, for
   photons and electrons and muons.
2. Three of the PAT isolation variables for photons are named identically
   to the RECO quantities.

Issue (1) just means we carry some not-useful information around. It's not
the end of the world. If you would like to update all PAT isolation
variables to be correct, in a configurable way, that would be great, but
not urgent.

Issue (2) is urgent. The not-useful information is preventing users to
access the useful one. This is why my original proposal was to rename it.

It is an issue of code maintenance: if there is no one responsible for
these variables, shouldn't we at least make sure they do not interfere with
analysis? If there is someone responsible for keeping them up to date, then
they should proceed on the route you described: study the two isolation
codes (Reco and Pat) and try to obtain the identical Reco value from the
Pat code.

What do you think?

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

[monttj]

@gzevi
We can do the quick solution as you suggested for now.
Then already merged #7764 needs to be revised.
We will come back to this issue in PAT with more complete idea.

[gzevi]

I have made two new pull requests implementing this suggestion:
#8493 based on 75X (undoing #7764)
#8494 backports this to 74X

[ktf]

Closing this as it does not merge anymore and alternatives have been provided.