Memory reduction of the Cluster Charge Histogram used in SiStrip gain calibration #20010

dimattia · 2017-08-01T21:08:36Z

This pull requests implements the code needed to reduce the number of bins of the cluster charge histogram. The implementation requires to book a TH2S histogram with variable bin size, which was incidentally not supported by the DQMStore. The fix for this is delivered within the pull request under the DQMservice package.

The motivation of this change has been already presented here
https://indico.cern.ch/event/649344/contributions/2672267/attachments/1498323/2332518/OptimizeChHisto.pdf

cmsbuild · 2017-08-01T21:08:56Z

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

It involves the following packages:

CalibTracker/SiStripChannelGain
DQMServices/Core

@ghellwig, @vazzolini, @kmaeshima, @arunhep, @cerminar, @dmitrijus, @cmsbuild, @franzoni, @vanbesien, @lpernie can you please review it and eventually sign? Thanks.
@ghellwig, @barvic, @gbenelli, @tocheng, @jlagram, @OlivierBondu, @mmusich this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

arunhep · 2017-08-01T21:22:20Z

@cmsbuild please test

cmsbuild · 2017-08-01T21:23:03Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/21992/console Started: 2017/08/02 01:16

cmsbuild · 2017-08-02T00:53:08Z

+1
Tested at: 8bdcb43
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20010/21992/summary.html

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

cmsbuild · 2017-08-02T00:53:11Z

Comparison job queued.

boudoul · 2017-08-02T01:07:26Z

