[91X] SiPixelAli PCL payload creation thresholds from db - CondFormats #18087

mmusich · 2017-03-27T08:18:22Z

Greetings,
this Pull Request is the first step in making the payload creation thresholds of the Pixel Alignment workflow in Prompt Calibration Loop configurable from Database via Global Tag.
In this PR a new CondFormat is added to store the thresholds in a generic container class. The condition format has been designed:

allowing full granularity (i.e. possibility to specify each single threshold for each single degree of freedom)
allowing flexibility in the list of alignable detectors (might want to add / remove structures in future)
keeping in mind possible extensions to more than the current 6 spatial degrees of freedom that are aligned (sensor surfaces, pixel/strip calibration parameters, etc. etc.)

For a full presentation about the concept see this presentation
Once this is merged a second PR will follow, including changes in the consumer code in the Alignment package.

Attention: @ghellwig, @mschrode, @meng-xiao

cmsbuild · 2017-03-27T08:18:40Z

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

CondCore/PCLConfigPlugins
CondCore/Utilities
CondFormats/DataRecord
CondFormats/PCLConfig

The following packages do not have a category, yet:

CondCore/PCLConfigPlugins
CondFormats/PCLConfig
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories.py to assign category

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

cms-bot commands are listed here #13028

arunhep · 2017-03-27T09:35:57Z

please test

cmsbuild · 2017-03-27T09:36:10Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/18720/console Started: 2017/03/27 11:51

ghellwig

From my side, I'm fine with the PR after the requested changes are made.

ghellwig · 2017-03-27T10:22:05Z

CondFormats/PCLConfig/interface/AlignPCLThresholds.h

@@ -0,0 +1,58 @@
+#ifndef _AlignPCLThresholds_h_
+#define _AlignPCLThresholds_h_


@mmusich I think this include guard should read CondFormats_PCLConfig_interface_AlignPCLThresholds_h or similar

@ghellwig OK, picky but probably safer.
Is it recommended in the coding rules? I see it's rather common practice troughout the CondFormats package to define guards with the name of the class alone.

I don't know if it is written down somewhere explicitely, but it reduces the risk of clashes within whole CMSSW

ghellwig · 2017-03-27T10:22:35Z

CondFormats/PCLConfig/interface/AlignPCLThresholds.h

+#include <string>
+#include <vector>
+
+using namespace std;


@mmusich no using namespace statements in header files

ghellwig · 2017-03-27T10:23:40Z

CondFormats/PCLConfig/interface/AlignPCLThresholds.h

+  AlignPCLThresholds(){}
+  virtual ~AlignPCLThresholds(){}
+
+  void setAlignPCLThreshold(string AlignableId, const AlignPCLThreshold &Threshold);


please pass AlignableId as const std::string&
the same applies in the other methods

ghellwig · 2017-03-27T10:25:16Z

CondFormats/PCLConfig/plugins/AlignPCLThresholdsReader.cc

