RPC-validation migration to use ESGetToken #33785
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33785/22756
|
|
A new Pull Request was created by @jhgoh (Junghwan John Goh) for master. It involves the following packages: Validation/MuonRPCDigis @andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| @@ -31,6 +29,9 @@ RPCRecHitValid::RPCRecHitValid(const edm::ParameterSet &pset) { | |||
| muonToken_ = consumes<reco::MuonCollection>(pset.getParameter<edm::InputTag>("muon")); | |||
|
|
|||
| subDir_ = pset.getParameter<std::string>("subDir"); | |||
|
|
|||
| rpcGeomToken_ = esConsumes(); | |||
| rpcGeomTokenInRun_ = esConsumes<edm::Transition::BeginRun>(); | |||
There was a problem hiding this comment.
I am not sure why you need here a Transition since bookHistograms are not a real beginRun, though MEs are booked at the beginning, I guess it does not harm
There was a problem hiding this comment.
@makortel since bookHistograms is not a genuine beginRun function, could this type of lines (or its omision) introduce differences in MEs when the code is run in the multi-thread case? Thanks
There was a problem hiding this comment.
bookHistograms() is called from beginRun() so framework sees it as code run at beginRun and therefore BeginRun transition is correct. Framework throws an exception if the token is used in a different transition than the consumes was declared for.
There was a problem hiding this comment.
And the other way around? I mean, if BeginRun transition is set (but not needed) instead of a simple esConsumes() is set, would it harm? Harm = likely to produce wrong results?
There was a problem hiding this comment.
I'm not sure I understand what you mean with "if BeginRun transition is set but not needed". The default transition for esConsumes is Event transition (i.e. in analyze()/produce()/filter()). Using a token declared for BeginRun transition at Event transitions (or vice versa) leads to an exception.
| @@ -355,8 +355,7 @@ void RPCRecHitValid::analyze(const edm::Event &event, const edm::EventSetup &eve | |||
| h_eventCount->Fill(1); | |||
|
|
|||
| // Get the RPC Geometry | |||
| edm::ESHandle<RPCGeometry> rpcGeom; | |||
| eventSetup.get<MuonGeometryRecord>().get(rpcGeom); | |||
| auto rpcGeom = eventSetup.getHandle(rpcGeomToken_); | |||
There was a problem hiding this comment.
I am not sure why two exact geometries are needed in the class, couldn't we recycle them?
There was a problem hiding this comment.
no problem to make a member variable for a cache, to keep the pointer to the geometry taken at the bookHistogram().
Actually, from the sentence in the twiki page, I assumed values are taken from a cache, probably I misunderstand.
Behind the scenes, the EventSetup system only runs the algorithm used to create the data in a Record the first time that data is requested after a Record has changed its IOV. All subsequent requests will get the cached value.
A fix is made in my local branch and small test is ongoing.
There was a problem hiding this comment.
EventSetup system indeed caches the products, and the overhead of getting it again for each event is negligible. Getting the products directly from EventSetup on each necessary transition is the best general pattern as developer does not have to assume anything about the IOVs of the concerned EventSetup Records. (imagine taking a pointer at beginRun for a product that changes at lumi boundaries)
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ddfd9f/15203/summary.html Comparison SummarySummary:
|
|
+1 |
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-pending
pending-signatures
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This PR is for the migration of RPC validation modules to use the esConsumes, as described in the https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideHowToGetDataFromES#In_ED_module
Related issue: #31061
PR validation:
Minimal checks were done with runTheMatrix.py command with WF 1325.0
@andresib @mileva