Skip to content

Dead modules #319

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

Merged
osschar merged 3 commits into from
Jun 22, 2021
Merged

Dead modules #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c7d97d
first version of dead moduless in LayerOfHits
cerati Jun 4, 2021
b4fd2be
fix bugs and include also partially dead modules
cerati Jun 8, 2021
13fdfb7
Handle dead reagion that spans over +/- pi boundary.
osschar Jun 21, 2021
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
7 changes: 7 additions & 0 deletions Hit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,5 +329,12 @@ typedef std::vector<HitOnTrack> HoTVec;

void print(std::string label, const MeasurementState& s);

struct DeadRegion
{
float phi1, phi2, q1, q2;
DeadRegion(float a1, float a2, float b1, float b2) : phi1(a1), phi2(a2), q1(b1), q2(b2) {}
};
typedef std::vector<DeadRegion> DeadVec;

} // end namespace mkfit
#endif
31 changes: 31 additions & 0 deletions mkFit/HitStructures.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ void LayerOfHits::setup_bins(float qmin, float qmax, float dq)
m_fq = m_nq / (qmax - qmin); // qbin = (q_hit - m_qmin) * m_fq;

m_phi_bin_infos.resize(m_nq);
m_phi_bin_deads.resize(m_nq);
}

void LayerOfHits::SetupLayer(const LayerInfo &li)
Expand Down Expand Up @@ -182,6 +183,36 @@ void LayerOfHits::SuckInHits(const HitVec &hitv)

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

void LayerOfHits::SuckInDeads(const DeadVec &deadv)
{
assert (m_nq > 0 && "SetupLayer() was not called.");

empty_q_bins_dead(0, m_nq);

for (const auto& d : deadv) {
int q_bin_1 = GetQBinChecked(d.q1);
int q_bin_2 = GetQBinChecked(d.q2) + 1;
int phi_bin_1 = GetPhiBin(d.phi1);
int phi_bin_2 = GetPhiBin(d.phi2) + 1;
for (int q_bin = q_bin_1; q_bin < q_bin_2; q_bin++) {
if (phi_bin_1 > phi_bin_2) {
for (int pb = phi_bin_1; pb < Config::m_nphi; pb++) {
m_phi_bin_deads[q_bin][pb] = true;
}
for (int pb = 0; pb < phi_bin_2; pb++) {
m_phi_bin_deads[q_bin][pb] = true;
}
}
else
{
for (int pb = phi_bin_1; pb < phi_bin_2; pb++) {
m_phi_bin_deads[q_bin][pb] = true;
}
}
}
}

}

