Skip to content

Commit

Permalink
Making first pass at updating deprecated ECAL modules
Browse files Browse the repository at this point in the history
  • Loading branch information
@atishelmanch
atishelmanch committed Nov 26, 2021
1 parent 4f68d5b commit f383597
Show file tree
Hide file tree
Showing 6 changed files with 20 additions and 23 deletions.
4 changes: 2 additions & 2 deletions CalibCalorimetry/EcalLaserAnalyzer/plugins/EcalStatusAnalyzer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ void EcalStatusAnalyzer::beginJob() {
}

//========================================================================
void EcalStatusAnalyzer::analyze(edm::StreamID,const edm::Event & e, const edm::EventSetup& c) const {
// void EcalStatusAnalyzer::analyze(const edm::Event& e, const edm::EventSetup& c) {
//void EcalStatusAnalyzer::analyze(edm::StreamID,const edm::Event & e, const edm::EventSetup& c) const {
void EcalStatusAnalyzer::analyze(const edm::Event& e, const edm::EventSetup& c) {
//========================================================================

++iEvent;
Expand Down
13 changes: 7 additions & 6 deletions CalibCalorimetry/EcalLaserAnalyzer/plugins/EcalStatusAnalyzer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,20 @@
#include <map>

#include <memory>
#include <FWCore/Framework/interface/global/EDAnalyzer.h>
#include <FWCore/Framework/interface/stream/EDAnalyzer.h>

This comment has been minimized.

Sign in to view

Copy link
@thomreis

thomreis Nov 26, 2021

Why did you make this a stream::EDAnalyzer? global::EDAnalyzer would be already a thread save module so I do not think there is need to change this module.

This comment has been minimized.

Sign in to view

Copy link
@atishelmanch

atishelmanch Nov 29, 2021

Author Owner

A general question: How do we make sure global and stream processes are thread safe? For example, if we create a module which fill histograms and writes to a file, but we define it as a global or stream, will scram b or anything else catch that it is not thread safe? Or rather, do we need to check this for ourselves?

This comment has been minimized.

Sign in to view

Copy link
@thomreis

thomreis Nov 29, 2021

The choice of the module type is a design decision. So the developer needs to decide if a module type fits to the algorithm that is going to be implemented. So in you example if you want to write to a file you will not select a stream module in general. You can specialise all three module types with template arguments that extend their capabilities (e.g. use non-thread save resources).

This comment has been minimized.

Sign in to view

Copy link
@atishelmanch

atishelmanch Nov 29, 2021

Author Owner

So the developer needs to decide if a module type fits to the algorithm that is going to be implemented.

Ok.

So in you example if you want to write to a file you will not select a stream module in general.

But if I did accidentally select a stream module, would this show up as an error somewhere?

This comment has been minimized.

Sign in to view

Copy link
@thomreis

thomreis Nov 29, 2021

No there would be no error to warn you about this. You might get a crash if two instances try to write to the same file at the same time.

This comment has been minimized.

Sign in to view

Copy link
@atishelmanch

atishelmanch Nov 29, 2021 •
edited
Loading

Author Owner

Ok, thanks for the explanation. Now for this module, I'm trying to set it to global but am having issues with the analyze method. It seems in global it is defined as a const function, but when I try to match this with the EcalStatusAnalyzer::analyze method, I am receiving many errors due to attempts to set values of member variables in a const function such as:

/afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/CalibCalorimetry/EcalLaserAnalyzer/plugins/EcalStatusAnalyzer.cc: In member function 'virtual void EcalStatusAnalyzer::analyze(edm::StreamID, const edm::Event&, const edm::EventSetup&) const':
/afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/CalibCalorimetry/EcalLaserAnalyzer/plugins/EcalStatusAnalyzer.cc:84:5: error: increment of member 'EcalStatusAnalyzer::iEvent' in read-only object
   84 |   ++iEvent;

This comment has been minimized.

Sign in to view

Copy link
@atishelmanch

atishelmanch Nov 29, 2021

Author Owner

Does this mean the module needs to be defined as one?

This comment has been minimized.

Sign in to view

Copy link
@thomreis

thomreis Nov 29, 2021

I guess you should just make this a one module then.

class Timestamp;

class EcalStatusAnalyzer : public edm::global::EDAnalyzer<> {
class EcalStatusAnalyzer : public edm::stream::EDAnalyzer<> {
public:
explicit EcalStatusAnalyzer(const edm::ParameterSet& iConfig);
~EcalStatusAnalyzer() override;

// void analyze(edm::StreamID, const edm::Event& e, const edm::EventSetup& c) override;
void analyze(edm::StreamID, const edm::Event& e, const edm::EventSetup& c) const override ;
void beginJob() override;
void endJob() override;
void analyze(const edm::Event& e, const edm::EventSetup& c);
//void analyze(edm::StreamID, const edm::Event& e, const edm::EventSetup& c);
//void analyze(edm::StreamID, const edm::Event& e, const edm::EventSetup& c) const override ;
void beginJob();
void endJob();

enum EcalLaserColorType {
iBLUE = 0,
Expand Down
8 changes: 4 additions & 4 deletions CalibCalorimetry/EcalLaserSorting/interface/LaserSorter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
#include <vector>

#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/EDAnalyzer.h"
#include <FWCore/Framework/interface/stream/EDAnalyzer.h>

This comment has been minimized.

Sign in to view

Copy link
@thomreis

thomreis Nov 26, 2021

EDAnalyzers are rarely stream because in general they need to see all events. So this should probably be global or one. Given that this writes to a file as well I would make it one.

This comment has been minimized.

Sign in to view

Copy link
@atishelmanch

atishelmanch Nov 29, 2021

Author Owner

Changing to one
#include "FWCore/Utilities/interface/InputTag.h"

#include "DataFormats/FEDRawData/interface/FEDRawDataCollection.h"
Expand All @@ -41,7 +41,7 @@
* the each output file.Once a file is completed (see above), it is renamed
* without the enclosing .part suffix.
*/
class LaserSorter : public edm::EDAnalyzer {
class LaserSorter : public edm::stream::EDAnalyzer<> {
//inner classes
private:
struct IndexRecord {
Expand Down Expand Up @@ -121,8 +121,8 @@ class LaserSorter : public edm::EDAnalyzer {
//methods
public:
void analyze(const edm::Event&, const edm::EventSetup&) override;
void endJob() override;
void beginJob() override;
void endJob() ;

This comment has been minimized.

Sign in to view

Copy link
@thomreis

thomreis Nov 26, 2021 •
edited
Loading

If you had to remove the override to get rid of a compile error this means that the base class of edm::stream::EDAnalyzer<> does not have a endJob defined. This means that the endJob() is never going to be called by the framework. Both, global and one would provide an endJob() so for those you should keep the override keyword.

This comment has been minimized.

Sign in to view

Copy link
@atishelmanch

atishelmanch Nov 29, 2021 •
edited
Loading

Author Owner

ok - switching to one and putting override's back to endJob and beginJob since, as you pointed out, I see them defined here in the RunWatcher template:

https://github.com/cms-sw/cmssw/blob/5c02f10b1f6568160171a0d5b9c6fa9b5b85267b/FWCore/Framework/interface/one/implementors.h#L94-L95

And loaded during the one class definition via:

"FWCore/Framework/interface/one/analyzerAbilityToImplementor.h"
--> "FWCore/Framework/interface/one/implementors.h"

This comment has been minimized.

Sign in to view

Copy link
@atishelmanch

atishelmanch Nov 29, 2021 •
edited
Loading

Author Owner

Actually to correct myself, I see functions for beginRun and endRun in the above link, and for endJob and beginJob here in EDAnalyzerBase:

https://github.com/cms-sw/cmssw/blob/5c02f10b1f6568160171a0d5b9c6fa9b5b85267b/FWCore/Framework/interface/one/EDAnalyzerBase.h#L107-L108

This comment has been minimized.

Sign in to view

Copy link
@thomreis

thomreis Nov 29, 2021

Yes, to use beginRun and endRun you would have to construct the module with the template argument edm::one::WatchRuns. Otherwise you only have beginJob and endJob.
void beginJob() ;
void beginRun(edm::Run const&, edm::EventSetup const&) override;

private:
Expand Down
4 changes: 2 additions & 2 deletions CalibCalorimetry/EcalPedestalOffsets/interface/testChannel.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#include "CalibCalorimetry/EcalPedestalOffsets/interface/TPedResult.h"
#include "CalibCalorimetry/EcalPedestalOffsets/interface/TPedValues.h"
#include "DataFormats/EcalDigi/interface/EcalDigiCollections.h"
#include "FWCore/Framework/interface/EDAnalyzer.h"
#include "FWCore/Framework/interface/one/EDAnalyzer.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include <map>
Expand All @@ -33,7 +33,7 @@
#include <string>
#include <vector>

class testChannel : public edm::EDAnalyzer {
class testChannel : public edm::one::EDAnalyzer<> {
public:
//! Constructor
testChannel(const edm::ParameterSet &ps);
Expand Down
10 changes: 3 additions & 7 deletions CalibCalorimetry/EcalTPGTools/plugins/EcalTPGParamReaderFromDB.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

// user include files
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/EDAnalyzer.h"
#include "FWCore/Framework/interface/one/EDAnalyzer.h"

#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/MakerMacros.h"
Expand All @@ -12,14 +12,14 @@

using namespace std;

class EcalTPGParamReaderFromDB : public edm::EDAnalyzer {
class EcalTPGParamReaderFromDB : public edm::one::EDAnalyzer<> {
public:
explicit EcalTPGParamReaderFromDB(const edm::ParameterSet&);
~EcalTPGParamReaderFromDB() override;

private:
void beginJob() override;
void analyze(const edm::Event&, const edm::EventSetup&) override;
void analyze(const edm::Event&, const edm::EventSetup&);

This comment has been minimized.

Sign in to view

Copy link
@thomreis

thomreis Nov 26, 2021

Why did you need to remove the override?

This comment has been minimized.

Sign in to view

Copy link
@atishelmanch

atishelmanch Nov 29, 2021

Author Owner

Looks like it doesn't need to be removed - I've put it back in.
void endJob() override;

std::string host;
Expand Down Expand Up @@ -48,10 +48,6 @@ EcalTPGParamReaderFromDB::~EcalTPGParamReaderFromDB() {}
void EcalTPGParamReaderFromDB::analyze(const edm::Event& ev, const edm::EventSetup& es) {
EcalTPGDBApp app(sid, user, pass);

//int i ;
//app.readTPGPedestals(i);
//app.writeTPGLUT();
//app.writeTPGWeights();
}

void EcalTPGParamReaderFromDB::beginJob() {}
Expand Down
4 changes: 2 additions & 2 deletions CalibCalorimetry/EcalTPGTools/plugins/testEcalTPGScale.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

// user include files
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/EDAnalyzer.h"
#include "FWCore/Framework/interface/one/EDAnalyzer.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
Expand All @@ -28,7 +28,7 @@

class CaloSubdetectorGeometry;

class testEcalTPGScale : public edm::EDAnalyzer {
class testEcalTPGScale : public edm::one::EDAnalyzer<> {
public:
explicit testEcalTPGScale(edm::ParameterSet const& pSet);
void analyze(const edm::Event& evt, const edm::EventSetup& evtSetup) override;
Expand Down

2 comments on commit f383597

@atishelmanch
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomreis - In light of cms-AlCaDB/AlCaTools#41, these past few commits contain a first pass at updating the deprecated ECAL modules found from the most recent static analyzer report: [Link]. The main purpose so far is to touch the modules to be updated, but there may be room for improvement as far as choosing one, stream or global for the class types [Reference].

What do you think?

@thomreis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Abe,
I realise now that I made comments only on this commit until now. I will move to the full branch.

Please sign in to comment.