-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Adapt PF code to read Hcal thresholds from GT #43025
Changes from 4 commits
506c568
6a2fbd9
d258dbb
608b139
2fbc415
84cccf5
9990f0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,6 +11,9 @@ | |||||
#include "CondFormats/HcalObjects/interface/HcalRecoParams.h" | ||||||
#include "CondFormats/DataRecord/interface/HcalRecoParamsRcd.h" | ||||||
|
||||||
#include "CondFormats/HcalObjects/interface/HcalPFCuts.h" | ||||||
#include "CondFormats/DataRecord/interface/HcalPFCutsRcd.h" | ||||||
|
||||||
#include "Geometry/CaloGeometry/interface/CaloGeometry.h" | ||||||
#include "Geometry/Records/interface/CaloGeometryRecord.h" | ||||||
#include "Geometry/CaloTopology/interface/HcalTopology.h" | ||||||
|
@@ -26,6 +29,7 @@ class HcalChannelPropertiesEP : public edm::ESProducer { | |||||
public: | ||||||
typedef std::unique_ptr<HcalRecoParams> ReturnType1; | ||||||
typedef std::unique_ptr<HcalChannelPropertiesVec> ReturnType2; | ||||||
typedef std::unique_ptr<HcalPFCuts> ReturnType3; | ||||||
|
||||||
inline HcalChannelPropertiesEP(const edm::ParameterSet&) { | ||||||
auto cc1 = setWhatProduced(this, &HcalChannelPropertiesEP::produce1); | ||||||
|
@@ -39,13 +43,16 @@ class HcalChannelPropertiesEP : public edm::ESProducer { | |||||
sevToken_ = cc2.consumes(); | ||||||
qualToken_ = cc2.consumes(qTag); | ||||||
geomToken_ = cc2.consumes(); | ||||||
|
||||||
edm::es::Label productLabel("withTopo"); | ||||||
auto cc3 = setWhatProduced(this, &HcalChannelPropertiesEP::produce3, productLabel); | ||||||
topoToken3_ = cc3.consumes(); | ||||||
pfcutsToken_ = cc3.consumes(); | ||||||
} | ||||||
|
||||||
inline ~HcalChannelPropertiesEP() override {} | ||||||
|
||||||
ReturnType1 produce1(const HcalChannelPropertiesAuxRecord& rcd) { | ||||||
using namespace edm; | ||||||
|
||||||
const HcalTopology& htopo = rcd.getRecord<HcalRecNumberingRecord>().get(topoToken_); | ||||||
const HcalRecoParams& params = rcd.getRecord<HcalRecoParamsRcd>().get(paramsToken_); | ||||||
|
||||||
|
@@ -61,8 +68,7 @@ class HcalChannelPropertiesEP : public edm::ESProducer { | |||||
// This means that we are sometimes going to rebuild the | ||||||
// whole table on the lumi block boundaries instead of | ||||||
// just updating the list of bad channels. | ||||||
using namespace edm; | ||||||
|
||||||
// | ||||||
// Retrieve various event setup records and data products | ||||||
const HcalDbRecord& dbRecord = rcd.getRecord<HcalDbRecord>(); | ||||||
const HcalDbService& cond = dbRecord.get(condToken_); | ||||||
|
@@ -118,18 +124,28 @@ class HcalChannelPropertiesEP : public edm::ESProducer { | |||||
return prod; | ||||||
} | ||||||
|
||||||
ReturnType3 produce3(const HcalPFCutsRcd& rcd) { | ||||||
const HcalTopology& htopo = rcd.getRecord<HcalRecNumberingRecord>().get(topoToken3_); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be just
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, thanks. I was just following the standard old recipe which requires look up of dependent records. I presume, @swagata87 can make the requested change. |
||||||
const HcalPFCuts& cuts = rcd.get(pfcutsToken_); | ||||||
|
||||||
ReturnType3 prod = std::make_unique<HcalPFCuts>(cuts); | ||||||
prod->setTopo(&htopo); | ||||||
return prod; | ||||||
} | ||||||
|
||||||
HcalChannelPropertiesEP() = delete; | ||||||
HcalChannelPropertiesEP(const HcalChannelPropertiesEP&) = delete; | ||||||
HcalChannelPropertiesEP& operator=(const HcalChannelPropertiesEP&) = delete; | ||||||
|
||||||
private: | ||||||
edm::ESGetToken<HcalDbService, HcalDbRecord> condToken_; | ||||||
edm::ESGetToken<HcalTopology, HcalRecNumberingRecord> topoToken_; | ||||||
edm::ESGetToken<HcalTopology, HcalRecNumberingRecord> topoToken_, topoToken3_; | ||||||
edm::ESGetToken<HcalRecoParams, HcalRecoParamsRcd> paramsToken_; | ||||||
edm::ESGetToken<HcalSeverityLevelComputer, HcalSeverityLevelComputerRcd> sevToken_; | ||||||
edm::ESGetToken<HcalChannelQuality, HcalChannelQualityRcd> qualToken_; | ||||||
edm::ESGetToken<CaloGeometry, CaloGeometryRecord> geomToken_; | ||||||
edm::ESGetToken<HcalRecoParams, HcalChannelPropertiesAuxRecord> myParamsToken_; | ||||||
edm::ESGetToken<HcalPFCuts, HcalPFCutsRcd> pfcutsToken_; | ||||||
}; | ||||||
|
||||||
DEFINE_FWK_EVENTSETUP_MODULE(HcalChannelPropertiesEP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to keep the phase-2 HLT behind offline at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure I understood the question. For ECAL, this boolean needs to be False as ECAL thresholds are handled differently. In offline config also, for ECAL it is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's confusing to have the same parameter name if different thresholds are applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same code runs for all calorimeters (ECAL, HB, HE, HO, HF). So it is kept as True for HB and HE, where it is needed. And for others it is kept as False.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think changing this behaviour would profit the maintainability of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mean to delay this PR, but just a question for my own understanding. (the question is not specific to Phase 2).
In the case of ECAL (and following the analogy with HCAL), are any of the PFCluster config parameters (e.g.
seedingThreshold
orgatheringThreshold
) related to the values in the GT record of typeEcalPFRecHitThresholdsRcd
? Is theEcalPFRecHitThresholds
tag used in PFlow producers at any point ? (if not, should that happen eventually ?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ECAL, things work a bit differently. In the step where PFRecHits are produced from ECAL RecHits, we use the EcalPFRecHitThresholdsRcd from GT. These are per-crystal noise thresholds, so they are so many in numbers (~75K) that one has to use GT-based approach. Also, they are updated once a year, generally.
Here is the code for that:
cmssw/RecoParticleFlow/PFClusterProducer/interface/PFRecHitQTests.h
Lines 53 to 87 in d59e80b
Then, in the clustering step, ECAL has seeding thresholds and gathering thresholds that are passed from the config file, here:
cmssw/RecoParticleFlow/PFClusterProducer/python/particleFlowClusterECALUncorrected_cfi.py
Lines 51 to 76 in 786ae10
Since these are just one number for barrel and one for endcap, and they don't need update every year, there is no need to put those in GT. One can of course double check with ECAL DPG if they plan to update these numbers in Run3 or if they plan to put them in GT, but I don't think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swagata87 , thanks for the explanation !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the cost of this is pretty negligible.
Anyway I am about to sign this PR, but I still find confusing to have the same parameters configured differently in the phase-2 HLT menu depending on the "flavour". Please consider adding a comment to each instance for better documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay I have now added a comment to every place where it is False in Phase2 HLT configs.