void LayerOfHits::BeginRegistrationOfHits(const HitVec &hitv)
{
Expand Down
29 changes: 29 additions & 0 deletions mkFit/HitStructures.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ typedef std::array<PhiBinInfo_t, Config::m_nphi> vecPhiBinInfo_t;

typedef std::vector<vecPhiBinInfo_t> vecvecPhiBinInfo_t;

typedef std::array<bool, Config::m_nphi> vecPhiBinDead_t;
Copy link
Collaborator

@osschar osschar Jun 17, 2021

Choose a reason for hiding this comment

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

Config::m_nphi = 128, 6.28/128 = .04906

Should we consider trying to increase phi-bins to 256 to reduce false identification of bad hits? IIRC, Slava said there is little speed change if one varies this variable. It was 1024 before Slava's last changes here.

Copy link
Collaborator

@slava77 slava77 Jun 17, 2021

Choose a reason for hiding this comment

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

at a radius of 1.1 m (roughly the last layer, TOB) 128 bins corresponds to 5.4 cm.
TOB modules are 9 cm. So, a phi bin is close to a half of the module or less.
TIB modules are 6 cm (the last TIB radius is 50 cm; a phi bin size at this radius is 2.5 cm).
BPIX is 1.6 cm (the last layer is 16 cm; a bin size is 0.8 cm).

So, unless this technology is going to appropriately pick up fractions of modules we will just be repeating the values.

Copy link
Collaborator Author

@cerati cerati Jun 17, 2021

Choose a reason for hiding this comment

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

Since the overall structure of the bins is already in place, repeating values means just a bool and is thus a very small effect. So it's good that the bin size is smaller than the module size, I was more concerned that we could over-assign invalid hits. To me this means that the dead modules do not require an increase in phi bins, but if we choose to do so that is not going to create problems either

Copy link
Collaborator

@makortel makortel Jun 17, 2021

Choose a reason for hiding this comment

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

repeating values means just a bool and is thus a very small effect

Just to note that std::array<bool, N> takes one byte per element (in case with you were after "bit per element" with "just a bool"). Nevertheless a byte per element could be ok storage-wise (128 bytes per layer if I guess correctly?), and avoids bit shifts (although their real cost in modern hardware is unclear to me).

Copy link
Collaborator Author

@cerati cerati Jun 17, 2021

Choose a reason for hiding this comment

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

yes, I assumed one bit. Thanks for clarifying. In any case it's still a minimal footprint (if we were to create a dedicated bin structure it would necessarily be larger, or even if we were to add many dummy hits as in the first implementation by Allie)

Copy link
Collaborator

@osschar osschar Jun 17, 2021

Choose a reason for hiding this comment

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

This is actually even more pronounced for q-bins. We have both a) too large q bins (at least in some layers), and b) earch an extra q-bin up/down. So we effectively pick up dead regions way too often.

Maybe we could restrict this matching at least in q, unless the track passes relatively close to the edge of the bin.

We should re-check q-bins as well. @mmasciov haven't I tried to stick it on you a couple of months back? :)

Copy link
Collaborator

@osschar osschar Jun 17, 2021

Choose a reason for hiding this comment

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

And it's 128 bytes * N_q_bins. We should switch to bits ... that part of code is heavily L1 limited.


typedef std::vector<vecPhiBinDead_t> vecvecPhiBinDead_t;

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

inline bool sortHitsByPhiMT(const Hit& h1, const Hit& h2)
Expand Down Expand Up @@ -96,6 +100,7 @@ class LayerOfHits
public:
const LayerInfo *m_layer_info = 0;
vecvecPhiBinInfo_t m_phi_bin_infos;
vecvecPhiBinDead_t m_phi_bin_deads;
std::vector<float> m_hit_phis;
std::vector<float> m_hit_qs;

Expand Down Expand Up @@ -196,6 +201,22 @@ class LayerOfHits
}
}

void empty_phi_bins_dead(int q_bin, int phi_bin_1, int phi_bin_2)
{
for (int pb = phi_bin_1; pb < phi_bin_2; ++pb)
{
m_phi_bin_deads[q_bin][pb] = false;
}
}

void empty_q_bins_dead(int q_bin_1, int q_bin_2)
{
for (int qb = q_bin_1; qb < q_bin_2; ++qb)
{
empty_phi_bins_dead(qb, 0, Config::m_nphi);
}
}

public:
LayerOfHits() {}

Expand Down Expand Up @@ -228,6 +249,9 @@ class LayerOfHits
// Get in all hits from given hit-vec
void SuckInHits(const HitVec &hitv);

// Get in all hits from given dead-vec
void SuckInDeads(const DeadVec &deadv);

// Use external hit-vec and only use hits that are passed to me.
void BeginRegistrationOfHits(const HitVec &hitv);
void RegisterHit(int idx);
Expand Down Expand Up @@ -282,6 +306,11 @@ class EventOfHits
*/
}

void SuckInDeads(int layer, const DeadVec &deadv)
{
m_layers_of_hits[layer].SuckInDeads(deadv);
}

