Understand and fix efficiency drop in track reco including UT, following the merge of "Update UT Channel ID and geometry"

added UT label

FYI @tskwarni , @ganowak , @hawu, @gbassi , from the phi distribution in @msaur slides it seems that bins between -90, and +90 are suppressed for Down tracks, it is as if the whole A(or C)-side is missing, no? this means that the drop of eff. is it coming from some mis-modeling in Detector/ project. But I could be missing some connection in Rec/ ?

I think you are right, below you find the efficiency for downsream tracks versus phi (don't be mislead by the title, this is divided by reconstructible particles). It clearly shows zero efficiency between -90 and 90 degrees and 8-10% efficiency everywhere else. 8-10% efficiency is what we had before the change on average.

If I have the quadrants right, we are missing the C-side.

@dovombru many thanks for the nice check. To go a step further: Allen is using Detector/ AFAIK, Allen nightlies tests are not experiencing such a huge drop in efficiency, which means that we can eliminate the Detector/ project as a culprit, do you agree?

The UT geometry in Allen is obtained from the dumper: https://gitlab.cern.ch/lhcb/Allen/-/blob/master/Dumpers/BinaryDumpers/src/DumpUTGeometry.cpp

But since the whole decoding is implemented in different ways, the missing C-side could come from many places I think.

assigned to @bkhanji

Some observations and ideas.

For the HLT2 downstream efficiencies (from here: https://lhcb-nightlies.web.cern.ch/nightly/lhcb-master-mr/7794/Moore/x86_64_v3-centos7-gcc11+detdesc-opt+g/tests)

reco_light_pr_kf: 4591 from     5708 [ 80.43 %] vs 4588 from     5708 [ 80.38 %]
baseline        : 4591 from     5708 [ 80.43 %] vs 4588 from     5708 [ 80.38 %]
fastest         :  542 from     5708 [  9.50 %] vs  242 from     5708 [  4.24 %]

So for downstream only the sequence which actually removes hits before running the downstream seems really affected.

This does not really explain the drop in VeloUT, but PrLongLivedTracking and VeloUT do a different lookup of the sectors for the hits:

PrLongLivedTracking uses https://gitlab.cern.ch/lhcb/LHCb/-/blob/master/UT/UTDAQ/include/UTDAQ/UTDAQHelper.h#L114

PrVeloUT uses essentially this (and the next) lookup table: https://gitlab.cern.ch/lhcb/LHCb/-/blob/master/UT/UTDAQ/src/Lib/UTDAQHelper.cpp#L102

One check could be to test the consistency of the two.

Another check could be to replace this line https://gitlab.cern.ch/lhcb/Rec/-/blob/master/Pr/PrVeloUT/src/PrVeloUT.cpp#L844 with

for ( int i = 0; i < myHits.size(); i += simd::size ) {

and make sure here https://gitlab.cern.ch/lhcb/Rec/-/blob/master/Pr/PrVeloUT/src/PrVeloUT.cpp#L820 that the loop has only 1 iteration.

This makes it loop over all hits in the UT and should be insensitive to the way sensors are looked up.

Further observations: It apparently depends on the sample one runs over. For hlt2_light_reco_pr_kf_with_mcchecking the Upstream tracking is essentially unchanged. For hlt1_reco_baseline_with_mcchecking there is a big drop in efficiency. Running hlt1_reco_baseline_with_mcchecking on the sample of hlt2_light_reco_pr_kf_with_mcchecking gives the same efficiency as with the original hlt2_light_reco_pr_kf_with_mcchecking test.

I made a couple plots for Upstream tracking:

"MiniBrunel" corresponds to the file which is used in the hlt1_reco_baseline_with_mcchecking qmtest, the full identifier is MiniBrunel_2018_MinBias_FTv4_DIGI. It shows the same holes we saw about 4 years ago, which came from a swap in the numbering scheme of the UT
DC19 corresponds to the file which is used in the hlt2_light_reco_pr_kf_with_mcchecking, the full identifier is upgrade_DC19_01_MinBiasMD. It does not show holes.
Sim10aU1 correspond to the data challenge file DC_Sim10aU1_MinBias.py. It does not show holes either.

For Downstream: running the hlt2_fastest_reco I get only one half of the UT (first picture), running hlt2_light_reco_pr_kf I get the full UT (the input sample is the same).

The problem is here: https://gitlab.cern.ch/lhcb/Rec/-/blob/master/Pr/PrAlgorithms/src/PrResidualPrUTHits.cpp#L79

if I replace

const int fullChanIdx =
      static_cast<int>( UTInfo::DetectorNumbers::Layers ) * static_cast<int>( UTInfo::DetectorNumbers::Stations ) *
      static_cast<int>( UTInfo::DetectorNumbers::Regions ) * static_cast<int>( UTInfo::DetectorNumbers::Sectors );

with

  const int fullChanIdx =
 static_cast<int>( UTInfo::DetectorNumbers::Sides )*static_cast<int>( UTInfo::DetectorNumbers::HalfLayers )*
    static_cast<int>( UTInfo::DetectorNumbers::Staves )*static_cast<int>( UTInfo::DetectorNumbers::Faces )*
    static_cast<int>( UTInfo::DetectorNumbers::Modules )*static_cast<int>( UTInfo::DetectorNumbers::SubSectors );

I get the other side back.

Many thanks @decianm this is great news!. Do I understand correctly that after the changes Rec!3413 (merged), the Upstream tracks behavior also returns to normal? i.e. the strange behavior in both Down and Upstream tracks are related to the same bug? One other thing: when removing the:

const int fullChanIdx =
      static_cast<int>( UTInfo::DetectorNumbers::Layers ) * static_cast<int>( UTInfo::DetectorNumbers::Stations ) *
      static_cast<int>( UTInfo::DetectorNumbers::Regions ) * static_cast<int>( UTInfo::DetectorNumbers::Sectors );

The backward compatibility is still guaranteed?

Rec!3413 (merged) is only supposed to fix the lower efficiency for the Downstream in the fastest sequence (i.e. the missing half of the UT).

Upstream is most likely a different issue (as discussed yesterday, it very much looks like the return of the sensor swap) and will need a different fix.

The tests should tell us about backward compatibility, but the test I did was on a rather old sample, and if I understood correctly the new numbering scheme is valid independently of the geometry (but we'll see).

Yes, sorry! my wishful thinking dragged me away from the root of the upstream issue which you explained yesterday to me(FYI for others: issue is explained in !2074 (merged) and first two slides here: https://indico.cern.ch/event/848770/contributions/3566445/attachments/1908754/3153278/LHCb-UT-soft-Sep16-2019.pdf )

The problem with the holes is actually slightly different than I assumed in the beginning. The lookup table, which was used before, which actually still switches between the "old" and the "new" sector numbering scheme is still called here, but then not used, as this was replaced here by a calculation of the sector number here.

In principle one could hack the switch into the calculation, but I am not yet convinced that this rather lengthy calculation is the best one can do, so it needs some

@decianm @bkhanji what is the status here? Is the problem with the holes still present?

No that was solved long ago by @decianm for both upstream and Downstream

To be correct, the fix for certain samples was only merged yesterday: !4135 (merged)

Indeed, with that MR hlt1_reco_baseline_with_mcchecking has more reasonable efficiencies for upstream tracks as well :) I will close this issue.

mentioned in merge request Rec!3413 (merged)

running the HLT2 light reco sequence on the x86_64_v2-centos7-gcc12+detdesc-dbg+alubsan platform yields

/cvmfs/lhcb.cern.ch/lib/lcg/releases/gcc/12.1.0-57c96/x86_64-centos7/include/c++/12.1.0/array:61:36: runtime error: index 3 out of bounds for type 'array [2]'
    #0 0x7f4769c5654b in std::__array_traits<std::array<std::array<std::array<std::array<float, 2ul>, 8ul>, 2ul>, 9ul>, 2ul>::_S_ref(std::array<std::array<std::array<std::array<float, 2ul>, 8ul>, 2ul>, 9ul> const (&) [2], unsigned long) /cvmfs/lhcb.cern.ch/lib/lcg/releases/gcc/12.1.0-57c96/x86_64-centos7/include/c++/12.1.0/array:61
    #1 0x7f4769c5654b in std::array<std::array<std::array<std::array<std::array<float, 2ul>, 8ul>, 2ul>, 9ul>, 2ul>::operator[](unsigned long) /cvmfs/lhcb.cern.ch/lib/lcg/releases/gcc/12.1.0-57c96/x86_64-centos7/include/c++/12.1.0/array:209
    #2 0x7f4769c5654b in LHCb::UTDAQ::GeomCache::GeomCache(DeUTDetector const&) ../UT/UTDAQ/src/Lib/UTDAQHelper.cpp:216

the relevant code is

GeomCache::GeomCache( const DeUTDetector& utDet ) {
    for ( int iLayer = 0; iLayer < static_cast<int>( UTInfo::DetectorNumbers::HalfLayers ); ++iLayer ) {
      // get layer info
      unsigned int layerIndex = iLayer;
      // auto         tlayer     = utDet.getLayerGeom( layerIndex );
      // Second pass
      for ( const auto& sector : utDet.sectors( DeUTDetector::LayerID_t{layerIndex} ) ) {
        auto center = sector->toGlobal( Gaudi::XYZPoint{0, 0, 0} );
        sectorsZ[sector->elementID().side()][iLayer][sector->elementID().stave()][sector->elementID().face()]
                [sector->elementID().module()][sector->elementID().sector()] = center.z();
      }
      layers[layerIndex] = utDet.getLayerGeom( layerIndex );
    }

i.e. one of above functions returns an index 3 but a maximum of 1 is allowed

@bkhanji @xuyuan the iLayer index goes up to 3 i.e. HalfLayers while the corresponding array expects Layers i.e. up to 1. Please fix

or drop, it appears not to be used anywhere

I'll do it

it is used here https://gitlab.cern.ch/lhcb/Rec/-/blob/master/Pr/PrVeloUT/src/PrVeloUT.cpp#L186 no ?

fixed by !4121 (merged), but does not affect any efficiencies.

mentioned in merge request !4121 (merged)

mentioned in issue #309

mentioned in merge request !4135 (merged)

is this now fully addressed?

closed

Understand and fix efficiency drop in track reco including UT, following the merge of "Update UT Channel ID and geometry"

Child items 0

Activity