Codes to run hadronic tau JRA and TF. by Calpas · Pull Request #10 · cms-jet/JetMETAnalysis

Calpas · 2016-04-21T12:27:00Z

No description provided.

aperloff · 2016-04-21T14:56:38Z

I've reviewed much of the code and I do have line-by-line comments. However, first I want to make a couple of general comments and requests.

Can you please describe to me why there are three new classes dealing with crystal ball fits (i.e. fitCrystalBall, crystalBall, and crystalBallRes). What is the purpose of each? Why can these not be condensed?
Can you remind me how your crystal ball fit is different than the standard crystal ball function?
There are a couple of places in the code where you have outright changed the behavior that is integral to the pre-existing code (i.e. L2Creator.cc lines 82 and 879). This is not good and we should work to make both workflows possible.
I am getting a lot of whitespace changes. Are you using tabs or spaces? If spaces, how many? I can turn off these warning in my github review, but please be aware of this in the future.
What do function.py an variable.py do?
Why the change in the BuildFile.xml?
I'm not opposed to you adding entirely new pieces of code to the package. However, I am opposed to code duplication (i.e. two pieces of code that do essentially the same thing). You must also know that it will become your responsibility to maintain. The rest of the users who are not familiar with your code must be able to compile.

aperloff · 2016-04-21T15:20:28Z

JetAnalyzers/BuildFile.xml

-<flags   EDM_PLUGIN="1"/>
 <!--
+<flags   EDM_PLUGIN="1"/>
+-->


Why the change from EDM_PLUGIN to lib? Will this effect the EDAnalyzer in this package?

I changed it to make the src/ compiled.

Okay, I have no problem with this. As long as the code compiles and makes a .so in the CMSSW_BASE/lib folder I'm happy.

aperloff · 2016-04-21T15:38:57Z

I've left comments and questions in the code.

Calpas · 2016-04-22T11:16:30Z

On 4/21/16 4:56 PM, Alexx Perloff wrote:

@Calpas https://github.com/Calpas

I've reviewed much of the code and I do have line-by-line comments.
However, first I want to make a couple of general comments and requests.

Can you please describe to me why there are three new classes
dealing with crystal ball fits (i.e. fitCrystalBall, crystalBall, and
crystalBallRes). What is the purpose of each? Why can these not be
condensed?

I did not wanted to add to much changes into your existing code so I
prefer write my new fonctions and just
call them from your's. Also, these function are used in several
functions, so we definitely vote to make it standalone.

For clarity reason I used 3 separate functions:
fitCrystalBall is a void function that calls crystalBall which performs
the fit and crystalBallRes print/draw the results.

Can you remind me how your crystal ball fit is different than the
standard crystal ball function?

It uses several functions (polynome, exponential and up to 3 sum gaus)
depending on the tauRSP range.

There are a couple of places in the code where you have outright
changed the behavior that is integral to the pre-existing code (i.e.
L2Creator.cc lines 82 and 879). This is not good and we should work to
make both workflows possible.

I could add a flag for only tau?

I am getting a lot of whitespace changes. Are you using tabs or
spaces? If spaces, how many? I can turn off these warning in my github
review, but please be aware of this in the future.

The code has a weird structure that I did not like at all.
I used a more conventional structure. For me a tab =2 spaces.

What do function.py an variable.py do?

They contain variable and function. I separate for clarity reason.

Why the change in the BuildFile.xml?

To make the src compiled.

I'm not opposed to you adding entirely new pieces of code to the
package. However, I am opposed to code duplication (i.e. two pieces of
code that do essentially the same thing). You must also know that it
will become your responsibility to maintain. The rest of the users who
are not familiar with your code must be able to compile.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10 (comment)

Cheers,
Betty

aperloff · 2016-04-22T14:07:02Z

JetUtilities/src/crystalBall.cc

+#include "JetMETAnalysis/JetUtilities/interface/crystalBall.h"
+
+#include "TMath.h"
+


Move the TMath header to the crystalBall header.

Why? I'm using math functions here.

As a style I like to keep all of the headers in the header file of the class that uses them. It also prevents multiple linking issues.

I prefer keep it where I'm using it to do not go back and forth to check
o set the header.
More over this is my code, so I prefer keep my style.

Le 27/04/2016 03:24, Alexx Perloff a écrit :

In JetUtilities/src/crystalBall.cc
#10 (comment):

@@ -0,0 +1,227 @@
+#include "JetMETAnalysis/JetUtilities/interface/crystalBall.h"
+
+#include "TMath.h"
+

As a style I like to keep all of the headers in the header file of the
class that uses them. It also prevents multiple linking issues.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/cms-jet/JetMETAnalysis/pull/10/files/abbd2ce4a9b0377684ddf51b4382b801e0a66c05#r61187055

