Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

From cmssw 12 2 0 pre2 ecal deprecated modules #33

Conversation

thomreis
Copy link
Owner

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

@thomreis
Copy link
Owner Author

Hi @atishelmanch since I can not make comments on a complete branch I made a PR to my repository to be able to comment here.

@@ -44,7 +44,7 @@
// CMSSW include files
#include "CondCore/CondDB/interface/Time.h"
#include "DataFormats/Common/interface/Handle.h"
#include "FWCore/Framework/interface/EDAnalyzer.h"
#include "FWCore/Framework/interface/stream/EDAnalyzer.h"
Copy link
Owner Author

Choose a reason for hiding this comment

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

EDAnalyzers are typically global or one since they need to see all events.

Choose a reason for hiding this comment

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

Setting as one module

@@ -76,6 +76,7 @@ void EcalStatusAnalyzer::beginJob() {
}

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

Choose a reason for hiding this comment

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

Remove commented code.

Choose a reason for hiding this comment

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

Indeed

@@ -4,18 +4,20 @@
#include <map>

#include <memory>
#include <FWCore/Framework/interface/EDAnalyzer.h>
#include <FWCore/Framework/interface/stream/EDAnalyzer.h>
Copy link
Owner Author

Choose a reason for hiding this comment

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

EDAnalyzers are typically global or one since they usually need to see all events.

Choose a reason for hiding this comment

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

Changed to a one module

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();
Copy link
Owner Author

Choose a reason for hiding this comment

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

If you have to remove the override keyword this means that the base class does not have the function and therefore the CMSSW framework does never call these functions for this module type. beginJob() exists for one and global and should have the override keyword for those types.

Choose a reason for hiding this comment

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

Placed override back in.

@@ -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>
Copy link
Owner Author

Choose a reason for hiding this comment

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

EDAnalyzers are typically global or one since they usually need to see all events.

Choose a reason for hiding this comment

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

When trying to change to global, encounter the same issue as arrived at here:

atishelmanch@f383597#r61033990

Copy link
Owner Author

Choose a reason for hiding this comment

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

If global is not possible you can make it a one module.

Choose a reason for hiding this comment

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

Indeed - changing to one.

@@ -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() ;
Copy link
Owner Author

Choose a reason for hiding this comment

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

Same as above.

Choose a reason for hiding this comment

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

Placed back in

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&);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Why was the override keyword removed?

Choose a reason for hiding this comment

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

Not sure - placed back in.

@@ -7,7 +7,7 @@
//
// Description:

#include "FWCore/Framework/interface/EDProducer.h"
#include "FWCore/Framework/interface/one/EDProducer.h"
Copy link
Owner Author

Choose a reason for hiding this comment

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

Is there a specific need that this is a one module? Could it be stream instead?

Choose a reason for hiding this comment

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

No specific need - I've just updated to stream which required an adjustment of the ECALpedestalPCLworker class defined in Calibration/EcalCalibAlgos/interface/ECALpedestalPCLworker.h (not sure why)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I do not understand it neither.

Copy link
Owner Author

Choose a reason for hiding this comment

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

So in order to make this a stream module it seems needed to remove this .h file and the corresponding DEFINE_FWK_MODULE from the SealModule.cc. The it compiles. There seems to be some strange interference between this and ECALpedestalPCLworker.

Choose a reason for hiding this comment

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

Indeed it compiles following that suggestion. Interesting problem.

@@ -31,32 +29,6 @@
#include <stdexcept>
#include <vector>
Copy link
Owner Author

Choose a reason for hiding this comment

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

I see that this still uses getByLabel() to get collections from the event. This is deprecated as well and should be changed to the getByToken() method.
There is also a new TH1F created that is never deleted.

Choose a reason for hiding this comment

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

There is also a new TH1F created that is never deleted.

Now deleted

I see that this still uses getByLabel() to get collections from the event. This is deprecated as well and should be changed to the getByToken() method.

Ok - for this next commit I haven't updated it yet but will try to.

Choose a reason for hiding this comment

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

Ok - I've taken a stab at this and it compiles. Not 100% sure if it's done correctly.

public:
explicit EcalPedestalHistory(const edm::ParameterSet&);
~EcalPedestalHistory() override;
void analyze(const edm::Event&, const edm::EventSetup&) override;
void beginRun(edm::Run const&, edm::EventSetup const&) override;
void beginRun(edm::Run const&, edm::EventSetup const&); // no need to override beginRun when inheriting from one::EDAnalyzer
Copy link
Owner Author

