Luminosity producer using files from brilcalc #29271
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29271/14322
|
|
A new Pull Request was created by @plujan (Paul Lujan) for master. It involves the following packages: RecoLuminosity/LumiProducer @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| class LumiProducerFromBrilcalc : public edm::stream::EDProducer<> { | ||
| public: | ||
| explicit LumiProducerFromBrilcalc(const edm::ParameterSet&); | ||
| ~LumiProducerFromBrilcalc(); |
There was a problem hiding this comment.
The default destructor can be used instead
| static void fillDescriptions(edm::ConfigurationDescriptions& descriptions); | ||
|
|
||
| private: | ||
| virtual void beginStream(edm::StreamID) override; |
There was a problem hiding this comment.
All these methods but the produce() one do nothing.
Do they really need to be defined here? Is there any work in progress expected for eventually populating them?
Otherwise, you could simplify and just remove all them
There was a problem hiding this comment.
No, it's just leftover boilerplate from the skeleton, so it can certainly be taken out.
| virtual void produce(edm::Event&, const edm::EventSetup&) override; | ||
| virtual void endStream() override; | ||
| virtual void beginRun(edm::Run const&, edm::EventSetup const&) override; | ||
| //virtual void endRun(edm::Run const&, edm::EventSetup const&) override; |
There was a problem hiding this comment.
As above: in particular, please remove the commented out methods, as well as their implementation, if not needed
| //virtual void endLuminosityBlock(edm::LuminosityBlock const&, edm::EventSetup const&) override; | ||
|
|
||
| // ----------member data --------------------------- | ||
| std::string lumiFile_; |
|
|
||
| // ----------member data --------------------------- | ||
| std::string lumiFile_; | ||
| bool throwIfNotFound_; |
| // ----------member data --------------------------- | ||
| std::string lumiFile_; | ||
| bool throwIfNotFound_; | ||
| bool doBunchByBunch_; |
| std::map<std::pair<int, int>, std::pair<float, float> > lumiData_; | ||
| }; | ||
|
|
||
| // |
There was a problem hiding this comment.
Please remove the unneeded commented out lines
| std::string lumiFile_; | ||
| bool throwIfNotFound_; | ||
| bool doBunchByBunch_; | ||
| std::map<std::pair<int, int>, std::pair<float, float> > lumiData_; |
There was a problem hiding this comment.
This cannot be const, since it is initialized in the constructor.
Anyhow, the internal state of the producer is never modified while processing the events: the producer can become a global module, then
There was a problem hiding this comment.
Sure, done (this also required changing the lumiData_[] accesses to lumiData_.at(), to keep const).
| void LumiProducerFromBrilcalc::fillDescriptions(edm::ConfigurationDescriptions& descriptions) { | ||
| // Allowed parameters | ||
| edm::ParameterSetDescription desc; | ||
| desc.addUntracked<std::string>("lumiFile"); |
There was a problem hiding this comment.
Ok, this is now used only in private analyses, therefore it is mostly a matter of taste whether to define a parameter tracked or untracked.
On the other hand, if this is going to be processed centrally, it will certainly become important to keep track of the lumi file used, or whether the missing runs in it had been given lumi=0 or not.
I think that all these parameters should be tracked, instead
There was a problem hiding this comment.
Right, sounds reasonable, changed.
* removed unused stuff from skeleton * change to global module * change parameters to tracked
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@plujan I assume that you checked that with the latest (small) updates the producer still runs as expected: could you please confirm (by running your test job, for example)? |
|
Yes, the test job still runs successfully. Also, I forgot to ask: should I also create a backport of this PR for 10_X? |
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
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 will now be reviewed by the release team before it's merged. @davidlange6, @silviodonato, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
I imagine that a backport in 10_6, to be used for the UL analyses, can be useful. All possible backports should be decided based on physics needs, though. Technically, this code automatically satisfies the no-change policy, and it is therefore viable to be backported |
|
+1 |
PR description:
This adds a new producer,
LumiProducerFromBrilcalc, which will produce luminosity information in CMSSW using a csv file produced bybrilcalc.LumiInfoclass that this creates, see https://twiki.cern.ch/twiki/bin/view/CMS/LuminosityCMSSWPR validation:
The included
TestLumiProducerFromBrilcalchas been created and run to verify that it works properly (although it does not work as an automated test).I have also checked the steps on the PR checklist and added some documentation in README.md.
Notes: