bsunanda:Run2-alca77 Try Muon analysis in HB/HE using SIM information #18125

-#ifdef DebugLog
-  edm::LogInfo("HcalHBHEMuon") << "Run " << RunNumber << " Event " << EventNumber
-			       << " Lumi " << LumiNumber << " BX " << BXNumber;
+  int depthHE = (maxDepth_ <= 6) ? 3 : 4;


@bsunanda could you add a comment on these magic numbers, please
Or better use constants with a meaningful name.

ghellwig · 2017-04-10T13:27:36Z

Calibration/HcalCalibAlgos/plugins/HcalHBHEMuonAnalyzer.cc

-      unsigned int isHot = 0;
-      for (int i=0; i<7; ++i) eHcalDepth[i]=eHcalDepthHot[i]=-10000;
+      double eEcal(0), eHcal(0), activeLengthTot(0), activeLengthHotTot(0);
+      double eHcalDepth[7], eHcalDepthHot[7], activeL[7], activeHotL[7];


@bsunanda please use a named constant to clarify where the magic number 7 comes from or just use the size of the std::array proposed above

ghellwig · 2017-04-10T13:27:57Z

Calibration/HcalCalibAlgos/plugins/HcalHBHEMuonAnalyzer.cc

+      double eHcalDepth[7], eHcalDepthHot[7], activeL[7], activeHotL[7];
+      unsigned int isHot(0);
+      bool         tmpmatch(false);
+      for (int i=0; i<7; ++i) 


ghellwig · 2017-04-10T13:31:32Z

Calibration/HcalCalibAlgos/plugins/HcalHBHEMuonAnalyzer.cc

+  std::vector<double>       hcalDepth4EnergyHot_, hcalDepth4ActiveLengthHot_;
+  std::vector<double>       hcalDepth5EnergyHot_, hcalDepth5ActiveLengthHot_;
+  std::vector<double>       hcalDepth6EnergyHot_, hcalDepth6ActiveLengthHot_;
+  std::vector<double>       hcalDepth7EnergyHot_, hcalDepth7ActiveLengthHot_;


@bsunanda I think, it would be more readible if you replace the 7 vectors with an std::array<std::vector<double>, 7>.
This would then need to be propagated to the rest of the code, of course.

There have been some downstream codes made by a few users which utilizes the current construct. It is better if we leave this for the time being

I do not see how downstream code would need to know how the private data members are organized...
The branches written to the tree could still have the same name and would still be vectors.
You just replace seven lines of code with a loop containing one line of code (+1 line for the head of the loop).
Having this multiple times reduces a lot the complexity and makes reading the code a lot easier.

ghellwig · 2017-04-10T13:35:35Z

Calibration/IsolatedParticles/plugins/HcalHBHEMuonSimAnalyzer.cc

+
+// user include files
+#include "FWCore/Framework/interface/Frameworkfwd.h"
+#include "FWCore/Framework/interface/EDAnalyzer.h"


this should be

#include "FWCore/Framework/interface/one/EDAnalyzer.h"

ghellwig · 2017-04-10T13:42:21Z

Calibration/IsolatedParticles/python/hcalHBHEMuonSim_cfi.py

+                                         TimeMaxCutECAL = cms.untracked.double(500.0),
+                                         TimeMinCutHCAL = cms.untracked.double(-500.0),
+                                         TimeMaxCutHCAL = cms.untracked.double(500.0),
+                                         )


this would just go into fillDescriptions above

Why is this file still here?
If you rename the description in fillDescriptions from "hcalHBHEMuonSim" to "HcalHBHEMuonSimAnalyzer" you can drop this file because it will be automatically created.

ghellwig · 2017-04-10T13:46:03Z

Calibration/IsolatedParticles/interface/FindCaloHit.h

