Make ClusterMultiplicityFilter multithread safe #12024

nickmccoll · 2015-10-21T12:00:28Z

Small changes to use edm::stream::EDFilter<>, consumes, and edmNew::DetSetVector

cmsbuild · 2015-10-21T12:01:13Z

A new Pull Request was created by @nickmccoll (Nick McColl) for CMSSW_7_6_X.

Make ClusterMultiplicityFilter multithread safe

It involves the following packages:

RecoLocalTracker/SubCollectionProducers

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @forthommel, @yduhm, @GiacomoSguazzoni, @gbenelli, @rovere, @VinInn, @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' 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-10-21T12:04:55Z

RecoLocalTracker/SubCollectionProducers/interface/ClusterMultiplicityFilter.h

   public:
      explicit ClusterMultiplicityFilter(const edm::ParameterSet&);
      ~ClusterMultiplicityFilter();

   private:
-      virtual void beginJob() ;
+
      virtual bool filter(edm::Event&, const edm::EventSetup&);


please add override: filter(edm::Event&, const edm::EventSetup&) override;

cmsbuild · 2015-10-21T12:31:27Z

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

slava77 · 2015-10-21T12:39:24Z

RecoLocalTracker/SubCollectionProducers/src/ClusterMultiplicityFilter.cc

 #include "Geometry/TrackerGeometryBuilder/interface/TrackerGeometry.h"


-ClusterMultiplicityFilter::ClusterMultiplicityFilter(const edm::ParameterSet& iConfig)
+ClusterMultiplicityFilter::ClusterMultiplicityFilter(const edm::ParameterSet& iConfig) :
+  maxNumberOfClusters_(iConfig.getUntrackedParameter<unsigned int>("MaxNumberOfClusters")),


both config parameters affect the result of the code, so, they should be made tracked (remove "Untracked")
[not sure if this would trigger changes in large number of places: let's discuss if it does, given that list]

cmsbuild · 2015-10-21T12:51:47Z

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

slava77 · 2015-10-21T13:00:31Z

@cmsbuild please test
@nickmccoll thanks for the quick follow up

cmsbuild · 2015-10-21T13:01:18Z

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

Dr15Jones · 2015-10-21T13:26:42Z

RecoLocalTracker/SubCollectionProducers/src/ClusterMultiplicityFilter.cc

 {
-  maxNumberOfClusters_    = iConfig.getUntrackedParameter<unsigned int>("MaxNumberOfClusters");
-  clusterCollectionLabel_ = iConfig.getUntrackedParameter<std::string>("ClusterCollectionLabel");
+  clusters_ = consumes<edmNew::DetSetVector<SiStripCluster> >(clusterCollectionTag_);


You could make this const as well if you move its initialization to before the {.

cmsbuild · 2015-10-21T16:09:07Z

+1
Tested at: d2a350d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12024/9102/summary.html

cmsbuild · 2015-10-21T16:11:14Z

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

slava77 · 2015-10-21T16:12:34Z

@cmsbuild please test

cmsbuild · 2015-10-21T16:16:11Z

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

cmsbuild · 2015-10-21T18:04:05Z

+1
Tested at: ca6f124
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12024/9106/summary.html

cmsbuild · 2015-10-21T18:22:52Z

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

cmsbuild · 2015-10-21T19:28:52Z

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

slava77 · 2015-10-21T22:29:35Z

+1

for #12024 ca6f124

code changes look as described
jenkins tests pass: static analyzer does not complain about ClusterMultiplicityFilter anymore, and comparisons with baseline show no differences

@nickmccoll please make a separate PR for 80X (it should be possible to reuse the same topic branch)

cmsbuild · 2015-10-21T22:31:20Z

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

davidlange6 · 2015-10-22T07:21:10Z

+1

…ter_MultiThreadSafe Make ClusterMultiplicityFilter multithread safe

changes to ClusterMultiplicityFilter

8b70d18

cmsbuild added this to the Next CMSSW_7_6_X milestone Oct 21, 2015

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending comparison-pending labels Oct 21, 2015

slava77 reviewed Oct 21, 2015
View reviewed changes

added override

99d4ba9

slava77 reviewed Oct 21, 2015
View reviewed changes

Made parameters tracked and changed string to inputtag

d2a350d

cmsbuild added tests-started and removed tests-pending labels Oct 21, 2015

Dr15Jones reviewed Oct 21, 2015
View reviewed changes

Made ClusterMultiplicityFilter a global::EDFilter

ca6f124

cmsbuild added tests-approved and removed tests-started labels Oct 21, 2015

cmsbuild added tests-pending and removed tests-approved labels Oct 21, 2015

cmsbuild added tests-started and removed tests-pending labels Oct 21, 2015

cmsbuild added tests-approved and removed tests-started labels Oct 21, 2015

cmsbuild added comparison-available and removed comparison-pending labels Oct 21, 2015

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

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

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

Merge pull request #12024 from nickmccoll/Make_ClusterMultiplicityFil…

553351e

…ter_MultiThreadSafe Make ClusterMultiplicityFilter multithread safe

cmsbuild merged commit 553351e into cms-sw:CMSSW_7_6_X Oct 22, 2015

nickmccoll deleted the Make_ClusterMultiplicityFilter_MultiThreadSafe branch September 12, 2016 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ClusterMultiplicityFilter multithread safe #12024

Make ClusterMultiplicityFilter multithread safe #12024

nickmccoll commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

slava77 Oct 21, 2015

cmsbuild commented Oct 21, 2015

slava77 Oct 21, 2015

cmsbuild commented Oct 21, 2015

slava77 commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

Dr15Jones Oct 21, 2015

cmsbuild commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

slava77 commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

slava77 commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

davidlange6 commented Oct 22, 2015

Navigation Menu

Make ClusterMultiplicityFilter multithread safe #12024

Make ClusterMultiplicityFilter multithread safe #12024

Conversation

nickmccoll commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

slava77 Oct 21, 2015

Choose a reason for hiding this comment

cmsbuild commented Oct 21, 2015

slava77 Oct 21, 2015

Choose a reason for hiding this comment

cmsbuild commented Oct 21, 2015

slava77 commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

Dr15Jones Oct 21, 2015

Choose a reason for hiding this comment

cmsbuild commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

slava77 commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

slava77 commented Oct 21, 2015

cmsbuild commented Oct 21, 2015

davidlange6 commented Oct 22, 2015