Permanently enabling method 3 #11507

igv4321 · 2015-09-26T04:40:59Z

Permanently enabling "Method 3" in HcalSimpleRecAlgo.

Some trivial cleanup of HcalSimpleRecAlgo (e.g., removed unused default constructor, added a buffer vector of doubles so that this vector does not have to be reallocated for every digi, etc).

Require proper initialization of "Method 3" in the HcalHitReconstructor module.

cmsbuild · 2015-09-26T04:41:10Z

A new Pull Request was created by @igv4321 (Igor Volobouev) for CMSSW_7_6_X.

Permanently enabling method 3

It involves the following packages:

RecoLocalCalo/HcalRecAlgos
RecoLocalCalo/HcalRecProducers

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@argiro this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

slava77 · 2015-09-26T15:41:54Z

RecoLocalCalo/HcalRecAlgos/interface/HcalSimpleRecAlgo.h

  // S.Brandt Feb19 : Add a pointer to the HLT algo
  std::auto_ptr<HcalDeterministicFit> hltOOTpuCorr_;
+
+  // Speed up the code a little bit by not allocating a vector every time
+  mutable std::vector<double> vectorBuffer_;


What's the size of the saving from this buffer?
I'd much rather see no mutables.

please remove this.
I don't see much benefit.

The saving does not come from the "size" of the vector. It is instead about not allocating (and then deallocating) the memory from the heap for each calorimeter cell processed.

there are several more allocations in the methods called internally.
So, if a fix needs to be done, better do it properly.

Your change with a mutable opens a possibility to pass information between hits.
This is not good.

Cleaning and clearing is apparently done correctly now, but this makes the code much more fragile in the future developments.

this is with 25ns hlt in 760pre5
https://slava77sk.web.cern.ch/slava77sk/reco/cgi-bin/igprof-navigator/CMSSW_7_6_0_pre5-orig.256729.step2-hlt.pp/797

i don't see any cpu cost reasons for this buffer

The allocations internal to "Method 2" and "Method 3" classes are, of course, irrelevant to this discussion. Bug the authors of the corresponding classes if you are so inclined.

The change with a mutable, of course, does not open any possibility to pass information between hits. The vector is cleared any time it is used.

Any code is "fragile" to some degree. I believe, the advantages outweigh the drawbacks in this particular case. The "mutable" keyword is included into the language precisely for the uses similar to this.

The cost reasons are obvious: we avoid allocating the contents of the vector on the heap for every hcal rechit.

slava77 · 2015-09-26T16:04:26Z

@cmsbuild please test

cmsbuild · 2015-09-26T16:06:09Z

The tests are being triggered in jenkins.

cmsbuild · 2015-09-26T17:33:28Z

+1
Tested at: 9ec547d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11507/8413/summary.html

cmsbuild · 2015-09-26T18:26:14Z

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

slava77 · 2015-09-27T16:19:29Z

-1
pending code review

fwyzard · 2015-09-27T21:44:49Z

I do not understand lines 167 and 172 of RecoLocalCalo/HcalRecProducers/src/HcalHitReconstructor.cc .

slava77 · 2015-09-28T01:58:48Z

@fwyzard
a good question, actually,
the change was introduced back in #5389 or, originally authored by @FHead #5168
Why was this setPileupCorrection_ = 0; added right after assignment setPileupCorrection_ = &HcalSimpleRecAlgo::set{HF or HO}PileupCorrection;

slava77 · 2015-09-28T12:06:40Z

@igv4321
Please factor out the technical optimization (mutable vector buffer) into a separate PR.
We need the physics level feature (M3 values stored) in the release.

The optimization made is still questionable to me and it is not time critical.
Also, a more appropriate solution will not include the vector, which makes this improvement only temporary.

Thank you.

igv4321 · 2015-09-28T18:18:07Z

Do I need to make a comment for the system to see the latest push?

cmsbuild · 2015-09-28T18:21:54Z

Pull request #11507 was updated. @cmsbuild, @cvuosalo, @slava77 can you please check and sign again.

slava77 · 2015-09-28T18:27:29Z

Igor,

yes, unfortunately cms-bot doesn't notice a commit from an "old" branch and a message is needed.

Thank you very much for the factorization of the technical performance.

I noticed a change in a follow up to Andrea's comment about
setPileupCorrection_ = 0;
Could you please elaborate a bit on the history of the changes here.

slava77 · 2015-09-28T18:27:38Z

@cmsbuild please test

cmsbuild · 2015-09-28T18:31:50Z

The tests are being triggered in jenkins.

igv4321 · 2015-09-28T19:01:24Z

"setPileupCorrection_" variable is actually configured (as intended) by module parameters "dataOOTCorrectionName", "dataOOTCorrectionCategory", "mcOOTCorrectionName", and "mcOOTCorrectionCategory". Yi Chen, who put those lines in, simply did not realize this.

It is harmless to set this variable to 0 at those points in the code, as "Method 1" was not intended for HF and HO to begin with. While "Method 1" is currently deprecated, I don't want to completely remove this mechanism yet -- perhaps, some other method will be developed, inheriting from AbsOOTPileupCorrection.

cmsbuild · 2015-09-28T20:04:21Z

+1
Tested at: bb4492a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11507/8459/summary.html

cmsbuild · 2015-09-29T11:37:36Z

+1
Tested at: bb4492a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11507/8465/summary.html

cmsbuild · 2015-09-29T12:47:24Z

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

slava77 · 2015-09-30T23:23:17Z

@igv4321
Do you know of the plans to add DQM/Validation plots to monitor the M3 value saved in the aux and also compare the reco (M2) and M3?

