MuonIDHltAlg - seg fault in dd4hep builds

Seg. fault seen here

https://lhcb-nightlies.web.cern.ch/nightly/lhcb-dd4hep/1281/Moore/x86_64_v2-centos7-gcc11+dd4hep-dbg/tests#RecoConf.hlt2_light_reco_pr_kf_without_UT_Calo_with_mcchecking

#11 MuonIDHltAlg<LHCb::Event::v3::Tracks>::countHits (this=0x33eba500, p=<optimized out>, p
entry=3295.37744, extrapolation=..., hitContainer=..., cond=...) at /cvmfs/lhcb.cern.ch/lib/lcg/releases/gcc/11.1.0-e80bf/x86_64-centos7/include/c++/11.1.0/array:56
#12 0x00007f560ef34986 in MuonIDHltAlg<LHCb::Event::v3::Tracks>::operator()(LHCb::Event::v3::Tracks const&, MuonHitContainer const&, details::Cache const&) const::{lambda(auto:1 const&)#1}::operator()<LHCb::Event::ZipProxy<LHCb::Event::v3::Tracks::FittedProxy<(SIMDWrapper::InstructionSet)2, (LHCb::Pr::ProxyBehaviour)1, LHCb::Event::v3::Tracks const> > >(LHCb::Event::ZipProxy<LHCb::Event::v3::Tracks::FittedProxy<(SIMDWrapper::InstructionSet)2, (LHCb::Pr::ProxyBehaviour)1, LHCb::Event::v3::Tracks const> > const&) const (__closure=__closure
entry=0x7f560b2bf3b0, track=...) at ../Muon/MuonID/src/component/MuonIDHltAlg.cpp:285
#13 0x00007f560ef47afe in MuonIDHltAlg<LHCb::Event::v3::Tracks>::operator() (this=this
entry=0x33eba500, input_tracks=..., hitContainer=..., cond=...) at ../Muon/MuonID/src/component/MuonIDHltAlg.cpp:313

@desimone @farchill @decianm @cmarinbe FYI

added to epic &1

added RTA WP2 dd4hep labels

added Muon label

changed the description

Interestingly only seen in the test with MC checking...

https://lhcb-nightlies.web.cern.ch/nightly/lhcb-dd4hep/1281/Moore/x86_64_v2-centos7-gcc11+dd4hep-dbg/tests#RecoConf.hlt2_light_reco_pr_kf_without_UT_Calo

Is the MuonIDHltAlg component configured differently between the two tasks ??

The reason for this appears to be nothing relating to the MUONS is running in the test without checking. Presumably something is up with the configuration leading to the scheduler not think anything needs the muon reconstructed information, hence its (correctly) skipping it.

@decianm Any idea why this line is commented out

https://gitlab.cern.ch/lhcb/Moore/-/blob/master/Hlt/RecoConf/python/RecoConf/standalone.py#L785

as I believe its the reason the muon sequence is not running in the without mc checking task ?

I remember that some time ago it was excluded because it was not thread-safe, this could be a remnant of it.

Actually, looking at the other standalone sequences, the same feature is present.

Yeah, I was also looking.

I'll test adding it back, at least so far as the sequences initalise etc. If it is indeed not thread safe that something else that will be have to be addressed before it can be used in production though.

It was made thread-safe quite some time ago, so this should not be an issue anymore.

@cmarinbe @decianm can you ping here whomever is responsible for MuonIDHltAlg ?

I think @mfontana and @rvazquez know better.

@satta FYI

@jonrob, I'm working on it with the help of @desimone and @farchill. What I can see is that the pieces of code in LHCb/Muon/MuonDAQ that were extracting the hits using DetDesc need to be changed to DD4HEP so that MuonIDHltAlg can have hits. Also in MuonIDHltAlg there are some uses of DetDesc that need to be adapted. Those should be (at first order) easy changes as the functionality has stayed the same.

OK thanks. I propose then that whilst Detector!199 (merged) is looking close to being ready to merge we keep it open until we have a fix for this, as in any case having the Detector changes in master is nice it is not much practical use until this is addressed, at least in terms of running the Muon in HLT2, and perhaps bug fixes will be needed there.

agreed

mentioned in merge request Detector!199 (merged)

mentioned in merge request LHCb!3800 (closed)

The fix to the seg. fault in the builds is in LHCb!3800 (closed) and !3137 (closed). I have tested locally and all the hlt2_light_reco_pr_kf_without* tests pass.

Unfortunately the fixes did not work. See the comments in !3137 (closed).

assigned to @rvazquez

@rvazquez @farchill I had a sudden realisation this afternoon that the issue here was likely one I had run into with the RICH, namely the fact you shouldn't take the memory address of a dd4hep DetElem and cache it long term. This is because the object you get is just a short term lightweight handle over the real object. Instead, just copy the handle, which is what this !3141 (merged) does. This fixes the seg. fault.