Choose a reason for hiding this comment

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

As mentioned above, the compile error that you probably got about the override keyword not overriding is there to protect you. edm::one::EDAnalyzer<> does not have a beginRun() by default. If something needs to be done at every beginning of a run the you can make it a edm::one::EDAnalyzer<edm::one::WatchRuns>, which has beginRun() and also endRun() and should have the override keyword.

Choose a reason for hiding this comment

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

If something needs to be done at every beginning of a run

This module clearly has some functionality in the beginRun() method, but I don't understand why we would want to switch to the edm::one::EDAnalyzer<edm::one::WatchRuns> module rather than just use the beginRun() definition included in this class.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Again the same reason as for the other ones. The base class of edm::one::EDAnalyzer<> does not have a beginRun() and if you create beginRun() in this class then the framework does not know about it and will never call it when it calls the beginRun() functions. It just happens that you have a function in EcalPedestalHistory that is called beginRun() as well but for the framework this does not matter since it is not in the base class. edm::one::EDAnalyzer<edm::one::WatchRuns> on the other hand does introduce a beginRun() and if your EcalPedestalHistory inherits from it the framework will call its beingRun().

The legacy modules all had beginRun(), beginLuminositySection(), beginJob(), etc. defined all the time but the thread safe ones do not.

Copy link
Owner Author

Choose a reason for hiding this comment

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

So if you use edm::one::EDAnalyzer<edm::one::WatchRuns> you should be able to leave the beginRun() here as it was for the legacy module (including the override).

Choose a reason for hiding this comment

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

Ok I see - so it's a matter of the "framework" knowing about the method, and this is only true if it comes from the new thread-safe classes which we are inheriting from.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes. The framework (cmsRun) runs only the functions (produce, analyze, beginRun, endJob, etc.) of modules that it knows about. But not all modules have all functions (E.g. an EDProducer has no analyze). So the framework run the functions that are available in the base class of a module. The thread-safe module types also to not have all functions but can obtain some by template modifiers (like adding beginRun with the edm::one::WatchRuns template modifier).

@atishelmanch
Copy link

@thomreis I've just added a commit responding to all of your comments except the update of getByLabel to getByToken [1]. What do you think?

[1] #33 (comment)

Copy link
Owner Author

@thomreis thomreis left a comment

Choose a reason for hiding this comment

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

Already pretty good. A couple of things need to be addressed still.