Currently, jenkins comparisons show no differences from the change made here (as expected from the lack of the DQM/validation plots)

slava77 · 2015-10-01T03:00:08Z

+1

for #11507 bb4492a

code changes are rather minimal and look OK
jenkins test pass and comparisons with baseline show no difference
tested locally in CMSSW_7_6_X_2015-09-29-0900. The following numbers are from 134.805 workflow (a 25ns run 254790 from 2015C)
- event size increases negligibly: hbhe rechit size is up by about 10%, which adds up to 0.5% in RECO, but is a factor of a few smaller in AOD (only reduced rechit collection is stored)
- CPU time increases negligibly: hbheprereco time is up by about 2% (0.1% of all reco time)
- the M3 energy now shows in in eaux() and appears to be correlated well with the M2 energy (::energy())

cmsbuild · 2015-10-01T03:01:10Z

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

abdoulline · 2015-10-01T05:48:02Z

<...>
Do you know of the plans to add DQM/Validation plots to monitor the M3 value saved in the aux and also compare the reco (M2) and M3?
<...>

The (original) idea was just to have M3 vs M2 right away without manupulation of different input files and DetId matching for on-demand comparisons/tests/checks (like the ones performed for "Early 2015 Physics Commissioning" task force).
But including this kind of comparison (M3 used in HLT vs M2 used in Offline) in Offline data DQM and MC Validation seems to be a good idea.

davidlange6 · 2015-10-01T10:31:39Z

+1

Permanently enabling method 3

Permanently enabling method 3

9ec547d

cmsbuild added this to the Next CMSSW_7_6_X milestone Sep 26, 2015

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Sep 26, 2015

slava77 reviewed Sep 26, 2015
View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Sep 26, 2015

cmsbuild added tests-approved and removed tests-started labels Sep 26, 2015

cmsbuild added comparison-available and removed comparison-pending labels Sep 26, 2015

cmsbuild added reconstruction-rejected and removed reconstruction-pending labels Sep 27, 2015

reverting code optimizations

bb4492a

cmsbuild removed comparison-available reconstruction-rejected labels Sep 28, 2015

cmsbuild added reconstruction-pending tests-pending comparison-pending labels Sep 28, 2015

cmsbuild added tests-started and removed tests-pending labels Sep 28, 2015

cmsbuild added tests-approved and removed tests-started labels Sep 28, 2015

cmsbuild added comparison-available and removed comparison-pending labels Sep 29, 2015

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Oct 1, 2015

cmsbuild added orp-approved and removed orp-pending labels Oct 1, 2015

cmsbuild added a commit that referenced this pull request Oct 1, 2015

Merge pull request #11507 from igv4321/always-run-hcal-method3

3cca4f7

Permanently enabling method 3

cmsbuild merged commit 3cca4f7 into cms-sw:CMSSW_7_6_X Oct 1, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permanently enabling method 3 #11507

Permanently enabling method 3 #11507

igv4321 commented Sep 26, 2015

cmsbuild commented Sep 26, 2015

slava77 Sep 26, 2015

slava77 Sep 26, 2015

igv4321 Sep 26, 2015

slava77 Sep 26, 2015

slava77 Sep 26, 2015

igv4321 Sep 26, 2015

slava77 commented Sep 26, 2015

cmsbuild commented Sep 26, 2015

cmsbuild commented Sep 26, 2015

cmsbuild commented Sep 26, 2015

slava77 commented Sep 27, 2015

fwyzard commented Sep 27, 2015

slava77 commented Sep 28, 2015

slava77 commented Sep 28, 2015

igv4321 commented Sep 28, 2015

cmsbuild commented Sep 28, 2015

slava77 commented Sep 28, 2015

slava77 commented Sep 28, 2015

cmsbuild commented Sep 28, 2015

igv4321 commented Sep 28, 2015

cmsbuild commented Sep 28, 2015

cmsbuild commented Sep 29, 2015

cmsbuild commented Sep 29, 2015

slava77 commented Sep 30, 2015

slava77 commented Oct 1, 2015

cmsbuild commented Oct 1, 2015

abdoulline commented Oct 1, 2015

davidlange6 commented Oct 1, 2015

Permanently enabling method 3 #11507

Permanently enabling method 3 #11507

Conversation

igv4321 commented Sep 26, 2015

cmsbuild commented Sep 26, 2015

slava77 Sep 26, 2015

Choose a reason for hiding this comment

slava77 Sep 26, 2015

Choose a reason for hiding this comment

igv4321 Sep 26, 2015

Choose a reason for hiding this comment

slava77 Sep 26, 2015

Choose a reason for hiding this comment

slava77 Sep 26, 2015

Choose a reason for hiding this comment

igv4321 Sep 26, 2015

Choose a reason for hiding this comment

slava77 commented Sep 26, 2015

cmsbuild commented Sep 26, 2015

cmsbuild commented Sep 26, 2015

cmsbuild commented Sep 26, 2015

slava77 commented Sep 27, 2015

fwyzard commented Sep 27, 2015

slava77 commented Sep 28, 2015

slava77 commented Sep 28, 2015

igv4321 commented Sep 28, 2015

cmsbuild commented Sep 28, 2015

slava77 commented Sep 28, 2015

slava77 commented Sep 28, 2015

cmsbuild commented Sep 28, 2015

igv4321 commented Sep 28, 2015

cmsbuild commented Sep 28, 2015

cmsbuild commented Sep 29, 2015

cmsbuild commented Sep 29, 2015

slava77 commented Sep 30, 2015

slava77 commented Oct 1, 2015

cmsbuild commented Oct 1, 2015

abdoulline commented Oct 1, 2015

davidlange6 commented Oct 1, 2015