LayerOfHits& operator[](int i) { return m_layers_of_hits[i]; }
const LayerOfHits& operator[](int i) const { return m_layers_of_hits[i]; }
};
Expand Down
18 changes: 8 additions & 10 deletions mkFit/MkFinder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,12 @@ void MkFinder::SelectHitIndices(const LayerOfHits &layer_of_hits,
{
const int pb = pi & L.m_phi_mask;

if (L.m_phi_bin_deads[qi][pb] == true)
Copy link
Collaborator

@osschar osschar Jun 17, 2021

Choose a reason for hiding this comment

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

This is checked for ALL phi/q bins within (potentially large?) search windows. This means, if we find a hit, that the missed-hit candidate will be generated as being in-gap -- i.e., without the missing hit penalty.

Should we only set this if no hits are found? Or if the found hit is more than some dphi-sigma away?

Copy link
Collaborator Author

@cerati cerati Jun 17, 2021

Choose a reason for hiding this comment

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

yes, this is conservative. But I think CMSSW does the same. If there is a dead region within the search window you have no guarantee that the track did not pass through it - even if you find a hit it could be an accidental match. This does not mean that we cannot be more creative than CMSSW, but I think this is a reasonable starting point.

{
//std::cout << "dead module for track in layer=" << L.layer_id() << " qb=" << qi << " pb=" << pb << " q=" << q << " phi=" << phi<< std::endl;
XWsrResult[itrack].m_in_gap = true;
}

// MT: The following line is the biggest hog (4% total run time).
// This comes from cache misses, I presume.
// It might make sense to make first loop to extract bin indices
Expand Down Expand Up @@ -610,15 +616,7 @@ void MkFinder::SelectHitIndices(const LayerOfHits &layer_of_hits,
// Avi says we should have *minimal* search windows per layer.
// Also ... if bins are sufficiently small, we do not need the extra
// checks, see above.
if (L.GetHit(hi_orig).mcHitID() == -7)
{
//ARH: This will need a better treatment but works for now
XWsrResult[itrack].m_in_gap = true;
}
else
{
XHitArr.At(itrack, XHitSize[itrack]++, 0) = hi_orig;
}
XHitArr.At(itrack, XHitSize[itrack]++, 0) = hi_orig;
}
else
{
Expand Down Expand Up @@ -1065,7 +1063,7 @@ void MkFinder::FindCandidatesCloneEngine(const LayerOfHits &layer_of_hits, CandC
tmpList.hitIdx = fake_hit_idx;
tmpList.module = -1;
tmpList.nhits = NFoundHits(itrack,0,0);
tmpList.ntailholes= NTailMinusOneHits(itrack,0,0)+1;
tmpList.ntailholes= (fake_hit_idx == -1 ? NTailMinusOneHits(itrack,0,0)+1 : NTailMinusOneHits(itrack,0,0));
tmpList.noverlaps= NOverlapHits(itrack,0,0);
tmpList.nholes = num_inside_minus_one_hits(itrack);
tmpList.pt = std::abs(1.0f / Par[iP].At(itrack,3,0));
Expand Down
7 changes: 7 additions & 0 deletions mkFit/MkStdSeqs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ void LoadHits(Event &ev, EventOfHits &eoh)
});
}

void LoadDeads(EventOfHits &eoh, const std::vector<DeadVec>& deadvectors)
{
for (size_t il = 0; il < deadvectors.size(); il++) {
eoh.SuckInDeads(int(il), deadvectors[il]);
}
}

// Loading hits in CMSSW from two "large multi-layer vectors".
// orig_hitvectors[0] - pixels,
// orig_hitvectors[1] - strips.
Expand Down
2 changes: 2 additions & 0 deletions mkFit/MkStdSeqs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ namespace StdSeq
{
void LoadHits(Event &ev, EventOfHits &eoh);

void LoadDeads(EventOfHits &eoh, const std::vector<DeadVec>& deadvectors);

void Cmssw_LoadHits_Begin(EventOfHits &eoh, const std::vector<const HitVec*> &orig_hitvectors);
void Cmssw_LoadHits_End(EventOfHits &eoh);

Expand Down
Loading