@@ -123,7 +123,7 @@ class LaserSorter : public edm::EDAnalyzer {
void analyze(const edm::Event&, const edm::EventSetup&) override;
void endJob() override;
void beginJob() override;
void beginRun(edm::Run const&, edm::EventSetup const&) override;
void beginRun(edm::Run const&, edm::EventSetup const&);
Copy link
Owner Author

Choose a reason for hiding this comment

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

As I just explained in the other comment you need to make a edm::one::EDAnalyzer<edm::one::WatchRuns> if you want to use beginRun().

Choose a reason for hiding this comment

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

Implemented

public:
explicit EcalStatusAnalyzer(const edm::ParameterSet& iConfig);
~EcalStatusAnalyzer() override;

void analyze(const edm::Event& e, const edm::EventSetup& c) override;
void analyze(const edm::Event& e, const edm::EventSetup& c) ;
Copy link
Owner Author

Choose a reason for hiding this comment

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

This should keep the override as far as I can tell.

Choose a reason for hiding this comment

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

agreed

@@ -34,7 +34,7 @@

class ECALpedestalPCLworker : public DQMEDAnalyzer {
public:
explicit ECALpedestalPCLworker(const edm::ParameterSet &);
explicit ECALpedestalPCLworker(const edm::ParameterSet &, const DQMEDAnalyzerGlobalCache*);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Why does this need const DQMEDAnalyzerGlobalCache*? I do not see it being used.

Choose a reason for hiding this comment

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

If I don't include it, I receive the following error, seemingly related to changing the base class of ElectronRecalibSuperClusterAssociator :

>> Building  shared library tmp/slc7_amd64_gcc900/src/Calibration/EcalCalibAlgos/src/CalibrationEcalCalibAlgos/libCalibrationEcalCalibAlgos.so
In file included from /afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/ProducingModuleAdaptor.h:32,
                 from /afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/EDProducerAdaptor.h:25,
                 from /afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/EDProducerBase.h:27,
                 from /afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/implementors.h:31,
                 from /afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/AbilityToImplementor.h:25,
                 from /afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/EDProducer.h:22,
                 from /afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/Calibration/EcalCalibAlgos/interface/ElectronRecalibSuperClusterAssociator.h:10,
                 from /afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/Calibration/EcalCalibAlgos/plugins/SealModule.cc:6:
/afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/makeGlobal.h: In instantiation of 'T* edm::stream::impl::makeStreamModule(const edm::ParameterSet&, const G*) [with T = ECALpedestalPCLworker; G = DQMEDAnalyzerGlobalCache]':
/afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/ProducingModuleAdaptor.h:96:47:   required from 'void edm::stream::ProducingModuleAdaptor<T, M, B>::setupStreamModules() [with T = ECALpedestalPCLworker; M = edm::stream::EDProducerBase; B = edm::stream::EDProducerAdaptorBase]'
/afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/ProducingModuleAdaptor.h:94:12:   required from here
/afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/FWCore/Framework/interface/stream/makeGlobal.h:40:16: error: no matching function for call to 'ECALpedestalPCLworker::ECALpedestalPCLworker(const edm::ParameterSet&, const DQMEDAnalyzerGlobalCache*&)'
   40 |         return new T(iPSet, iGlobal);
      |                ^~~~~~~~~~~~~~~~~~~~~
In file included from /afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/Calibration/EcalCalibAlgos/plugins/SealModule.cc:9:
/afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/Calibration/EcalCalibAlgos/interface/ECALpedestalPCLworker.h:37:12: note: candidate: 'ECALpedestalPCLworker::ECALpedestalPCLworker(const edm::ParameterSet&)'
   37 |   explicit ECALpedestalPCLworker(const edm::ParameterSet &);
      |            ^~~~~~~~~~~~~~~~~~~~~
/afs/cern.ch/work/a/atishelm/private/ECAL_AlCaDB_Issue/CMSSW_12_2_0_pre2/src/Calibration/EcalCalibAlgos/interface/ECALpedestalPCLworker.h:37:12: note:   candidate expects 1 argument, 2 provided
gmake: *** [config/SCRAM/GMake/Makefile.rules:1700: tmp/slc7_amd64_gcc900/src/Calibration/EcalCalibAlgos/plugins/CalibrationEcalCalibAlgosAuto/SealModule.cc.o] Error 1

Copy link
Owner Author

Choose a reason for hiding this comment

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

That does not make sense since this is the only line that has been changed in the .h and the .cc files. And in the official CMSSW version it compiles just fine. The compile error also states that ECALpedestalPCLworker::ECALpedestalPCLworker(const edm::ParameterSet&) is a candidate.

@@ -87,6 +59,7 @@ void miscalibExample::endJob() {

scEnergy->Write();
f.Close();
scEnergy->~TH1();
Copy link
Owner Author

Choose a reason for hiding this comment

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

Why not simply delete scEnergy;?

Copy link

@atishelmanch atishelmanch Nov 30, 2021

Choose a reason for hiding this comment

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

No particular reason. What's the difference between deleting the pointer and calling the destructor?

(making the change for now)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I am not sure if calling the destructor actually also deallocates the memory.
Delete on a class object calls the destructor and then deallocates the memory.

public:
explicit EcalPedestalHistory(const edm::ParameterSet&);
~EcalPedestalHistory() override;
void analyze(const edm::Event&, const edm::EventSetup&) override;
void beginRun(edm::Run const&, edm::EventSetup const&) override;
void beginRun(edm::Run const&, edm::EventSetup const&);
Copy link
Owner Author

Choose a reason for hiding this comment

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

You need to make a edm::one::EDAnalyzer<edm::one::WatchRuns> if you want to use beginRun(). And keep override then.

Choose a reason for hiding this comment

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

updated

@atishelmanch
Copy link

atishelmanch commented Nov 30, 2021

@thomreis - Ok, I think now everything is addressed (including an attempt at the getByToken method [1]), except for the changing of ElectronRecalibSuperClusterAssociator to stream which produces the error with ECALpedestalPCLworker. For the moment I've switched ElectronRecalibSuperClusterAssociator back to a one type and the error goes away. Any other suggestions for that? Thanks.

[1] #33 (comment)

//========================================================================

cout << "Entering endRun" << endl;
cout << "Exiting endRun" << endl;
Copy link
Owner Author

Choose a reason for hiding this comment

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

I do not think that these outputs are needed. {} should be fine here.

Copy link

@atishelmanch atishelmanch Nov 30, 2021

Choose a reason for hiding this comment

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

agreed - I've also removed similar cout's in other member functions, e.g.:

void EcalPedestalHistory::endJob() {
  //========================================================================

  cout << "Entering endJob" << endl;
  cout << "Exiting endJob" << endl;
}  //endJob

@@ -10,7 +10,7 @@
#include "Calibration/EcalCalibAlgos/interface/ECALpedestalPCLHarvester.h"

DEFINE_FWK_MODULE(miscalibExample);
//DEFINE_FWK_MODULE(ElectronRecalibSuperClusterAssociator);
DEFINE_FWK_MODULE(ElectronRecalibSuperClusterAssociator);
Copy link
Owner Author

Choose a reason for hiding this comment

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

If you uncomment this one you should remove the line at the bottom of Calibration/EcalCalibAlgos/src/ElectronRecalibSuperClusterAssociator.cc to not have it duplicated (This might actually be the reason why you had compile errors before).

Choose a reason for hiding this comment

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

Updated following the above suggestion: #33 (comment)

@@ -48,6 +51,10 @@ class miscalibExample : public edm::EDAnalyzer {
std::string ecalHitsProducer_;
int read_events;

edm::EDGetTokenT<edm::View<reco::SuperClusterCollection> > correctedHybridSuperClusterToken_;
Copy link
Owner Author

Choose a reason for hiding this comment

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

Shouldn't this be edm::EDGetTokenT<reco::SuperClusterCollection> correctedHybridSuperClusterToken_;? Why the edm::View?

Choose a reason for hiding this comment

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

No particular reason - got it from an example. Switching to reco::SuperClusterCollection

@@ -48,6 +51,10 @@ class miscalibExample : public edm::EDAnalyzer {
std::string ecalHitsProducer_;
Copy link
Owner Author

Choose a reason for hiding this comment

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

BarrelHitsCollection_, and ecalHitsProducer_ seem to be unused and can be removed.

Choose a reason for hiding this comment

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

agreed

miscalibExample::miscalibExample(const edm::ParameterSet& iConfig) {
rootfile_ = iConfig.getUntrackedParameter<std::string>("rootfile", "ecalSimpleTBanalysis.root");
correctedHybridSuperClusterProducer_ = iConfig.getParameter<std::string>("correctedHybridSuperClusterProducer");
correctedHybridSuperClusterCollection_ = iConfig.getParameter<std::string>("correctedHybridSuperClusterCollection");
edm::InputTag correctedHybridSuperClusterTag_ = iConfig.getParameter<edm::InputTag>("correctedHybridSuperClusterCollection");
Copy link
Owner Author

Choose a reason for hiding this comment

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

correctedHybridSuperClusterCollection is a string not an InputTag in the configuration.
So this should rather be:

correctedHybridSuperClusterCollection_ = iConfig.getParameter<std::string>("correctedHybridSuperClusterCollection");
correctedHybridSuperClusterToken_ = consumes<reco::SuperClusterCollection>(edm::InputTag(correctedHybridSuperClusterProducer_, correctedHybridSuperClusterTag_);

You could set everything in this constructor also in the initializer list of the constructor (and make the class members const).

Choose a reason for hiding this comment

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

In this version of the file I removed the declaration of the correctedHybridSuperClusterCollection variable altogether because I wasn't sure how it was used aside from being an argument in the getByLabel() method. Should it still be there?

Choose a reason for hiding this comment

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

Now I see your point - updated

Copy link
Owner Author

Choose a reason for hiding this comment

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

This line can actually be removed now. correctedHybridSuperClusterTag_ is not used anymore.

@@ -7,7 +7,7 @@
//
// Description:

#include "FWCore/Framework/interface/EDProducer.h"
#include "FWCore/Framework/interface/one/EDProducer.h"
Copy link
Owner Author

Choose a reason for hiding this comment

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

I do not understand it neither.

@atishelmanch
Copy link

Just about done. Only outstanding issue is that out of the box could not set miscalibExample member variables const

@thomreis
Copy link
Owner Author

thomreis commented Dec 1, 2021

To set the miscalibExample variables const you need to initialise them with the initializer list of the class constructor and not within the constructor body.
The rest looks fine now with one variable left to be removed.
Also make sure to run scram build code-checks and scram build code-format before to do a PR.

int read_events;

edm::EDGetTokenT<reco::SuperClusterCollection> correctedHybridSuperClusterToken_;
Copy link
Owner Author

Choose a reason for hiding this comment

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

This one could be const as well.

Choose a reason for hiding this comment

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

agreed

@thomreis thomreis merged commit f0646ac into thomreis:master Jan 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants