Izisopou patch new fitting approaches #30

izisopou · 2020-10-28T17:21:49Z

Changes that mainly focus on the implementation of:

A fitting approach for the Offset (L1) Corrections (semi-simple, simple, complex parametrizations) & sanity checks for the fits.
A fitting approach for the Relative and Absolute (L2L3) Corrections (standard+gaussian function) & sanity checks for the fits.

Veto regions for UL 2017 and UL 2018 added and commented out.

Veto regions for UL2017 and UL2018 added and commented out.

This script is necessary for matching jets between the NoPU (or EpsilonPU) and PU files.

Implemented fitting approach with standard+gaussian function, along with a pt-clipping technique and sanity checks for the fits in all eta bins.

Implemented 2D fitting approach with semi-simple parametrization (also complex and simple parametrizations, which were previously used are commented out), along with sanity checks for the fits in all eta bins.

Added pt-clip for the fitting approach ---> ptclipfit (and for splines --->ptclip).

A slightly modified script of the xrootd/XrdCl/XrdClFileSystem.hh one, in order to avoid compilation errors.

This change includes the XrdClFileSystem_v2.hh script instead of the XrdClFileSystem.hh so as to avoid compilation errors.

aperloff · 2020-11-03T17:33:51Z

JetAnalyzers/bin/jet_correction_analyzer_x.cc

