PFlow fix/improvement in the HF region for phase2 #28442
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,10 +52,35 @@ namespace reco { | |
| HOLayer = 8, | ||
| /// VFcal(HF) front face | ||
| VFcalEntrance = 9, | ||
| // Number of valid layers | ||
| NLayers = 10, | ||
| // Unknown | ||
| Unknown = -1 | ||
| }; | ||
|
|
||
| static std::map<std::string, LayerType> create_map() { | ||
| std::map<std::string, LayerType> m; | ||
| m["ClosestApproach"] = LayerType::ClosestApproach; | ||
| m["BeamPipeOrEndVertex"] = LayerType::BeamPipeOrEndVertex; | ||
| m["PS1"] = LayerType::PS1; | ||
| m["PS2"] = LayerType::PS2; | ||
| m["ECALEntrance"] = LayerType::ECALEntrance; | ||
| m["ECALShowerMax"] = LayerType::ECALShowerMax; | ||
| m["HCALEntrance"] = LayerType::HCALEntrance; | ||
| m["HCALExit"] = LayerType::HCALExit; | ||
| m["HOLayer"] = LayerType::HOLayer; | ||
| m["VFcalEntrance"] = LayerType::VFcalEntrance; | ||
| return m; | ||
| } | ||
| static LayerType layerTypeFromString(std::string layerTypeString) { | ||
| std::map<std::string, LayerType> layerTypeMap = create_map(); | ||
|
There was a problem hiding this comment. I think it might be preferable for a few reasons to define the map as a constant within this function: const std::map<std::string, LayerType> layerTypeMap{
{"ClosestApproach", LayerType::ClosestApproach},
{"BeamPipeOrEndVertex", LayerType::BeamPipeOrEndVertex},
{"PS1", LayerType::PS1},
{"PS2", LayerType::PS2},
{"ECALEntrance", LayerType::ECALEntrance},
{"ECALShowerMax", LayerType::ECALShowerMax},
{"HCALEntrance", LayerType::HCALEntrance},
{"HCALExit", LayerType::HCALExit},
{"HOLayer", LayerType::HOLayer},
{"VFcalEntrance", LayerType::VFcalEntrance},
};There was a problem hiding this comment. This looks like a very expensive way to map a name of an enum to the enum (even with the pre-constructed cmssw/DataFormats/TrackReco/src/TrackBase.cc Lines 137 to 142 in 431e2fd for an example) There was a problem hiding this comment. Okay. This will be called only at the beginning of each job 1-3 times, and not for each event, so I don't think it's critical to make it less expensive, but let me think.. There was a problem hiding this comment. If only used in job startup the There was a problem hiding this comment. Thanks. At the end, I just adopted your string to enum conversion method. I agree that it's nicer. |
||
| std::map<std::string, LayerType>::iterator it = layerTypeMap.find(layerTypeString); | ||
| if (it != layerTypeMap.end()) | ||
| return it->second; | ||
| else | ||
| return LayerType::Unknown; | ||
| } | ||
|
|
||
| /// default constructor. Set variables at default dummy values | ||
| PFTrajectoryPoint(); | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,9 @@ class KDTreeLinkerTrackHcal : public KDTreeLinkerBase { | |
| // Here we free all allocated structures. | ||
| void clear() override; | ||
|
|
||
| void setTrajectoryPoints(const reco::PFTrajectoryPoint::LayerType trajectoryLayerEntrance, | ||
| const reco::PFTrajectoryPoint::LayerType trajectoryLayerExit) override; | ||
|
|
||
| private: | ||
| // Data used by the KDTree algorithm : sets of Tracks and HCAL clusters. | ||
| BlockEltSet targetSet_; | ||
|
|
@@ -50,6 +53,11 @@ class KDTreeLinkerTrackHcal : public KDTreeLinkerBase { | |
|
|
||
| // KD trees | ||
| KDTreeLinkerAlgo<reco::PFRecHit const*> tree_; | ||
|
|
||
| // TrajectoryPoints | ||
| reco::PFTrajectoryPoint::LayerType _trajectoryLayerEntrance; | ||
| reco::PFTrajectoryPoint::LayerType _trajectoryLayerExit; | ||
| bool _checkExit; | ||
|
There was a problem hiding this comment. please use a trailing underscore http://cms-sw.github.io/cms_coding_rules.html#2--naming-rules-1 |
||
| }; | ||
|
|
||
| // the text name is different so that we can easily | ||
|
|
@@ -64,11 +72,26 @@ KDTreeLinkerTrackHcal::KDTreeLinkerTrackHcal() : KDTreeLinkerBase() { | |
| KDTreeLinkerTrackHcal::~KDTreeLinkerTrackHcal() { clear(); } | ||
|
|
||
| void KDTreeLinkerTrackHcal::insertTargetElt(reco::PFBlockElement* track) { | ||
| if (track->trackRefPF()->extrapolatedPoint(_trajectoryLayerEntrance).isValid()) { | ||
| targetSet_.insert(track); | ||
| } | ||
| } | ||
|
|
||
| void KDTreeLinkerTrackHcal::setTrajectoryPoints(const reco::PFTrajectoryPoint::LayerType trajectoryLayerEntrance, | ||
| const reco::PFTrajectoryPoint::LayerType trajectoryLayerExit) { | ||
| _trajectoryLayerEntrance = trajectoryLayerEntrance; | ||
| _trajectoryLayerExit = trajectoryLayerExit; | ||
| // make sure the requested setting is supported | ||
| assert((_trajectoryLayerEntrance == reco::PFTrajectoryPoint::HCALEntrance && | ||
| _trajectoryLayerExit == reco::PFTrajectoryPoint::HCALExit) || | ||
| (_trajectoryLayerEntrance == reco::PFTrajectoryPoint::HCALEntrance && | ||
| _trajectoryLayerExit == reco::PFTrajectoryPoint::Unknown) || | ||
| (_trajectoryLayerEntrance == reco::PFTrajectoryPoint::VFcalEntrance && | ||
| _trajectoryLayerExit == reco::PFTrajectoryPoint::Unknown)); | ||
| // flag if exit layer should be checked or not | ||
| _checkExit = (_trajectoryLayerExit == reco::PFTrajectoryPoint::Unknown) ? false : true; | ||
|
There was a problem hiding this comment. this is just |
||
| } | ||
|
|
||
| void KDTreeLinkerTrackHcal::insertFieldClusterElt(reco::PFBlockElement* hcalCluster) { | ||
| const reco::PFClusterRef& clusterref = hcalCluster->clusterRef(); | ||
|
|
||
|
|
@@ -85,7 +108,7 @@ void KDTreeLinkerTrackHcal::insertFieldClusterElt(reco::PFBlockElement* hcalClus | |
| const reco::PFRecHitRef& rh = fraction[rhit].recHitRef(); | ||
| double fract = fraction[rhit].fraction(); | ||
|
|
||
| if ((rh.isNull()) || (fract < cutOffFrac)) | ||
| continue; | ||
|
|
||
| const reco::PFRecHit& rechit = *rh; | ||
|
|
@@ -142,19 +165,24 @@ void KDTreeLinkerTrackHcal::searchLinks() { | |
| for (BlockEltSet::iterator it = targetSet_.begin(); it != targetSet_.end(); it++) { | ||
| reco::PFRecTrackRef trackref = (*it)->trackRefPF(); | ||
|
|
||
| const reco::PFTrajectoryPoint& atHCAL = trackref->extrapolatedPoint(_trajectoryLayerEntrance); | ||
|
|
||
| // The track didn't reach hcal | ||
| if (!atHCAL.isValid()) | ||
| continue; | ||
|
|
||
| // In case the exit point check is requested, check eta and phi differences between entrance and exit | ||
| double dHeta = 0.0; | ||
| float dHphi = 0.0; | ||
| if (_checkExit) { | ||
| const reco::PFTrajectoryPoint& atHCALExit = trackref->extrapolatedPoint(_trajectoryLayerExit); | ||
| dHeta = atHCALExit.positionREP().eta() - atHCAL.positionREP().eta(); | ||
| dHphi = atHCALExit.positionREP().phi() - atHCAL.positionREP().phi(); | ||
| if (dHphi > M_PI) | ||
| dHphi = dHphi - 2. * M_PI; | ||
| else if (dHphi < -M_PI) | ||
| dHphi = dHphi + 2. * M_PI; | ||
| } // _checkExit | ||
|
|
||
| float tracketa = atHCAL.positionREP().eta() + 0.1 * dHeta; | ||
| float trackphi = atHCAL.positionREP().phi() + 0.1 * dHphi; | ||
|
|
@@ -217,7 +245,7 @@ void KDTreeLinkerTrackHcal::updatePFBlockEltWithLinks() { | |
|
|
||
| for (BlockEltSet::iterator jt = it->second.begin(); jt != it->second.end(); ++jt) { | ||
| reco::PFRecTrackRef trackref = (*jt)->trackRefPF(); | ||
| const reco::PFTrajectoryPoint& atHCAL = trackref->extrapolatedPoint(_trajectoryLayerEntrance); | ||
| double tracketa = atHCAL.positionREP().eta(); | ||
| double trackphi = atHCAL.positionREP().phi(); | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
as a future development item, it would be even better to get these directly from the geometry instead of hardcoding the values
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.
although this is a number that is not supposed to change, I agree with @kpedro88 that this implementation is intrinsicattly weak, although its fix can be deferred to a second moment
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 understand. If we make this update, I want to update a few other hardcoded numbers, but some hardcoded numbers may not be directly extractable from geometry. It's not a trivial change, so I prefer to keep this separate, while fixing this known small inconsistency for now.