+
+namespace edmtest
+{
+  class AlignPCLThresholdsReader : public edm::EDAnalyzer


please make it an edm::one::EDAnalyzer

I don't think we'll ever need to have this run MT, but OK

true, but since it's a new class we should use the current standard

ghellwig · 2017-03-27T10:26:50Z

CondFormats/PCLConfig/plugins/AlignPCLThresholdsReader.cc

+
+  private:
+    bool printdebug_;
+    std::string formatedOutput_;


I believe these could be both const as is in general good practice for configuration parameters

ghellwig · 2017-03-27T10:29:43Z

CondFormats/PCLConfig/plugins/AlignPCLThresholdsWriter.cc

+
+      // ----------member data ---------------------------
+      std::string m_record;
+      unsigned int m_minNrecords;


could be both const

ghellwig · 2017-03-27T10:31:35Z

CondFormats/PCLConfig/plugins/AlignPCLThresholdsWriter.cc

+  m_parameters(iConfig.getParameter<std::vector<edm::ParameterSet> >("thresholds"))
+{
+   //now do what ever initialization is needed
+   usesResource("TFileService");


can you change that to usesResource(TFileService::kSharedResource);
it's technically the same, of course

ghellwig · 2017-03-27T10:32:23Z

CondFormats/PCLConfig/plugins/AlignPCLThresholdsWriter.cc

+
+   // loop on the PSet and insert the conditions 
+
+   std::vector<std::string> mandatories = {"X","Y","Z","thetaX","thetaY","thetaZ"};


this could become a std::array

any reason about preferring arrays instead of vectors?
I can reserve the size upfront, but I still prefer to handle vectors over arrays

this was meant as a suggestion to emphasize the rigid nature of this list
but if it is not meant to be rigid, a vector is certainly best

mmusich · 2017-03-27T10:47:15Z

Hi @ghellwig, OK - I think I'll take all suggestions once tests are done, just to make sure there is no other issue.

cmsbuild · 2017-03-27T11:35:32Z

+1
Tested at: 7e2c725
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18087/18720/summary.html

cmsbuild · 2017-03-27T11:35:35Z

Comparison job queued.

davidlange6 · 2017-03-27T12:28:23Z

It is indeed part of the CMSSW coding rules (not that its always enforced, but recently we are trying)

…

On Mar 27, 2017, at 2:07 PM, Gregor Mittag ***@***.***> wrote: @ghellwig commented on this pull request. In CondFormats/PCLConfig/interface/AlignPCLThresholds.h: > @@ -0,0 +1,58 @@ +#ifndef _AlignPCLThresholds_h_ +#define _AlignPCLThresholds_h_ I don't know if it is written down somewhere explicitely, but it reduces the risk of clashes within whole CMSSW — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cmsbuild · 2017-03-27T13:19:28Z

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

ghellwig · 2017-03-27T13:45:16Z

@davidlange6, @mmusich,
I think the new packages should be assigned to db, right?
Who should take care of the corresponding PR to cms-bot?

…ignable label

…ules

cmsbuild · 2017-03-29T08:31:07Z

Pull request #18087 was updated. @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @ggovi, @mmusich, @davidlange6 can you please check and sign again.

ghellwig · 2017-03-29T08:35:17Z

please test

cmsbuild · 2017-03-29T08:35:35Z

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

cmsbuild · 2017-03-29T10:20:10Z

+1
Tested at: 73b9831
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18087/18772/summary.html

cmsbuild · 2017-03-29T10:20:13Z

Comparison job queued.

cmsbuild · 2017-03-29T11:36:36Z

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

mmusich · 2017-03-29T21:19:50Z

@ghellwig, @arunhep, @franzoni, @ggovi
Is there anything else I can do to get the PR signed again?
By the way, we'll need this merged in pre2 (due in two days according to https://twiki.cern.ch/twiki/bin/view/CMS/CMSSW_9_1_0) in order to propagate the new payload in Global Tag and change the consumer code.

ghellwig · 2017-03-30T08:24:36Z

+1

ggovi · 2017-03-30T15:26:42Z

+1

cmsbuild · 2017-03-30T15:27:01Z

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 requires discussion in the ORP meeting before it's merged. @Muzaffar, @davidlange6, @smuzaffar

smuzaffar · 2017-03-31T07:01:55Z

@davidlange6 @mmusich , looks like this PR broke IBs https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc530/CMSSW_9_1_X_2017-03-30-2300/CondFormats/PCLConfig
I shall check why the PR tests did not catch this.

mmusich · 2017-03-31T07:20:04Z

@smuzaffar might it be because of this: #18117 merged at the same time? Looks like AlignPCLThresholdsReader needs to have a proper include #include "FWCore/Framework/interface/one/EDAnalyzer.h" . I am testing it now.

davidlange6 · 2017-03-31T07:21:18Z

as just discussed with @smuzaffar and @dr15johns, this is from a header cleanup in fwk. @mmusich just needs to include the headers used by this code and will be ok.

…

On Mar 31, 2017, at 9:01 AM, Malik Shahzad Muzaffar ***@***.***> wrote: @davidlange6 @mmusich , looks like this PR broke IBs https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc530/CMSSW_9_1_X_2017-03-30-2300/CondFormats/PCLConfig I shall check why the PR tests did not catch this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mmusich · 2017-03-31T07:21:54Z

@davidlange6 I am preparing the branch; will PR it in a few minutes.

mmusich · 2017-03-31T07:27:26Z

#18137 should address the issue. Sorry for that.

mmusich added 2 commits March 27, 2017 10:03

Adding AlignPCLThresholds CondFormat

6076ecc

Mods to include AlignPCLThresholds in AlCa/DB infrastructure

7e2c725

cmsbuild added this to the Next CMSSW_9_1_X milestone Mar 27, 2017

cmsbuild added alca-pending comparison-pending db-pending new-package-pending orp-pending pending-signatures tests-pending labels Mar 27, 2017

cmsbuild added tests-started and removed tests-pending labels Mar 27, 2017

ghellwig reviewed Mar 27, 2017

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Mar 27, 2017

cmsbuild added comparison-available and removed comparison-pending labels Mar 27, 2017

mmusich added 2 commits March 27, 2017 15:47

pass by reference all methods arguments and add function to return al…

72ca154

…ignable label

Modernize Reader/Writer plugins to comply with current CMSSW coding r…

c6dc666

…ules

cmsbuild removed comparison-available tests-approved labels Mar 27, 2017

cmsbuild added the tests-pending label Mar 29, 2017

cmsbuild added tests-started and removed tests-pending labels Mar 29, 2017

cmsbuild added tests-approved and removed tests-started labels Mar 29, 2017

cmsbuild added comparison-available and removed comparison-pending labels Mar 29, 2017

cmsbuild added alca-approved and removed alca-pending labels Mar 30, 2017

cmsbuild added db-approved fully-signed and removed db-pending pending-signatures labels Mar 30, 2017

davidlange6 merged commit 504c5ff into cms-sw:master Mar 30, 2017

mmusich mentioned this pull request Apr 4, 2017

[91X] SiPixelAli PCL payload creation thresholds from db - Consumer Code #18196

Merged

		@@ -0,0 +1,58 @@
		#ifndef _AlignPCLThresholds_h_
		#define _AlignPCLThresholds_h_


		// loop on the PSet and insert the conditions

		std::vector<std::string> mandatories = {"X","Y","Z","thetaX","thetaY","thetaZ"};

[91X] SiPixelAli PCL payload creation thresholds from db - CondFormats #18087

[91X] SiPixelAli PCL payload creation thresholds from db - CondFormats #18087

Conversation

mmusich commented Mar 27, 2017 • edited

cmsbuild commented Mar 27, 2017

arunhep commented Mar 27, 2017

cmsbuild commented Mar 27, 2017 • edited

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

mmusich commented Mar 27, 2017

cmsbuild commented Mar 27, 2017

cmsbuild commented Mar 27, 2017

davidlange6 commented Mar 27, 2017 via email

cmsbuild commented Mar 27, 2017

ghellwig commented Mar 27, 2017

cmsbuild commented Mar 29, 2017

ghellwig commented Mar 29, 2017

cmsbuild commented Mar 29, 2017 • edited

cmsbuild commented Mar 29, 2017

cmsbuild commented Mar 29, 2017

cmsbuild commented Mar 29, 2017

mmusich commented Mar 29, 2017

ghellwig commented Mar 30, 2017

ggovi commented Mar 30, 2017

cmsbuild commented Mar 30, 2017

smuzaffar commented Mar 31, 2017

mmusich commented Mar 31, 2017

davidlange6 commented Mar 31, 2017 via email

mmusich commented Mar 31, 2017

mmusich commented Mar 31, 2017

mmusich commented Mar 27, 2017 •

edited

cmsbuild commented Mar 27, 2017 •

edited

cmsbuild commented Mar 29, 2017 •

edited