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

vhbb heppy: added factorized JEC #78

Merged
merged 6 commits into from
Dec 13, 2016

Conversation

jpata
Copy link

@jpata jpata commented Nov 28, 2016

From VHbb code (issue vhbb#566), add possibility to do factorized JEC uncertainties.

@jpata
Copy link
Author

jpata commented Nov 28, 2016

I appreciate if you can take a look and give your opinion @bianchini @arizzi

@jpata
Copy link
Author

jpata commented Dec 7, 2016

any comments? also cc @cbernet

@bianchini
Copy link

From what I can see it looks fine. Maybe @cbernet and @arizzi can comment better.

Copy link
Owner

@cbernet cbernet left a comment

Choose a reason for hiding this comment

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

Thanks Joosep!

As far as I can see from the code this looks ok.

Did you validate that everything is working as expected on the physics side?

I have only a few comments related to python code formatting and documentation.

Cheers, Colin

@@ -6,7 +6,7 @@
class JetReCalibrator:
def __init__(self,globalTag,jetFlavour,doResidualJECs,jecPath,upToLevel=3,
calculateSeparateCorrections=False,
calculateType1METCorrection=False, type1METParams={'jetPtThreshold':15., 'skipEMfractionThreshold':0.9, 'skipMuons':True} ):
calculateType1METCorrection=False, type1METParams={'jetPtThreshold':15., 'skipEMfractionThreshold':0.9, 'skipMuons':True}, factorizedJetCorrections=["Total"] ):
Copy link
Owner

Choose a reason for hiding this comment

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

could you please break the line that is too long, and also add to the constructor docstring the possible values for factorizedJetCorrections?

@@ -58,7 +68,7 @@ def __init__(self,globalTag,jetFlavour,doResidualJECs,jecPath,upToLevel=3,
for i in [self.L1JetPar,self.L2JetPar,self.L3JetPar,self.ResJetPar]: self.vParL3Res.push_back(i)
self.separateJetCorrectors["L1L2L3Res"] = ROOT.FactorizedJetCorrector(self.vParL3Res)

def getCorrection(self,jet,rho,delta=0,corrector=None):
def getCorrection(self,jet,rho,delta=0,corrector=None,uncertainty="Total"):
Copy link
Owner

Choose a reason for hiding this comment

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

please add a docstring

self.JetUncertainty.setJetEta(jet.eta())
self.JetUncertainty.setJetPt(corr * jet.pt() * jet.rawFactor())
JetUncertainty = self.factorizedUncertainties[uncertainty]
if not JetUncertainty: raise RuntimeError("Jet energy scale uncertainty shifts requested, but not available")
Copy link
Owner

Choose a reason for hiding this comment

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

please break line:

if not JetUncertainty: 
    blah

@arizzi
Copy link

arizzi commented Dec 7, 2016

apologies, I missed this thread. Do I understand correctly that the default list being ["Total"] the default behaviour is to not compute factorized JECs ? if so, then it is ok for me

@jpata
Copy link
Author

jpata commented Dec 7, 2016

Yes, ["Total"] is just the unfactorized JEC with no additional overhead.

@jpata
Copy link
Author

jpata commented Dec 7, 2016

@cbernet thanks, I'll implement your suggestions. The physics were tested and reported at a CMS meeting with no big complaints. For docstrings, I don't suppose you have a style suggestion? I'm using what the Sublime Text pluging AutoDocstring has by default (google [1]).

[1] https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

@cbernet
Copy link
Owner

cbernet commented Dec 7, 2016 via email

@jpata
Copy link
Author

jpata commented Dec 8, 2016

@cbernet I've added docstrings and improved formatting. As a side note I also tried building this repo in the CERN gitlab CI, it works.

@arizzi
Copy link

arizzi commented Dec 13, 2016

any news on this? can you merge? will you (@cbernet) port to CMSSW_8_0_24 or should we do it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants