Stateless clusterizer for SiStrip #12307

VinInn · 2015-11-07T15:19:42Z

make SiStrip clusterizer stateless to support multi-thread on demand.
This is required before making the container itself Thread Safe.
Purely technical changes, tested extensively long ago.
Results should be bit-identical to previous version

cmsbuild · 2015-11-07T15:21:13Z

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_8_0_X.

Stateless clusterizer for SiStrip

It involves the following packages:

RecoLocalTracker/SiStripClusterizer

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @forthommel, @yduhm, @GiacomoSguazzoni, @gbenelli, @rovere, @VinInn, @nickmccoll, @jlagram, @gpetruc, @cerati, @threus 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' or '@cmsbuild, 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-11-07T15:31:29Z

@cmsbuild please test

cmsbuild · 2015-11-07T15:36:09Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9508/console

cmsbuild · 2015-11-07T16:59:01Z

+1
Tested at: b902f4e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12307/9508/summary.html

cmsbuild · 2015-11-07T18:05:50Z

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

VinInn · 2015-11-07T20:43:39Z

something went wrong in the merge...
will have to check in more details the HLT workflow...

VinInn · 2015-11-08T11:37:50Z

tested (with debug on) for step2 of 134.91
identical output to original

VinInn · 2015-11-08T11:38:05Z

@cmsbuild please test

slava77 · 2015-11-08T11:40:46Z

@cmsbuild please test

cmsbuild · 2015-11-08T11:41:09Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9512/console

cmsbuild · 2015-11-08T11:41:12Z

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

slava77 · 2015-11-09T19:07:44Z

RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h

-  float noise(const uint16_t& strip) const { return SiStripNoises::getNoise( strip, noiseRange ); }
-  float gain(const uint16_t& strip)  const { return SiStripGain::getStripGain( strip, gainRange ); }
-  bool bad(const uint16_t& strip)    const { return qualityHandle->IsStripBad( qualityRange, strip ); }
+  // uint32_t currentId() const {return detId;}


this can be removed, unless needed for some imminent developments

Thanks for noticing.
I will remove it next round (when integrating ThreadSafe DetSetVector)

slava77 · 2015-11-09T19:38:11Z

I will delay signoff until the changes from #12161 make it through or a 80X PR equivalent of #12161 appears.

VinInn · 2015-11-10T08:12:30Z

I disagree.
Please integrate this one.
We will deal with #12161 later on
@boudoul please give your opinion
(btw Gaelle should be added to LocalReco, Calib etc (as TRK-DPG coord), Gabriele removed...)

I need this to continue making HLT thread safe.
FYI @fwyzard , @Dr15Jones
Next PR is ready...

Dr15Jones · 2015-11-10T14:41:04Z

RecoLocalTracker/SiStripClusterizer/interface/StripClusterizerAlgorithm.h

-  float gain(const uint16_t& strip)  const { return SiStripGain::getStripGain( strip, gainRange ); }
-  bool bad(const uint16_t& strip)    const { return qualityHandle->IsStripBad( qualityRange, strip ); }
+  // uint32_t currentId() const {return detId;}
+  Det setDetId(const uint32_t) const;


Maybe findDetId might be a less confusing name? A const set function usually sets off my warning bells. In this case nothing is actually set.

not anymore at least!
thanks for all other suggestions as well.

Dr15Jones · 2015-11-10T15:08:47Z

Based on my cursory overview, it seems pretty good to me beyond the few comments I had.

davidlange6 · 2015-11-11T16:03:11Z

@slava77 - given the state of the strip unpacker PR, I think we can move ahead with this and eventually put the unpacker in 80x once its fully checked out for HI (and for old data)

slava77 · 2015-11-11T16:22:36Z

There are several comments from Chris which need a follow up (or a notice that they will be fixed in the next PR as happened to my comments).

VinInn · 2015-11-11T16:36:17Z

I think that @Dr15Jones will be satisfied that his comments get adressed in the next PR that will also
include the Concurrent version of the DetSetVector

Dr15Jones · 2015-11-11T16:39:30Z

fine with me

slava77 · 2015-11-11T17:51:37Z

+1

for #12307 d9629b4

changes in the code are in line with the description; updates from the code review are expected in the upcoming PR
jenkins tests pass and comparisons with the baseline show no difference
local test on top of CMSSW_8_0_0_pre1 using run2 134.802 show no substantive difference in memory or CPU timing utilization

@boudoul @forthommel please note this PR with changes overlapping with the strip unpacker updates: some extra care may be needed to merge the strip unpacker updates into 80X, it's better to have a separate PR for that, once the issues with the unpacker DQM are sorted out.