Hi @dimattia , please change also the title of this PR (we can't get that this is for strips , gain calibration ), then put the same title in the backport #20011 (adding the string 92X is the title ) - thank you .

boudoul · 2017-08-02T01:30:07Z

CalibTracker/SiStripChannelGain/src/SiStripGainsPCLWorker.cc

@@ -267,6 +267,9 @@ void SiStripGainsPCLWorker::processEvent(const TrackerTopology* topo) {
      if(Validation)     {ClusterChargeOverPath/=(*gainused)[i];}
      if(OldGainRemoving){ClusterChargeOverPath*=(*gainused)[i];}
    }
+
+    // keep processing of pixel cluster charge until here
+    if(APV->SubDet<=2) continue;


hi @dimattia , Question : can't we get rid completely of the pixel part ? there are have checks and computation for the pixel above this line like here
https://github.com/dimattia/cmssw/blob/8bdcb4372e22de2ac5112b37575b3bead83546a5/CalibTracker/SiStripChannelGain/src/SiStripGainsPCLWorker.cc#L262
or here :
https://github.com/dimattia/cmssw/blob/8bdcb4372e22de2ac5112b37575b3bead83546a5/CalibTracker/SiStripChannelGain/src/SiStripGainsPCLWorker.cc#L238
which are eventually not used - Same comments for SiStripGainFromCalibTree.cc .
What do you think ? thanks

Ciao @boudoul in principle yes because the pixel cluster charge is neither collected in the AlCARECO histograms nor used. Anyway, I would like to keep the Pixel hit processing for a while because we may discover that, with the new Pixel detector readout, filtering the statistic according to the quality of the pixel hits in the track helps the quality of the dE/dx estimation for the Strip.

cmsbuild · 2017-08-02T06:29:06Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 25
DQMHistoTests: Total histograms compared: 2651090
DQMHistoTests: Total failures: 46542
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2604366
DQMHistoTests: Total skipped: 181
DQMHistoTests: Total Missing objects: 0
Checked 102 log files, 14 edm output root files, 25 DQM output files

mmusich · 2017-08-02T09:17:47Z

Hello @dimattia, @boudoul
for the record, this is how RSS consumption profile vs time of step3 of runTheMatrix.py -l 1001.0 -t 4 looks like with this PR with respect to plain CMSSW_9_3_0_pre3, both measured on cmsdev03

There is ~ factor 2 reduction at peak RSS consumption, as well as a reduction in processing time, both roughly in line with expectations.

dimattia · 2017-08-02T10:03:39Z

Hi @mmusich thank you for the measurement which are an independent confirmation of the improvements. Anyway I want to stress that the reduction seen at peak level is not in line with the reduction of the memory that this implementation provides.

Indeed we achieve a reduction of a factor of 4 in memory (moving from 89000x2000 bins to 72500x687 bins); now the ClusterCharge histogram takes 25 MBytes while before it needed 100 MBytes. Therefore your measurements simply point out that the huge increase in memory consumption is not only due to the Histogram size. I think it also comes from some weird feature in the framework.

mmusich · 2017-08-02T10:13:38Z

@dimattia (commenting on how to read the plot)

Anyway I want to stress that the reduction seen at peak level is not in line with the reduction of the memory that this implementation provides.

step3 of runTheMatrix.py -l 1001.0 probes a variety of calibration workflows (not only the Strip Gains), and, obviously, the rest of the workflows are untouched. This means that in the plot in #20010 (comment) there is an underlying contribution that doesn't scale with the binning of the CC histogram. For a quick check I think my test is good enough, since anyway wf 1001.0 is what is tested in IB relvals and run in real life applications.

mmusich · 2017-08-02T10:23:36Z

CalibTracker/SiStripChannelGain/plugins/SiStripGainFromCalibTree.cc

+    float* binYarray = new float[688];
+    double p0 = 5.445;
+    double p1 = 0.002113;
+    double p2 = 69.01576;


maybe there is a better place for these magic numbers in CalibTracker/SiStripChannelGain/interface/APVGainHelpers.h since they are used here and in SiStripGainsPCLWorker.cc as well?
Also do we ever expect to change them again? Maybe can be passed as tracked arguments.

@mmusich these magic numbers are the result of the optimization work pointed out in the PR description. They are not supposed to be changed.

@dimattia thanks, do you really need 5 decimals to describe the function? anyway moving them in a single place is a good idea.

@mmusich concerning the place of the "magic numbers". These numbers concerns the binning of histograms which - for the time being - are duplicated among the PCWorker and the SiStripGainFromCalibTree. The re-engineering of SiStripGainFromCalibTree.cc will happen later and is not a subject of this PR.

davidlange6 · 2017-08-02T10:32:09Z

Note that 72500*687 shorts should be ~95MB (down from 340MB)... (multiplied by n threads and other factors in the dqm end job processing)

…

On Aug 2, 2017, at 12:03 PM, dimattia ***@***.***> wrote: Hi @mmusich thank you for the measurement which are an independent confirmation of the improvements. Anyway I want to stress that the reduction seen at peak level is not in line with the reduction of the memory that this implementation provides. Indeed we achieve a reduction of a factor of 4 in memory (moving from 89000x2000 bins to 72500x687 bins); now the ClusterCharge histogram takes 25 MBytes while before it needed 100 MBytes. Therefore your measurements simply point out that the huge increase in memory consumption is not only due to the Histogram size. I think it also comes from some weird feature in the framework. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cmsbuild · 2017-09-11T09:29:42Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/22846/console Started: 2017/09/11 12:16

cmsbuild · 2017-09-11T12:16:33Z

-1

Tested at: 841ec1b

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

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20010/22846/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

RelVals:

When I ran the RelVals I found an error in the following worklfows:
1306.0 step4

runTheMatrix-results/1306.0_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15/step4_SingleMuPt1_UP15+SingleMuPt1_UP15+DIGIUP15+RECOUP15+HARVESTUP15.log

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

cmsbuild · 2017-09-11T12:16:35Z

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

davidlange6 · 2017-09-12T07:47:14Z

please test

cmsbuild · 2017-09-12T07:47:35Z

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

cmsbuild · 2017-09-12T10:36:08Z

+1
Tested at: 841ec1b
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20010/22882/summary.html

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

cmsbuild · 2017-09-12T10:36:11Z

Comparison job queued.

cmsbuild · 2017-09-12T12:45:01Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-20010/22882/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: 2642439
DQMHistoTests: Total failures: 209
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2642040
DQMHistoTests: Total skipped: 189
DQMHistoTests: Total Missing objects: 0
Checked 107 log files, 14 edm output root files, 26 DQM output files

mmusich · 2017-09-14T09:21:27Z

@dimattia in the interest of trying to converge with this PR, can you apply the code-checks patch suggested above:
curl https://cmssdt.cern.ch/SDT/code-checks/PR-20010/611/git-diff.patch | patch -p1
Thanks

mmusich · 2017-09-15T09:48:57Z

@lpernie @arunhep @dmitrijus
As you have signed the backport of this PR (which is already merged) any objection in signing this one as well (I am not sure if by policy the code-checks failure is a red flag for integration)

arunhep · 2017-09-15T09:51:25Z

+1

Charge Histogram bins reduced

8bdcb43

cmsbuild added this to the CMSSW_9_3_X milestone Aug 1, 2017

cmsbuild added alca-pending comparison-pending dqm-pending orp-pending pending-signatures tests-pending labels Aug 1, 2017

dimattia mentioned this pull request Aug 1, 2017

Memory reduction of the Cluster Charge Histogram used in SiStrip gain calibration (92X) #20011

Merged

cmsbuild added tests-started and removed tests-pending labels Aug 1, 2017

cmsbuild added tests-approved and removed tests-started labels Aug 2, 2017

boudoul reviewed Aug 2, 2017

View reviewed changes

cmsbuild added comparison-available and removed comparison-pending labels Aug 2, 2017

dimattia changed the title ~~Charge Histogram bins reduced~~ Memory reduction of the Cluster Charge Histogram used in SiStrip gain calibration Aug 2, 2017

arunhep mentioned this pull request Aug 2, 2017

Updates on SiStrip G2 and calibtree #20019

Merged

mmusich reviewed Aug 2, 2017

View reviewed changes

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

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Sep 11, 2017

cmsbuild added comparison-pending tests-pending tests-started and removed comparison-notrun tests-rejected tests-pending labels Sep 12, 2017

cmsbuild added tests-approved and removed tests-started labels Sep 12, 2017

cmsbuild added comparison-available and removed comparison-pending labels Sep 12, 2017

davidlange6 merged commit eda3314 into cms-sw:master Sep 15, 2017

cmsbuild added alca-approved and removed alca-pending labels Sep 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory reduction of the Cluster Charge Histogram used in SiStrip gain calibration #20010

Memory reduction of the Cluster Charge Histogram used in SiStrip gain calibration #20010

dimattia commented Aug 1, 2017

cmsbuild commented Aug 1, 2017

arunhep commented Aug 1, 2017

cmsbuild commented Aug 1, 2017 •

edited

cmsbuild commented Aug 2, 2017

cmsbuild commented Aug 2, 2017

boudoul commented Aug 2, 2017

boudoul Aug 2, 2017

dimattia Aug 2, 2017

cmsbuild commented Aug 2, 2017

mmusich commented Aug 2, 2017

dimattia commented Aug 2, 2017

mmusich commented Aug 2, 2017 •

edited

mmusich Aug 2, 2017 •

edited

dimattia Aug 2, 2017

mmusich Aug 2, 2017

dimattia Aug 2, 2017

davidlange6 commented Aug 2, 2017 via email

cmsbuild commented Sep 11, 2017 •

edited

cmsbuild commented Sep 11, 2017

cmsbuild commented Sep 11, 2017

davidlange6 commented Sep 12, 2017

cmsbuild commented Sep 12, 2017 •

edited

cmsbuild commented Sep 12, 2017

cmsbuild commented Sep 12, 2017

cmsbuild commented Sep 12, 2017

mmusich commented Sep 14, 2017

mmusich commented Sep 15, 2017

arunhep commented Sep 15, 2017

Memory reduction of the Cluster Charge Histogram used in SiStrip gain calibration #20010

Memory reduction of the Cluster Charge Histogram used in SiStrip gain calibration #20010

Conversation

dimattia commented Aug 1, 2017

cmsbuild commented Aug 1, 2017

arunhep commented Aug 1, 2017

cmsbuild commented Aug 1, 2017 • edited

cmsbuild commented Aug 2, 2017

cmsbuild commented Aug 2, 2017

boudoul commented Aug 2, 2017

boudoul Aug 2, 2017

Choose a reason for hiding this comment

dimattia Aug 2, 2017

Choose a reason for hiding this comment

cmsbuild commented Aug 2, 2017

mmusich commented Aug 2, 2017

dimattia commented Aug 2, 2017

mmusich commented Aug 2, 2017 • edited

mmusich Aug 2, 2017 • edited

Choose a reason for hiding this comment

dimattia Aug 2, 2017

Choose a reason for hiding this comment

mmusich Aug 2, 2017

Choose a reason for hiding this comment

dimattia Aug 2, 2017

Choose a reason for hiding this comment

davidlange6 commented Aug 2, 2017 via email

cmsbuild commented Sep 11, 2017 • edited

cmsbuild commented Sep 11, 2017

cmsbuild commented Sep 11, 2017

davidlange6 commented Sep 12, 2017

cmsbuild commented Sep 12, 2017 • edited

cmsbuild commented Sep 12, 2017

cmsbuild commented Sep 12, 2017

cmsbuild commented Sep 12, 2017

mmusich commented Sep 14, 2017

mmusich commented Sep 15, 2017

arunhep commented Sep 15, 2017

cmsbuild commented Aug 1, 2017 •

edited

mmusich commented Aug 2, 2017 •

edited

mmusich Aug 2, 2017 •

edited

cmsbuild commented Sep 11, 2017 •

edited

cmsbuild commented Sep 12, 2017 •

edited