@@ -603,6 +604,13 @@ int main(int argc,char**argv)

         if(nrefmax>0 && JRAEvt->nref>nrefmax) JRAEvt->nref = nrefmax;
         for (unsigned char iref=0;iref<JRAEvt->nref;iref++) {
+	    /*      


Do not leave unused, commented code. If it's commented because it's only sometimes used, then there should be a switch. If it's commented because you want to reference the old code occasionally you should look at the git history.

Okay, I'll remove the commented out code in this case.

aperloff · 2020-11-03T17:35:23Z

JetAnalyzers/bin/jet_correction_analyzer_x.cc

@@ -18,7 +18,8 @@
 #include "CondFormats/JetMETObjects/interface/FactorizedJetCorrector.h"
 #include "PhysicsTools/Utilities/interface/LumiReWeighting.h"
 #if __has_include("xrootd/XrdCl/XrdClFileSystem.hh")
-#include "xrootd/XrdCl/XrdClFileSystem.hh"
+//#include "xrootd/XrdCl/XrdClFileSystem.hh"


I left a comment in #29 about using a copy of an XRootD header. A short version is you shouldn't copy headers from external dependencies. Instead you should use static_asserts and header guards to make sure the dependencies you need exist during compilation.

We were getting compilation errors when using the xrootd/XrdCl/XrdClFileSystem.hh file and since it’s read-only we created a slightly modified XrdClFileSystem_v2.hh script in oder to avoid the error. This was a quick and easy way to keep the workflow running without getting errors. I’m not sure I understood what you are suggesting. Could you explain more in detail?

Without knowing what the compilation errors were I can't comment on an alternative for fixing them. I was assuming the original xrootd/XrdCl/XrdClFileSystem.hh file would work and that you were worried about dependencies. Perhaps you can post the compilation error you were seeing (as an attachment).

Thank you! I am attaching an image of the error we were having. Basically we changed the kXR_evict to kXR_wait , as the error suggested, and the compilation of XrdClFileSystem_v2.hh works fine without issues.

I can't be certain, but it look like adding #include "xrootd/XProtocol/XProtocol.hh" above the other XRootD header will fix the problem. Give it a try and let us know.

Thank you! I tested it and it works, I don't get the compilation error anymore. I'll add this line and remove the XrdClFileSystem_v2.hh code, if that's okay with you.

aperloff · 2020-11-03T17:36:19Z

JetAnalyzers/bin/jet_response_analyzer_x.cc

@@ -1478,6 +1478,12 @@ int main(int argc,char**argv)

        if (nrefmax>0) JRAEvt->nref = std::min((int)JRAEvt->nref,nrefmax);
        for (unsigned char iref=0;iref<JRAEvt->nref;iref++) {
+	  /*      


Same comment about commented code.

Okay, I'll remove the commented out code.

aperloff · 2020-11-03T17:37:22Z

JetAnalyzers/bin/jet_synchfit_x.cc

-
-         }//if
-
+TGraph2DErrors * getGraph2D(int iEta, TProfile3D * prof,


Please don't make white space changes unless absolutely necessary.

Okay, I'll fix that.

Please double-check. The diff of jet_synchfit_x.cc is still very long and e.g. the getGraph2D still looks ~identical except for white space changes. Please make the formating such that one can better see the changes in the logic (if there are any). Thanks!

Okay, I will double-check and try to see if I can make the format better!

aperloff · 2020-11-03T17:42:43Z

JetAnalyzers/bin/jet_match_x.cc

@@ -0,0 +1,1695 @@
+// ROOT Libraries


Please add a section to https://github.com/cms-jet/JetMETAnalysis/blob/master/JetAnalyzers/bin/README.md. I'm not sure what this code is suppose to do and how it's different from jet_synchtest_x.cc. If you're renaming the file that is not reflected in this PR.

The jet_match_x.cc code is actually a modification of the jet_synchtest_x.cc one, which we do not use anymore. It implements an option to remove a phi slice (for the HEM issue studies). So it's basically a rename, as you said, and I will add that in the description of the code to avoid confusion.

But does it really provide fundamentally different functionality? I think it would be better to add a "remove phi-slice" option to jet_synchtest_x.cc, if it's essentially the same thing

I agree with you, I will remove the jet_match_x.cc code and re-upload the jet_synchtest_x.cc with the latest updates.

aperloff · 2020-11-03T17:44:27Z

JetAnalyzers/bin/jet_synchfit_x.cc

-   if(!fin->IsOpen()) {
-      cout << "ERROR jet_synchfit_xx::getInputProfiles() could not open file " 
-           <<inputFilename<< endl;
+bool getInputProfiles(TString inputFilename, TProfile3D *& prof,


No white space changes unless necessary and globally matched.

I'll fix that here as well, thank you.

aperloff · 2020-11-03T17:47:52Z

JetUtilities/src/L2Creator.cc

@@ -46,6 +47,10 @@ L2Creator::L2Creator(CommandLine& cl) {
    histMet    = cl.getValue<string>  ("histMet",       "mu_h");
    histogramMetric = HistUtil::getHistogramMetricType(histMet);

+    ptclip     = cl.getValue<float>   ("ptclip",            0.);
+    statTh     = cl.getValue<int>     ("statTh",             4);
+    ptclipfit = cl.getValue<bool> ("ptclipfit", false);


Match style

Okay, done.

aperloff · 2020-11-03T17:49:21Z

JetUtilities/src/L2Creator.cc

@@ -537,6 +573,54 @@ void L2Creator::loopOverEtaBins() {
                }
            }

+
+	    //edw ptclipfit 


What is "ewd"? Is it needed here?

It is not needed and I will remove it.

aperloff · 2020-11-03T17:50:24Z

JetUtilities/src/L2Creator.cc

@@ -804,7 +918,7 @@ void L2Creator::makeCanvas(string makeCanvasVariable) {
                spline_func->SetRange(bounds.first,bounds.second);
                spline_func->SetLineColor(igraph%nperpad+1);
                spline_func->SetLineStyle(kDotted);
-                spline_func->Draw("same");
+//              spline_func->Draw("same");


Why comment out the Draw statement? If you don't want to draw a spline this should be controlled by logic, not by commenting out code.

It was actually done by accident, thank you for pointing it out! I fixed it.

The jet_match_x.cc code is actually a modification of the jet_synchtest_x.cc one, which we do not use anymore. It implements an option to remove a phi slice (for the HEM issue studies).

kirschen · 2020-11-05T19:19:22Z

General comment @izisopou : It would be great to give a bit more context in your commit messages (not the name of a changed file name, but what the commit is supposed to do), cf. e.g. a random Google find https://chris.beams.io/posts/git-commit/ and the "CMSSW on github" pages at http://cms-sw.github.io/index.html for some good practices :-)

The main focus of the changes are: 1) an option to remove a phi slice (for the HEM issue studies) 2) the implementation of energy fractions

izisopou added 9 commits October 28, 2020 18:39

Update jet_correction_analyzer_x.cc

e92defe

Veto regions for UL 2017 and UL 2018 added and commented out.

Update jet_response_analyzer_x.cc

e98be1e

Veto regions for UL2017 and UL2018 added and commented out.

Create jet_match_x.cc

697eaa1

This script is necessary for matching jets between the NoPU (or EpsilonPU) and PU files.

Update jet_synchfit_x.cc

f80e85a

Update L2Creator.cc

a6ad4ec

Implemented fitting approach with standard+gaussian function, along with a pt-clipping technique and sanity checks for the fits in all eta bins.

Update jet_synchfit_x.cc

8706627

Implemented 2D fitting approach with semi-simple parametrization (also complex and simple parametrizations, which were previously used are commented out), along with sanity checks for the fits in all eta bins.

Update L2Creator.hh

62c6b1c

Added pt-clip for the fitting approach ---> ptclipfit (and for splines --->ptclip).

Create XrdClFileSystem_v2.hh

d7c2f59

A slightly modified script of the xrootd/XrdCl/XrdClFileSystem.hh one, in order to avoid compilation errors.

Update jet_correction_analyzer_x.cc

f024f83

This change includes the XrdClFileSystem_v2.hh script instead of the XrdClFileSystem.hh so as to avoid compilation errors.

aperloff reviewed Nov 3, 2020

View reviewed changes

izisopou added 7 commits November 5, 2020 18:27

Update jet_correction_analyzer_x.cc

8c36870

Update jet_response_analyzer_x.cc

e71171b

Update jet_synchfit_x.cc

063a375

Update L2Creator.cc

ce54c0d

Update jet_match_x.cc

a206aa4

The jet_match_x.cc code is actually a modification of the jet_synchtest_x.cc one, which we do not use anymore. It implements an option to remove a phi slice (for the HEM issue studies).

Update jet_correction_analyzer_x.cc

b8a9cd6

Delete XrdClFileSystem_v2.hh

478c8bd

izisopou added 3 commits November 9, 2020 12:26

Update jet_synchtest_x.cc

83930e5

The main focus of the changes are: 1) an option to remove a phi slice (for the HEM issue studies) 2) the implementation of energy fractions

Delete jet_match_x.cc

2809db1

Update jet_synchfit_x.cc

ff9ce76

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Izisopou patch new fitting approaches #30

Izisopou patch new fitting approaches #30

izisopou commented Oct 28, 2020 •

edited

Loading

aperloff Nov 3, 2020

izisopou Nov 5, 2020

aperloff Nov 3, 2020

izisopou Nov 5, 2020

aperloff Nov 5, 2020

izisopou Nov 5, 2020

aperloff Nov 5, 2020

izisopou Nov 5, 2020

aperloff Nov 3, 2020

izisopou Nov 5, 2020

aperloff Nov 3, 2020

izisopou Nov 5, 2020

kirschen Nov 5, 2020

izisopou Nov 9, 2020

aperloff Nov 3, 2020

izisopou Nov 5, 2020

kirschen Nov 5, 2020

izisopou Nov 9, 2020

aperloff Nov 3, 2020

izisopou Nov 5, 2020

aperloff Nov 3, 2020

izisopou Nov 5, 2020

aperloff Nov 3, 2020

izisopou Nov 5, 2020

aperloff Nov 3, 2020

izisopou Nov 5, 2020

kirschen commented Nov 5, 2020


		}//if

		TGraph2DErrors * getGraph2D(int iEta, TProfile3D * prof,

Izisopou patch new fitting approaches #30

Are you sure you want to change the base?

Izisopou patch new fitting approaches #30

Conversation

izisopou commented Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirschen commented Nov 5, 2020

izisopou commented Oct 28, 2020 •

edited

Loading