cmsbuild · 2015-11-11T17:56:14Z

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

davidlange6 · 2015-11-12T06:48:32Z

+1

Stateless clusterizer for SiStrip

VinInn added 2 commits November 7, 2015 15:45

make clusterizer stateless to support multi-thread on demand

f960274

a bit of c++14

b902f4e

cmsbuild added this to the Next CMSSW_8_0_X milestone Nov 7, 2015

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Nov 7, 2015

cmsbuild added tests-started and removed tests-pending labels Nov 7, 2015

cmsbuild added tests-approved and removed tests-started labels Nov 7, 2015

cmsbuild added comparison-available and removed comparison-pending labels Nov 7, 2015

do not split cluster at edge of APV

d9629b4

cmsbuild added tests-pending comparison-pending and removed comparison-available tests-approved labels Nov 8, 2015

slava77 reviewed Nov 9, 2015
View reviewed changes

slava77 mentioned this pull request Nov 9, 2015

New FED modes handled in SiStrip unpacker #12161

Closed

Dr15Jones reviewed Nov 10, 2015
View reviewed changes

cmsbuild added reconstruction-approved fully-signed and removed pending-signatures reconstruction-pending labels Nov 11, 2015

cmsbuild added orp-approved and removed orp-pending labels Nov 12, 2015

cmsbuild added a commit that referenced this pull request Nov 12, 2015

Merge pull request #12307 from VinInn/StatelessClusterizer

877f2dd

Stateless clusterizer for SiStrip

cmsbuild merged commit 877f2dd into cms-sw:CMSSW_8_0_X Nov 12, 2015

slava77 mentioned this pull request Nov 12, 2015

Updated SiStrip unpacker (new FED readout modes) #12387

Merged

forthommel added a commit to forthommel/cmssw that referenced this pull request Nov 12, 2015

Merge of cms-sw#12307 removed from last commit

d084963

slava77 pushed a commit to slava77/cmssw that referenced this pull request Nov 15, 2015

Merge of cms-sw#12307 removed from last commit

4f03c51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stateless clusterizer for SiStrip #12307

Stateless clusterizer for SiStrip #12307

VinInn commented Nov 7, 2015

cmsbuild commented Nov 7, 2015

slava77 commented Nov 7, 2015

cmsbuild commented Nov 7, 2015

cmsbuild commented Nov 7, 2015

cmsbuild commented Nov 7, 2015

VinInn commented Nov 7, 2015

VinInn commented Nov 8, 2015

VinInn commented Nov 8, 2015

slava77 commented Nov 8, 2015

cmsbuild commented Nov 8, 2015

cmsbuild commented Nov 8, 2015

slava77 Nov 9, 2015

VinInn Nov 10, 2015

slava77 commented Nov 9, 2015

VinInn commented Nov 10, 2015

Dr15Jones Nov 10, 2015

VinInn Nov 10, 2015

Dr15Jones commented Nov 10, 2015

davidlange6 commented Nov 11, 2015

slava77 commented Nov 11, 2015

VinInn commented Nov 11, 2015

Dr15Jones commented Nov 11, 2015

slava77 commented Nov 11, 2015

cmsbuild commented Nov 11, 2015

davidlange6 commented Nov 12, 2015

Stateless clusterizer for SiStrip #12307

Stateless clusterizer for SiStrip #12307

Conversation

VinInn commented Nov 7, 2015

cmsbuild commented Nov 7, 2015

slava77 commented Nov 7, 2015

cmsbuild commented Nov 7, 2015

cmsbuild commented Nov 7, 2015

cmsbuild commented Nov 7, 2015

VinInn commented Nov 7, 2015

VinInn commented Nov 8, 2015

VinInn commented Nov 8, 2015

slava77 commented Nov 8, 2015

cmsbuild commented Nov 8, 2015

cmsbuild commented Nov 8, 2015

slava77 Nov 9, 2015

Choose a reason for hiding this comment

VinInn Nov 10, 2015

Choose a reason for hiding this comment

slava77 commented Nov 9, 2015

VinInn commented Nov 10, 2015

Dr15Jones Nov 10, 2015

Choose a reason for hiding this comment

VinInn Nov 10, 2015

Choose a reason for hiding this comment

Dr15Jones commented Nov 10, 2015

davidlange6 commented Nov 11, 2015

slava77 commented Nov 11, 2015

VinInn commented Nov 11, 2015

Dr15Jones commented Nov 11, 2015

slava77 commented Nov 11, 2015

cmsbuild commented Nov 11, 2015

davidlange6 commented Nov 12, 2015