Codes to run hadronic tau JRA and TF.

[Calpas]

No description provided.

[aperloff]

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

1. 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?
2. Can you remind me how your crystal ball fit is different than the standard crystal ball function?
3. 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.
4. 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.
5. What do function.py an variable.py do?
6. Why the change in the BuildFile.xml?
7. 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]

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

[Calpas]

I changed it to make the src/ compiled.

[aperloff]

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]

I've left comments and questions in the code.

[Calpas]

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.

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

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

3. 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?

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

5. What do function.py an variable.py do?

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

6. Why the change in the BuildFile.xml?

To make the src compiled.

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

Move the TMath header to the crystalBall header.

[Calpas]

Why? I'm using math functions here.

[aperloff]

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.

[Calpas]

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