Dead modules #319

cerati · 2021-06-08T22:49:48Z

Account for dead strip modules in mkFit. A bool for each phi-q bin is stores the information about dead modules and is checked in SelectHitIndices. If there is a dead module in the bin, an invalid hit with with idx=-7 is created.

For standalone, the list is currently taken from Run3 release (/data2/slava77/samples/CMSSW_11_2_0-orig/2021/11834.0_TTbar_14TeV+2021/AVE_50_BX01_25ns/step2_sorted.root) and stored in deadmodules.h.
This is probably working only for CloneEngine for now. In Standard, the -7 hits are added but I am not sure they are accounted for properly downstream (e.g. in the score).

For CMSSW integrated version, the list is taken at run time from the GlobalTag (a corresponding PR to trackreco/CMSSW will be made shortly).

Validation plots.
Standalone: https://cerati.web.cern.ch/cerati/dead-modules/
CMSSW: http://uaf-10.t2.ucsd.edu/~cerati/deadmodules/

cerati · 2021-06-09T00:31:39Z

link to cmssw PR: trackreco/cmssw#24

areinsvo · 2021-06-16T18:23:48Z

As far as I can tell, the -7 hits should be properly accounted for in STD as well. I did not test this with specific track friends, but the relevant code is here: https://github.com/trackreco/mkFit/blob/devel/mkFit/HitStructures.h#L608

It's hard to tell if this PR makes the difference between STD and CE worse. I know differences were introduced in the charge flip PR, which I'm revisiting and trying to debug now.

osschar · 2021-06-17T06:59:46Z

mkFit/deadmodules.h

+deadvectors[33].push_back({1.29496,1.3754,88.9013,109.39});
+deadvectors[33].push_back({1.83039,2.09648,32.5881,41.4833});
+deadvectors[33].push_back({-1.34772,-1.23286,50.5025,62.0376});
+deadvectors[33].push_back({3.14024,-3.02807,50.5027,62.0379});


I suspect this will cause trouble.
Should we handle it in producer (create two phi regions on each side of the divide) or in the SuckInDeads()?

what is that will cause troubles?

{3.14024,-3.02807,50.5027,62.0379} ... wrap around pi

This actually needs to be fixed before merge. Giuseppe, I'd propose to do it in the generator, then it will be automatically fixed for cmssw as well, right?
Something like:

if (phi_low > phi_high) { // Need more sanity checks? emit_for(phi_low, Config::PI, ...); emit_for(-Config::PI, phi_high, ....); }

it's not wrapping around pi=3.14159265359. I am not sure this is an issue

{3.14024,-3.02807,...} translates to bin numbers ~127 and ~0, respectively. The fill loop ends up with (i=127; i<0; ++i) :)

I pushed a fix that handles this in LayerOfHits::SuckInDeads(). You decide if you want to keep it like this or correct it in the producer -- probably it depends on how this is handled in CMSSW.

thanks! Yes having it here seems safe.

osschar · 2021-06-17T07:04:53Z

mkFit/mkFit.cc

@@ -341,6 +341,10 @@ void test_standard()

        StdSeq::LoadHits(ev, eoh);

+	std::vector<DeadVec> deadvectors(ev.layerHits_.size());
+#include "deadmodules.h"
+	StdSeq::LoadDeads(eoh, deadvectors);


We could in principle call this just once where EventOfHits objects are created, one for each event of flight.
[ or even share vecvecPhiBinDead_t m_phi_bin_deads via a const ptr ... this has the added benefit that if the ptr is null (not set), we know dead regions are not being used ]

Forgot to say, Slava says pixel dead regions change more frequently so for those we still need a per instance / per event update -- but this probably only applies to CMSSW implementation.

osschar · 2021-06-17T07:20:20Z

mkFit/deadmodules.h

+deadvectors[70].push_back({1.79387,1.90873,72.787,91.2269});
+deadvectors[70].push_back({-2.8063,-2.69143,72.7778,91.2158});
+deadvectors[70].push_back({2.39452,2.47495,88.9094,109.398});
+deadvectors[70].push_back({-1.53249,-1.45203,88.8906,109.379});


Interesting that there are no dead regions for layer 71.

osschar · 2021-06-17T07:30:35Z

mkFit/HitStructures.h

@@ -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;


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.

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.

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

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).

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)

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? :)

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

osschar · 2021-06-17T07:36:44Z

mkFit/MkFinder.cc

@@ -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)


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?

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.

cerati added 2 commits June 4, 2021 11:09

first version of dead moduless in LayerOfHits

8c7d97d

fix bugs and include also partially dead modules

b4fd2be

cerati requested a review from osschar June 8, 2021 22:50

cerati mentioned this pull request Jun 9, 2021

add dead strips from strip quality trackreco/cmssw#24

Closed

osschar reviewed Jun 17, 2021

View reviewed changes

makortel mentioned this pull request Jun 18, 2021

Add dead modules code to MkFitEventOfHitsProducer trackreco/cmssw#26

Merged

Handle dead reagion that spans over +/- pi boundary.

13fdfb7

osschar merged commit 733681e into devel Jun 22, 2021

cerati deleted the dead-modules branch June 23, 2021 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dead modules #319

Dead modules #319

cerati commented Jun 8, 2021

cerati commented Jun 9, 2021

areinsvo commented Jun 16, 2021

osschar Jun 17, 2021

cerati Jun 17, 2021

osschar Jun 17, 2021

osschar Jun 18, 2021

cerati Jun 19, 2021

osschar Jun 21, 2021

osschar Jun 21, 2021

cerati Jun 22, 2021

osschar Jun 17, 2021

osschar Jun 17, 2021

osschar Jun 17, 2021

osschar Jun 17, 2021

slava77 Jun 17, 2021

cerati Jun 17, 2021

makortel Jun 17, 2021

cerati Jun 17, 2021

osschar Jun 17, 2021

osschar Jun 17, 2021

osschar Jun 17, 2021

cerati Jun 17, 2021

		@@ -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;

Dead modules #319

Dead modules #319

Conversation

cerati commented Jun 8, 2021

cerati commented Jun 9, 2021

areinsvo commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment