Run2-alca99 Modify all macros used to calibrate Hcal using IsoTracks #20608

bsunanda · 2017-09-20T14:46:09Z

No description provided.

cmsbuild · 2017-09-20T14:46:28Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-09-20T14:50:39Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20608/856

cmsbuild · 2017-09-20T14:50:59Z

A new Pull Request was created by @bsunanda for master.

It involves the following packages:

Calibration/HcalCalibAlgos

@ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @lpernie can you please review it and eventually sign? Thanks.
@ghellwig, @mmusich, @tocheng this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

ghellwig

no major concerns, just address my minor comments

ghellwig · 2017-09-20T15:00:07Z

Calibration/HcalCalibAlgos/macros/CalibCorr.C

+  unsigned int correctDetId(unsigned int detId);
+
+  static const unsigned int nmax_=10;
+  bool                      debug_;


I suggest to make this const as this is known already at construction time.

ghellwig · 2017-09-20T15:08:58Z

Calibration/HcalCalibAlgos/macros/CalibCorr.C

+  unsigned int getDetId(int subdet, int ieta, int iphi, int depth);
+  unsigned int correctDetId(unsigned int detId);
+
+  static const unsigned int nmax_=10;


please make this constexpr (although it likely doesn't matter much in a ROOT macro)

Did you try constexpr in ROOT macro with cint - it does not work for 5.45

It does not work for 5.26

ok, if the macro has to cover several root versions, it's fine, but why do you use such an old version?
the default one is ROOT 6 in 94X

ghellwig · 2017-09-20T15:10:52Z

Calibration/HcalCalibAlgos/macros/CalibCorr.C

+
+void CalibCorr::readCorr(const std::string& infile) {
+
+  std::ifstream fInput(infile.c_str());


please drop .c_str() as there is also a constructor taking directly the std::string

Did not work

It has to work for ROOT 5.26 as well

same question as above: why do you need it for ROOT 5.26?

ghellwig · 2017-09-20T15:13:39Z

Calibration/HcalCalibAlgos/macros/CalibCorr.C

+  unsigned int id_ = ((4<<28)|((subdet&0x7)<<25));
+  id_ |= ((0x1000000) | ((depth&0xF)<<20) |
+	  ((ieta>0)?(0x80000|(ieta<<10)):((-ieta)<<10)) |
+	  (iphi&0x3FF));


are these magic numbers documented somewhere?

ghellwig · 2017-09-20T15:13:55Z

Calibration/HcalCalibAlgos/macros/CalibCorr.C

+    ieta   = ((detId >> 10) & 0x1FF);
+    zside  = (detId&0x80000)?(1):(-1);
+    depth  = ((detId >> 20) & 0xF);
+    iphi   = (detId & 0x3FF);


ghellwig · 2017-09-20T15:27:15Z

Calibration/HcalCalibAlgos/macros/CalibTree.C

+    double fact0, fact1, fact2;
+  };
+
+  CalibTree(const char *dupFileName, std::string rcorFileName, bool flag,


please use const std::string&

ghellwig · 2017-09-20T15:27:54Z

Calibration/HcalCalibAlgos/macros/CalibTree.C

+
+void Run(const char *inFileName, const char *dirName, const char *treeName, 
+	 const char *outFileName, const char *corrFileName,
+	 const char *dupFileName, const std::string rcorFileName, 


please use const std::string&

ghellwig · 2017-09-20T15:28:05Z

Calibration/HcalCalibAlgos/macros/CalibTree.C

+CalibTree::~CalibTree() {
+  if (cFactor_) delete cFactor_;
+  if (!fChain) return;
+  delete fChain->GetCurrentFile();


ghellwig · 2017-09-20T15:29:34Z

Calibration/HcalCalibAlgos/macros/CalibTree.C

+			 << std::endl;
+  }
+
+  for (std::map<unsigned int,TH1D*>::iterator itr = histos.begin();


this could be a const_iterator, right?
or use auto together with cbegin()
applies also to iterators below

ghellwig · 2017-09-20T15:30:30Z

Calibration/HcalCalibAlgos/macros/CalibTree.C

+  if (truncateFlag_) {
+    if ((subdet == 1) && (ieta > 14)) depth  = 1;
+  }
+  id = (subdet<<25) | (0x1000000) | ((depth&0xF)<<20) | ((zside>0)?(0x80000|(ieta<<10)):(ieta<<10));


magic numbers

davidlange6 · 2017-09-20T16:42:49Z

please test

cmsbuild · 2017-09-20T16:46:09Z

The tests are being triggered in jenkins.

cmsbuild · 2017-09-20T18:32:31Z

The code-checks are being triggered in jenkins.

cmsbuild · 2017-09-20T18:36:19Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20608/858

cmsbuild · 2017-09-20T18:36:36Z

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

davidlange6 · 2017-09-20T18:42:50Z

please test

cmsbuild · 2017-09-20T18:44:03Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23128/console Started: 2017/09/20 20:44

cmsbuild · 2017-10-03T20:09:10Z

The code-checks are being triggered in jenkins.

bsunanda · 2017-10-03T20:09:10Z

@cmsbuild Please test

cmsbuild · 2017-10-03T20:11:46Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/PR-20608/1123

cmsbuild · 2017-10-03T20:12:05Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/23416/console Started: 2017/10/03 22:12

cmsbuild · 2017-10-03T20:12:06Z

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

cmsbuild · 2017-10-03T22:08:23Z

+1
Tested at: 5f395b1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20608/23416/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
66f2f62
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20608/23416/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20608/23416/git-merge-result

cmsbuild · 2017-10-03T22:08:25Z

Comparison job queued.

cmsbuild · 2017-10-03T23:33:50Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 26
DQMHistoTests: Total histograms compared: 2761357
DQMHistoTests: Total failures: 241
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2760929
DQMHistoTests: Total skipped: 187
DQMHistoTests: Total Missing objects: 0
Checked 107 log files, 10 edm output root files, 26 DQM output files

ghellwig · 2017-10-04T07:31:54Z

+1

cmsbuild · 2017-10-04T07:33:13Z

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 will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2)

davidlange6 · 2017-10-04T08:24:23Z

+1

cmsbuild added this to the CMSSW_9_4_X milestone Sep 20, 2017

cmsbuild added alca-pending code-checks-pending comparison-pending orp-pending pending-signatures tests-pending labels Sep 20, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 20, 2017

ghellwig reviewed Sep 20, 2017

View reviewed changes

cmsbuild added tests-started code-checks-pending tests-pending and removed tests-pending code-checks-approved tests-started labels Sep 20, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 20, 2017

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

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-started labels Oct 3, 2017

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 3, 2017

cmsbuild added tests-started and removed tests-pending labels Oct 3, 2017

cmsbuild added tests-approved and removed tests-started labels Oct 3, 2017

cmsbuild added comparison-available and removed comparison-pending labels Oct 3, 2017

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Oct 4, 2017

cmsbuild added orp-approved and removed orp-pending labels Oct 4, 2017

cmsbuild merged commit 024141f into cms-sw:master Oct 4, 2017


		void CalibCorr::readCorr(const std::string& infile) {

		std::ifstream fInput(infile.c_str());

Run2-alca99 Modify all macros used to calibrate Hcal using IsoTracks #20608

Run2-alca99 Modify all macros used to calibrate Hcal using IsoTracks #20608

Conversation

bsunanda commented Sep 20, 2017

cmsbuild commented Sep 20, 2017

cmsbuild commented Sep 20, 2017

cmsbuild commented Sep 20, 2017

ghellwig left a 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

davidlange6 commented Sep 20, 2017

cmsbuild commented Sep 20, 2017

cmsbuild commented Sep 20, 2017

cmsbuild commented Sep 20, 2017

cmsbuild commented Sep 20, 2017

davidlange6 commented Sep 20, 2017

cmsbuild commented Sep 20, 2017 • edited

cmsbuild commented Oct 3, 2017

bsunanda commented Oct 3, 2017

cmsbuild commented Oct 3, 2017

cmsbuild commented Oct 3, 2017 • edited

cmsbuild commented Oct 3, 2017

cmsbuild commented Oct 3, 2017

cmsbuild commented Oct 3, 2017

cmsbuild commented Oct 3, 2017

ghellwig commented Oct 4, 2017

cmsbuild commented Oct 4, 2017

davidlange6 commented Oct 4, 2017

cmsbuild commented Sep 20, 2017 •

edited

cmsbuild commented Oct 3, 2017 •

edited