thanks a lot @jonrob, I wasn't close to that solution.

@rvazquez Can you please take a look at the MuonPID performance in the current lhcb-head builds, i.e. compare the same test between the DetDesc and dd4hep builds

https://lhcb-nightlies.web.cern.ch/nightly/lhcb-dd4hep/1290/Moore/x86_64_v2-centos7-gcc11-opt/tests#RecoConf.hlt2_light_reco_pr_kf_without_UT_Calo_with_mcchecking

https://lhcb-nightlies.web.cern.ch/nightly/lhcb-dd4hep/1290/Moore/x86_64_v2-centos7-gcc11+dd4hep-opt/tests#RecoConf.hlt2_light_reco_pr_kf_without_UT_Calo_with_mcchecking

There isn't a lot there to compare for the Muons, but what there is shows significant differences

DetDesc

MuonIDHlt2AlgLong                      INFO Number of counters : 7
 |    Counter                                      |     #     |    sum     | mean/eff^* | rms/err^*  |     min     |     max     |
 | "BgLL"                                          |       450 |  -367.8637 |   -0.81747 |     1.2289 |     -9.6688 |       0.0000 |
 | "MuLL"                                          |       450 |   -3720.09 |    -8.2669 |     3.6096 |     -11.513 |  -0.0025989 |
 | "muonMVAStat"                                   |       450 |  -171.7853 |   -0.38175 |    0.98425 |     -3.3735 |      4.2197 |
 |*"nInAcceptance"                                 |      5453 |       4316 |( 79.14909 +- 0.5501330)% |
 |*"nIsMuon"                                       |      5453 |        450 |( 8.252338 +- 0.3726218)% |
 |*"nIsMuonTight"                                  |      5453 |        212 |( 3.887768 +- 0.2617711)% |
 |*"nMomentumCut"                                  |      5453 |       4623 |( 84.77902 +- 0.4864607)% |

DD4HEP

MuonIDHlt2AlgLong                      INFO Number of counters : 7
 |    Counter                                      |     #     |    sum     | mean/eff^* | rms/err^*  |     min     |     max     |
 | "BgLL"                                          |        81 |  -109.1115 |    -1.3471 |     1.1101 |     -5.8434 |       0.0000 |
 | "MuLL"                                          |        81 |  -486.6161 |    -6.0076 |     3.2627 |     -11.513 |    -0.16680 |
 | "muonMVAStat"                                   |        81 |  -36.77727 |   -0.45404 |    0.89431 |     -1.9590 |      3.5293 |
 |*"nInAcceptance"                                 |      5449 |       4247 |( 77.94091 +- 0.5617178)% |
 |*"nIsMuon"                                       |      5449 |         81 |( 1.486511 +- 0.1639357)% |
 |*"nIsMuonTight"                                  |      5449 |         52 |(0.9543035 +- 0.1317051)% |
 |*"nMomentumCut"                                  |      5449 |       4628 |( 84.93302 +- 0.4846107)% |

So whilst the MuonPID is now running in the dd4hep builds, its performance probably needs looking into ?

@raaij Did you perform the comparison you discussed a few days back, so compare the info you where getting for the muons from DetDesc and DD4HEP for the HLT1 application ?

FYI @gligorov @decianm

See !3142 (merged)

This adds some debug printout of the Cache parameters in the MuonID alg, similar I think to what @raaij was going to do for HLT1. There are clear differences, @desimone @farchill @rvazquez Please take a look and comment ?

There is a (minor) difference of 4 tracks not been considered with DD4HEP that is not due to the muonID. After this, there are few differences (2%) in the tracks in the muon system acceptance, which depend on the geometrical coordinates of the muon system, and a large difference (factor 5) of tracks failing isMuon, which should be related to the hits in the stations (either lacking or assigned to the wrong chamber, or any other combination of things that could have gone wrong). We are looking into this.

@rvazquez yes, in the above I only show the diffs coming from the cached detector geometry differences, I didn't look into differences coming from the input hit information each build sees.

Just w.r.t. the geometry parameters, its reasonable for there to be small diffs between the frameworks, but the above diffs are way more than explainable by this I think.

mentioned in issue #406 (closed)

mentioned in merge request Detector!300 (merged)

@rvazquez Is this still relevant?

yes, @farchill and @bcouturi are looking into this

are there any news @rvazquez ?

I don't know what @farchill discovered on this issue.

as this seems stale and https://lhcb-nightlies.web.cern.ch/nightly/lhcb-master/2181/Moore/x86_64_v2-centos7-gcc12-dbg/tests#RecoConf_hlt2_light_reco_pr_kf_without_UT_with_mcchecking is working fine I am closing the issue. feel free to reopen if there's any development

closed