aperloff · 2016-04-22T14:24:45Z

JetUtilities/interface/crystalBall.h

+#define CRYSTALBALL_HH
+
+double crystalBall(double *xx, double *pp); // global fit using the function above
+double crystalBall_1(double *xx, double *pp); // global fit using the function above


What is the difference between crystalBall and crystalBall_1?

crystalBall uses a polynome of order 7 (for tauES) and crystalBall_1 a polynome of order 1 (for tau transfer function).

aperloff · 2016-04-22T14:25:15Z

Can the functions in crystalBallRes be moved to crystalBall? Why use two headers/namespaces when one would do? I'm just asking here as, again, I don't know all of the intricacies of these codes.

Also, since this is not a standard crystal ball function can we change the name to crystalBallTau or something like that. It's misleading to think that anyone could come along and use your crystal ball code. It's tailor made to your needs, right? As far as I can tell it's not generic. For instance, your crystalBall function can't simply replace the double-sided crystal ball function defined in jet_response_fitter_x.cc, right? Am I misreading things?

Okay, I agree that some of the legacy code had a weird structure that should have been changed. That being said, can we use tab=4 spaces? It's just a preference thing for me and means that any changes we both make won't trigger this white space issue. Two spaces just seems a little smushed to me.

This is good. I think we are converging.

Calpas · 2016-04-23T14:41:28Z

Le 22/04/2016 17:25, Alexx Perloff a écrit :

Can the functions in crystalBallRes be moved to crystalBall? Why use
two headers/namespaces when one would do? I'm just asking here as,
again, I don't know all of the intricacies of these codes.

No there are different. I prefer separate for now.

Also, since this is not a standard crystal ball function can we change
the name to crystalBallTau or something like that. It's misleading to
think that anyone could come along and use your crystal ball code.
It's tailor made to your needs, right? As far as I can tell it's not
generic. For instance, your crystalBall function can't simply replace
the double-sided crystal ball function defined in
jet_response_fitter_x.cc, right? Am I misreading things?

Yes we could change the name.

Okay, I agree that some of the legacy code had a weird structure that
should have been changed. That being said, can we use tab=4 spaces?
It's just a preference thing for me and means that any changes we both
make won't trigger this white space issue. Two spaces just seems a
little smushed to me.

For me I prefer the 2 spaces because when code line is too long, we
can't see all the line code on a single line right?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10 (comment)

aperloff · 2016-04-26T23:18:52Z

JetAnalyzers/bin/jet_response_fitter_x.cc

+	continue; 
+      }

+      //if(verbose>0) cout << "Attempting to fit " << histname << " ... " << endl;


Is this comment necessary or can it be removed?

Can be removed.

Le 27/04/2016 02:18, Alexx Perloff a écrit :

In JetAnalyzers/bin/jet_response_fitter_x.cc
#10 (comment):

//if(verbose>0) cout << "Attempting to fit " << histname << " ... " << endl;

Is this comment necessary or can it be removed?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/cms-jet/JetMETAnalysis/pull/10/files/6c09ed51401443d614ab03918245241673c2ddb0#r61180964

betty calpas added 7 commits April 21, 2016 13:07

Code to run JRAtau and SVfitMEM.

77c28e9

Codes for CrystalBall fit.

6985437

Add CrystalBall function for hadronic tau.

68ab422

Modified BuildFile.xml to compiled CrystalBall fucntion.

c52ad31

Codes for hadronic tau calibration and svfitMEM parameters fit.

1f7900a

mv files to JetUtilities pkg.

61801b4

Files for CrystalBall function.

0f73e5f

aperloff reviewed Apr 21, 2016
View reviewed changes

Update based on Alexx comments.

5d76f29

betty calpas added 2 commits April 22, 2016 16:11

Update for Alexx comments.

abbd2ce

Update for Alexx comments.

30685e4

aperloff reviewed Apr 22, 2016
View reviewed changes

betty calpas added 2 commits April 23, 2016 16:51

Update Alexx comments.

6b424f1

Update Alexx comments.

ec23f7e

Update Alexx comments.

6c09ed5

aperloff reviewed Apr 26, 2016
View reviewed changes

		#include "JetMETAnalysis/JetUtilities/interface/crystalBall.h"

		#include "TMath.h"

Comments

Conversation

Calpas commented Apr 21, 2016

Uh oh!

aperloff commented Apr 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aperloff commented Apr 21, 2016

Uh oh!

Calpas commented Apr 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aperloff commented Apr 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Calpas commented Apr 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aperloff commented Apr 22, 2016 •

edited

Loading