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.

Sign up for GitHub

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

PPS pixel track reco in bad pots #40683

Merged
merged 8 commits into from
Feb 10, 2023
Merged

PPS pixel track reco in bad pots #40683

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9be2e7e
2 planes reco
fabferro Feb 3, 2023
e30b3b3
cosmetics
fabferro Feb 3, 2023
0b9f563
clang fixes
fabferro Feb 6, 2023
adcf51a
code improvements
fabferro Feb 8, 2023
615648e
code format
fabferro Feb 8, 2023
8bff11d
bug fix in vector index
fabferro Feb 9, 2023
5c69e5b
variable name change
fabferro Feb 9, 2023
b71c4b7
reverting config parameter
fabferro Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion RecoPPS/Local/interface/RPixDetPatternFinder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class RPixDetPatternFinder {
typedef std::vector<PointInPlane> Road;

void setHits(const edm::DetSetVector<CTPPSPixelRecHit> *hitVector) { hitVector_ = hitVector; }
virtual void findPattern(bool isbadpot) = 0;
virtual void findPattern(bool *isbadpot) = 0;
void clear() { patternVector_.clear(); }
std::vector<Road> const &getPatterns() const { return patternVector_; }
void setGeometry(const CTPPSGeometry *geometry) { geometry_ = geometry; }
Expand Down
2 changes: 1 addition & 1 deletion RecoPPS/Local/interface/RPixRoadFinder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class RPixRoadFinder : public RPixDetPatternFinder {
public:
explicit RPixRoadFinder(const edm::ParameterSet &param);
~RPixRoadFinder() override;
void findPattern(bool isbadpot) override;
void findPattern(bool *isbadpot) override;

private:
int verbosity_;
Expand Down
54 changes: 44 additions & 10 deletions RecoPPS/Local/plugins/CTPPSPixelLocalTrackProducer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,30 +170,64 @@ void CTPPSPixelLocalTrackProducer::produce(edm::Event &iEvent, const edm::EventS
geometryWatcher_.check(iSetup);

// get mask
bool isBadPot_45_220 = false;

// bad pot flag vector 0=45-220, 1=45-210, 2=56-210, 3=56-220
bool isBadPotV[4] = {false};

if (!recHits->empty()) {
const auto &mask = iSetup.getData(tokenCTPPSPixelAnalysisMask_);

// Read Mask checking if 45-220-far is masked as bad and needs special treatment
std::map<uint32_t, CTPPSPixelROCAnalysisMask> const &maschera = mask.analysisMask;

bool mask_45_220[6][6] = {{false}};
//bool mask_45_220[6][6] = {{false}};
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to keep this commented out code?


// vector of masked rocs
bool maskV[4][6][6] = {{{false}}};

for (auto const &det : maschera) {
CTPPSPixelDetId detId(det.first);
unsigned int rocNum = (det.first & rocMask) >> rocOffset;
if (rocNum > 5 || detId.plane() > 5)
throw cms::Exception("InvalidRocOrPlaneNumber") << "roc number from mask: " << rocNum;

if (detId.arm() == 0 && detId.station() == 2 && detId.rp() == 3) { // pot 45-220-far
if (det.second.maskedPixels.size() == rocSizeInPixels) { // roc fully masked
mask_45_220[detId.plane()][rocNum] = true;
if (detId.arm() == 0 && detId.station() == 2 && detId.rp() == 3) { // pot 45-220-far
Copy link
Contributor

Choose a reason for hiding this comment

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

if (detId.arm() == 0 && detId.rp() == 3) {
if(detId.station() == 2){
}
else if(detId.station() == 0){
}

if (det.second.maskedPixels.size() == rocSizeInPixels || det.second.fullMask == true) { // roc fully masked
maskV[0][detId.plane()][rocNum] = true;
}
}
if (detId.arm() == 0 && detId.station() == 0 && detId.rp() == 3) { // pot 45-210-far
if (det.second.maskedPixels.size() == rocSizeInPixels || det.second.fullMask == true) { // roc fully masked
maskV[1][detId.plane()][rocNum] = true;
}
}
if (detId.arm() == 1 && detId.station() == 0 && detId.rp() == 3) { // pot 56-210-far
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above, you can use else if and spare some evaluations

if (det.second.maskedPixels.size() == rocSizeInPixels || det.second.fullMask == true) { // roc fully masked
maskV[2][detId.plane()][rocNum] = true;
}
}
if (detId.arm() == 1 && detId.station() == 2 && detId.rp() == 3) { // pot 56-220-far
if (det.second.maskedPixels.size() == rocSizeInPixels || det.second.fullMask == true) { // roc fully masked
maskV[3][detId.plane()][rocNum] = true;
}
}
}

// search for specific pattern that requires special reconstruction (isBadPot)
isBadPot_45_220 = mask_45_220[1][4] && mask_45_220[1][5] && mask_45_220[2][4] && mask_45_220[2][5] &&
mask_45_220[3][4] && mask_45_220[3][5] && mask_45_220[4][4] && mask_45_220[4][5];
// search for specific pattern that requires special reconstruction (use of two plane tracks)

for (unsigned int i = 0; i < 4; i++) {
unsigned int numberOfMaskedPlanes = 0;
for (unsigned int j = 0; j < 6; j++) {
if (maskV[i][j][0] && maskV[i][j][1] && maskV[i][j][4] && maskV[i][j][5])
perrotta marked this conversation as resolved.
Show resolved Hide resolved
numberOfMaskedPlanes++;
}
if (numberOfMaskedPlanes == 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the the number of masked planes is 5 or 6 (which is possible, in principle)? Is it not a "bad pot"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have 5 masked plane the pot is not bad but TOO bad, so you don’t activate the special 2-plane reco and go with standard reco that, as it has always been, does not even try to reconstruct a track (with one plane only wouldn’t make sense).

Copy link
Contributor

Choose a reason for hiding this comment

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

Still, don't you think this should be >= 4 instead here? Where is the "too bad pot" flagged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still, don't you think this should be >= 4 instead here? Where is the "too bad pot" flagged?

@fabferro rethinking at it, it must remain == 4, given the logic in the code.
Please just confirm that if there are more than 4 masked planes a dedicated treatment and/or a protection still exist.
Probably, a more intuitive name for this variable could be isBadButRecoverablePotV, but I understand that as such it is rather ugly: any better suggestion, still rendering the idea that those pots are "bad but not too much to throw them away"?

Copy link
Contributor Author

@fabferro fabferro Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@perrotta If the pot has >4 planes dead/masked the track reconstruction algorithm stops at a certain point because it requires at least 3 hits in 3 different planes to fit something. With only one active plane it will likely stop even before, at the level of pattern recognition.
If you believe that "isBadPot" is a misleading name I can change it in "is2PlanePot".
If you consider it more appropriate I can prepare the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you believe that "isBadPot" is a misleading name I can change it in "is2PlanePot".
If you consider it more appropriate I can prepare the change.

Thank you @fabferro
Yes, I think this can be a less misleading name for the varable, and if you don't mind I'd replace with it

isBadPotV[i] = true; // search for exactly 4 planes fully masked
edm::LogInfo("CTPPSPixelLocalTrackProducer") << " Masked planes in Pot #" << i << " = " << numberOfMaskedPlanes;
if (isBadPotV[i])
edm::LogInfo("CTPPSPixelLocalTrackProducer")
<< " Enabling 2 plane track reconstruction for pot #" << i << " (0=45-220, 1=45-210, 2=56-210, 3=56-220) ";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the latter part of this LogInfo still appropriate? It seems from the code changes that 45_220 aspect of the code is no longer valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This LogInfo tells in which pot the 2-plane-reco has been enables. It looks still appropriate to me.

}
}
std::vector<CTPPSPixelDetId> listOfPotWithHighOccupancyPlanes;
std::map<CTPPSPixelDetId, uint32_t> mapHitPerPot;
Expand All @@ -215,7 +249,7 @@ void CTPPSPixelLocalTrackProducer::produce(edm::Event &iEvent, const edm::EventS
if (maxHitPerPlane_ >= 0 && hitOnPlane > (uint32_t)maxHitPerPlane_) {
if (verbosity_ > 2)
edm::LogInfo("CTPPSPixelLocalTrackProducer")
<< " ---> To many hits in the plane, pot will be excluded from tracking cleared";
<< " ---> Too many hits in the plane, pot will be excluded from tracking cleared";
listOfPotWithHighOccupancyPlanes.push_back(tmpRomanPotId);
}
}
Expand All @@ -240,7 +274,7 @@ void CTPPSPixelLocalTrackProducer::produce(edm::Event &iEvent, const edm::EventS
patternFinder_->clear();
patternFinder_->setHits(&recHitVector);
patternFinder_->setGeometry(&geometry);
patternFinder_->findPattern(isBadPot_45_220);
patternFinder_->findPattern(isBadPotV);
std::vector<RPixDetPatternFinder::Road> patternVector = patternFinder_->getPatterns();

//loop on all the patterns
Expand Down
12 changes: 11 additions & 1 deletion RecoPPS/Local/src/RPixDetClusterizer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ void RPixDetClusterizer::buildClusters(unsigned int detId,
std::map<uint32_t, CTPPSPixelROCAnalysisMask> const &mask = maskera->analysisMask;
std::set<std::pair<unsigned char, unsigned char> > maskedPixels;

bool fullyMaskedRoc[6][6] = {{false}};
CTPPSPixelDetId currentId(detId);

// read and store masked pixels after converting ROC numbering into module numbering
CTPPSPixelIndices pI;
for (auto const &det : mask) {
Expand All @@ -42,6 +45,8 @@ void RPixDetClusterizer::buildClusters(unsigned int detId,
unsigned int rocNum = (det.first & rocMask) >> rocOffset;
if (rocNum > 5)
throw cms::Exception("InvalidRocNumber") << "roc number from mask: " << rocNum;
if (det.second.fullMask)
fullyMaskedRoc[currentId.plane()][rocNum] = true;
for (auto const &paio : det.second.maskedPixels) {
std::pair<unsigned char, unsigned char> pa = paio;
int modCol, modRow;
Expand Down Expand Up @@ -76,7 +81,12 @@ void RPixDetClusterizer::buildClusters(unsigned int detId,

// check if pixel is noisy or dead (i.e. in the mask)
const bool is_in = maskedPixels.find(pixel) != maskedPixels.end();
if (!is_in) {

// check if pixel inside a fully masked roc
int piano = currentId.plane();
int rocId = pI.getROCId(column, row);

if (!is_in && !fullyMaskedRoc[piano][rocId]) {
//calibrate digi and store the new ones
electrons = calibrate(detId, adc, row, column, pcalibrations);
if (electrons < SeedADCThreshold_ * ElectronADCGain_)
Expand Down
4 changes: 2 additions & 2 deletions RecoPPS/Local/src/RPixPlaneCombinatoryTracking.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ void RPixPlaneCombinatoryTracking::findTracks(int run) {
//The loop stops when the number of planes with recorded hits is less than the minimum number of planes required
//or if the track with minimum chiSquare found has a chiSquare higher than the maximum required

// bad Pot patch 45-220-fr 2022 -- beginning
// bad Pot patch -- beginning
// check number of hits in road
unsigned int hitNum = 0;
for (const auto &plane : *hitMap_) {
Expand Down Expand Up @@ -213,7 +213,7 @@ void RPixPlaneCombinatoryTracking::findTracks(int run) {
// save track in collection
localTrackVector_.push_back(track);
}
// bad Pot patch 45-220-fr 2022 -- end
// bad Pot patch -- end

while (hitMap_->size() >= trackMinNumberOfPoints_) {
if (verbosity_ >= 1)
Expand Down
63 changes: 37 additions & 26 deletions RecoPPS/Local/src/RPixRoadFinder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,13 @@ RPixRoadFinder::~RPixRoadFinder() {}

//------------------------------------------------------------------------------------------------//

void RPixRoadFinder::findPattern(bool isBadPot) {
void RPixRoadFinder::findPattern(bool* isBadPot) {
Road temp_all_hits;
temp_all_hits.clear();

Road temp_all_hits_badPot;
temp_all_hits_badPot.clear();
Road temp_all_hits_badPot[4];
Copy link
Contributor

Choose a reason for hiding this comment

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

The objects really have to be cleared just after creation? Seems surprising.

for (unsigned int i = 0; i < 4; i++)
temp_all_hits_badPot[i].clear();

// convert local hit sto global and push them to a vector
for (const auto& ds_rh2 : *hitVector_) {
Expand Down Expand Up @@ -69,14 +70,19 @@ void RPixRoadFinder::findPattern(bool isBadPot) {

math::Error<3>::type globalError = ROOT::Math::SimilarityT(theRotationTMatrix, localError);

// create new collection for planes 0 and 5 of pot 45-220-fr
// create new collections for bad and good pots

if (isBadPot == true && myid.arm() == 0 && myid.station() == 2 && localV.x() > 0 &&
(myid.plane() == 0 || myid.plane() == 5)) { // 45-220-far

temp_all_hits_badPot.emplace_back(PointInPlane{globalV, globalError, it_rh, myid});
if (isBadPot[0] == true && myid.arm() == 0 && myid.station() == 2) { // 45-220
temp_all_hits_badPot[0].emplace_back(PointInPlane{globalV, globalError, it_rh, myid});
} else if (isBadPot[1] == true && myid.arm() == 0 && myid.station() == 0) { // 45-210
temp_all_hits_badPot[1].emplace_back(PointInPlane{globalV, globalError, it_rh, myid});
} else if (isBadPot[2] == true && myid.arm() == 1 && myid.station() == 0) { // 56-210
temp_all_hits_badPot[2].emplace_back(PointInPlane{globalV, globalError, it_rh, myid});
} else if (isBadPot[3] == true && myid.arm() == 1 && myid.station() == 2) { // 56-220
temp_all_hits_badPot[2].emplace_back(PointInPlane{globalV, globalError, it_rh, myid});
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
temp_all_hits_badPot[2].emplace_back(PointInPlane{globalV, globalError, it_rh, myid});
temp_all_hits_badPot[3].emplace_back(PointInPlane{globalV, globalError, it_rh, myid});

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! Thanks Andrea for spotting it... Fix committed.

} else {
temp_all_hits.emplace_back(PointInPlane{globalV, globalError, it_rh, myid});
}
temp_all_hits.emplace_back(PointInPlane{globalV, globalError, it_rh, myid});
}
}

Expand Down Expand Up @@ -116,29 +122,34 @@ void RPixRoadFinder::findPattern(bool isBadPot) {
// end of algorithm

// badPot algorithm
Road::iterator it_gh1_bP = temp_all_hits_badPot.begin();
Road::iterator it_gh2_bP;

while (it_gh1_bP != temp_all_hits_badPot.end() && temp_all_hits_badPot.size() >= 2) {
Road temp_road;
for (unsigned int i = 0; i < 4; i++) {
if (isBadPot[i]) {
Road::iterator it_gh1_bP = temp_all_hits_badPot[i].begin();
Road::iterator it_gh2_bP;

it_gh2_bP = it_gh1_bP;
while (it_gh1_bP != temp_all_hits_badPot[i].end() && temp_all_hits_badPot[i].size() >= 2) {
Road temp_road;

const auto currPoint = it_gh1_bP->globalPoint;
it_gh2_bP = it_gh1_bP;

while (it_gh2_bP != temp_all_hits_badPot.end()) {
const auto subtraction = currPoint - it_gh2_bP->globalPoint;
const auto currPoint = it_gh1_bP->globalPoint;

if (subtraction.Rho() < roadRadiusBadPot_) {
temp_road.push_back(*it_gh2_bP);
temp_all_hits_badPot.erase(it_gh2_bP);
} else {
++it_gh2_bP;
}
}
while (it_gh2_bP != temp_all_hits_badPot[i].end()) {
const auto subtraction = currPoint - it_gh2_bP->globalPoint;

if (temp_road.size() == 2) { // look for isolated tracks
patternVector_.push_back(temp_road);
if (subtraction.Rho() < roadRadiusBadPot_) {
temp_road.push_back(*it_gh2_bP);
temp_all_hits_badPot[i].erase(it_gh2_bP);
} else {
++it_gh2_bP;
}
}

if (temp_road.size() == 2) { // look for isolated tracks
patternVector_.push_back(temp_road);
}
}
}
}
}