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

[aperloff]

What is the difference between crystalBall and crystalBall_1?

[Calpas]

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

[aperloff]

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]

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]

Is this comment necessary or can it be removed?

[Calpas]

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