@@ -34,6 +34,8 @@ namespace spr {
  template <typename T>
  std::vector<typename T::const_iterator> findHit(edm::Handle<T>& hits, DetId thisDet, bool debug=false);

+  std::vector<std::vector<PCaloHit>::const_iterator> findHit(std::vector<PCaloHit>& hits, DetId thisDet, bool debug=false);


Is this specialization really needed? From a quick look, it seems like you just write down what is done also in the general case.

I believe so. Could not use the template version

Why couldn't you use the template version?
I simply don't like to add specialization for every type one uses. This is almost the same as not using templates at all.

Can one use edm::Handle and std::vector in the same version?

ghellwig · 2017-04-10T13:47:49Z

Calibration/IsolatedParticles/interface/eHCALMatrix.h

@@ -39,6 +39,8 @@ namespace spr{
  template< typename T>
  double eHCALmatrix(const HcalTopology* topology, const DetId& det, edm::Handle<T>& hits, int ieta, int iphi, bool includeHO=false, bool algoNew=true, double hbThr=-100, double heThr=-100, double hfThr=-100, double hoThr=-100, double tMin=-500, double tMax=500, bool useRaw=false, bool debug=false);

+  double eHCALmatrix(const HcalTopology* topology, const DetId& det, std::vector<PCaloHit>& hits, int ieta, int iphi, bool includeHO=false, double hbThr=-100, double heThr=-100, double hfThr=-100, double hoThr=-100, double tMin=-500, double tMax=500, bool debug=false);


is this specialization really needed? or should the general template be updated to take a reference to T instead of a handle to T?

ghellwig · 2017-04-10T13:48:01Z

Calibration/IsolatedParticles/interface/eHCALMatrix.h

@@ -51,6 +53,8 @@ namespace spr{
  template <typename T>
  double eHCALmatrix(const CaloGeometry* geo, const HcalTopology* topology, const DetId& det0, edm::Handle<T>& hits, int ieta, int iphi, HcalDetId& hotCell, bool includeHO=false, bool useRaw=false, bool debug=false);

+  double eHCALmatrix(const CaloGeometry* geo, const HcalTopology* topology, const DetId& det0, std::vector<PCaloHit>& hits, int ieta, int iphi, HcalDetId& hotCell, bool includeHO=false, bool debug=false);


ghellwig · 2017-04-10T13:48:10Z

Calibration/IsolatedParticles/interface/eHCALMatrix.h

@@ -69,12 +73,16 @@ namespace spr{
  template <typename T>
  void energyHCALCell(HcalDetId detId, edm::Handle<T>& hits, std::vector<std::pair<double,int> >& energyCell, int maxDepth=1, double hbThr=-100, double heThr=-100, double hfThr=-100, double hoThr=-100, double tMin=-500, double tMax=500, bool useRaw=false, bool debug=false);

+  void energyHCALCell(HcalDetId detId, std::vector<PCaloHit>& hits, std::vector<std::pair<double,int> >& energyCell, int maxDepth=1, double hbThr=-100, double heThr=-100, double hfThr=-100, double hoThr=-100, double tMin=-500, double tMax=500, bool debug=false);


bsunanda · 2017-04-10T21:22:14Z

@cmsbuild Please test

cmsbuild · 2017-04-10T21:22:37Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19076/console Started: 2017/04/10 23:26

bsunanda · 2017-04-20T08:10:35Z

@cmsbuild Please test

cmsbuild · 2017-04-20T08:10:36Z

Pull request #18125 was updated. @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @mmusich, @davidlange6 can you please check and sign again.

cmsbuild · 2017-04-20T08:10:49Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19282/console Started: 2017/04/20 10:11

bsunanda · 2017-04-20T08:12:18Z

@ghellwig David asked me to protect the std::cout withing compile time switch which I did. Could you please check and approve this at the earliest. Now this PR is stuck for > 3 weeks

ghellwig · 2017-04-20T08:47:44Z

@bsunanda I believe what @davidlange6 meant was to replace debug with the macro, not to decorate it with the macro

cmsbuild · 2017-04-20T09:18:42Z

+1
Tested at: 47f52c8
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18125/19282/summary.html

cmsbuild · 2017-04-20T09:18:45Z

Comparison job queued.

cmsbuild · 2017-04-20T10:49:41Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18125/19282/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 1669 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 1826171
DQMHistoTests: Total failures: 6239
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1819759
DQMHistoTests: Total skipped: 173
DQMHistoTests: Total Missing objects: 0
Checked 94 log files, 14 edm output root files, 23 DQM output files

ghellwig · 2017-04-26T09:05:24Z

+1
@bsunanda approving for alca, but I let @davidlange6 decide on how he judges your handling of his comment

cmsbuild · 2017-04-26T09:05:41Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @Muzaffar, @davidlange6, @smuzaffar

bsunanda · 2017-04-27T13:58:53Z

@davidlange6 Could you please integrate this PR. Need this to be used for plan 1 calibration

davidlange6 · 2017-05-01T15:54:05Z

I agree that the ifdef followed by an if debug is duplicating things, but doesn't affect anything unless the debuging is compiled in

Try Muon analysis in HB/HE using SIM information

92c9078

cmsbuild added this to the Next CMSSW_9_1_X milestone Mar 29, 2017

cmsbuild added alca-pending comparison-pending orp-pending pending-signatures tests-pending labels Mar 29, 2017

cmsbuild added tests-started and removed tests-pending labels Mar 29, 2017

cmsbuild added tests-approved and removed tests-started labels Mar 29, 2017

cmsbuild added comparison-available and removed comparison-pending labels Mar 29, 2017

ghellwig suggested changes Apr 10, 2017

View reviewed changes

Sunanda added 2 commits April 10, 2017 17:20

Merged Run2-alca77 from repository bsunanda

76dee75

Take care of most of the comments

122d9fc

bsunanda force-pushed the Run2-alca77 branch from 92c9078 to 9cd2b60 Compare April 10, 2017 21:21

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels Apr 10, 2017

cmsbuild added alca-pending comparison-pending pending-signatures tests-pending and removed comparison-available fully-signed tests-approved labels Apr 20, 2017

cmsbuild added tests-started and removed tests-pending labels Apr 20, 2017

cmsbuild added tests-approved and removed tests-started labels Apr 20, 2017

cmsbuild added comparison-available and removed comparison-pending labels Apr 20, 2017

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Apr 26, 2017

davidlange6 merged commit 46f7453 into cms-sw:master May 1, 2017

bsunanda:Run2-alca77 Try Muon analysis in HB/HE using SIM information #18125

bsunanda:Run2-alca77 Try Muon analysis in HB/HE using SIM information #18125

Conversation

bsunanda commented Mar 29, 2017 • edited

bsunanda commented Mar 29, 2017

cmsbuild commented Mar 29, 2017 • edited

cmsbuild commented Mar 29, 2017

cmsbuild commented Mar 29, 2017

cmsbuild commented Mar 29, 2017

cmsbuild commented Mar 29, 2017

bsunanda commented Apr 5, 2017

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

ghellwig Apr 10, 2017 • edited

Choose a reason for hiding this comment

bsunanda commented Apr 10, 2017

cmsbuild commented Apr 10, 2017 • edited

bsunanda commented Apr 20, 2017

cmsbuild commented Apr 20, 2017

cmsbuild commented Apr 20, 2017 • edited

bsunanda commented Apr 20, 2017

ghellwig commented Apr 20, 2017

cmsbuild commented Apr 20, 2017

cmsbuild commented Apr 20, 2017

cmsbuild commented Apr 20, 2017

ghellwig commented Apr 26, 2017

cmsbuild commented Apr 26, 2017

bsunanda commented Apr 27, 2017

davidlange6 commented May 1, 2017

bsunanda commented Mar 29, 2017 •

edited

cmsbuild commented Mar 29, 2017 •

edited

ghellwig Apr 10, 2017 •

edited

cmsbuild commented Apr 10, 2017 •

edited

cmsbuild commented Apr 20, 2017 •

edited