Fixed MCHit->Pixel associator issue in VPClusterEffSimDQ

The MCHit to VPChannelID linker table sets the high bits in integer representing the the VPChannelID while the VPCluster list of associated pixels does not so they stopped matching (this is a change from previous behaviour). This fix uses the pixel only bits in VPChannelID. This restores the code functionality. This requires the change in VPChannelID Detector!662 (merged) to work.

requested review from @tlatham and @gcorti

added RTA label

changed the description

marked this merge request as ready

changed the description

Thanks @hcroft for tracking this down. I guess this problem must have been introduced when the VPChannelID was extended to include the orfx and orfy bits and we didn't notice until now?

@tlatham I think I've worked it out, the problem was the new default bit pattern of all ones not all zeros.

unsigned int m_channelID{std::numeric_limits<unsigned int>::max()};

sets every bit to 1, then if you go through and set each component explicitly rather than in a constructor that sets all of them you get the unused bits set to 1 not 0 as they were before. It has shown up an inconsistent way of setting VPChannelIDs. VPClustering::pointToChannel is the only place I could find with a search that does this, but that case does lead to "bad" VPChannelIDs. They work if you use the accessors, they just do not match the same bit pattern as other ways of generating the same pixel, so fail the associators.

added VP label

mentioned in merge request !795 (merged)

added 21 commits

• 8c6e2de9...d380002e - 11 earlier commits
• 0b8396aa - Dropped unused VPCheckers' VPOccupancyMonitor
• 3a519ee6 - Dropped unused VPCheckers' VPTrackEff
• 29f08c58 - Removed usage of GaudiHistoAlg in classes where it was actually no more used
• 7a80bf78 - Dropped unused VPHitMonitors
• 70942639 - More gaudi histo drop
• 8e1271e9 - Dropped unused UT code
• 8eb11e05 - RichParticleProperties: Adapt to RadiatorArray to DetectorArray migration
• f50eca28 - Prepare Lbcom v36p1
• 463467ea - Prepare Lbcom v36r1
• 7babe54d - Updated to master and removed unneccessary include

Compare with previous version

Updated MR to master and removed unnecessary LHCbID.h include.

resolved all threads

mentioned in issue Moore#896 (closed)

added 42 commits

• 7babe54d...79db823b - 40 commits from branch master
• 0e6c7eee - Fixed MCHit->Pixel associator issue in VPClusterEffSimDQ
• f4197b11 - Removed unneccessary include

Compare with previous version

Have also cleaned up the history with an interactive rebase

added 1 commit

• 65420c48 - Using new pixelID static function in VPChannelID to access pixel bits in integer rep.

Compare with previous version

changed the description

mentioned in merge request Detector!662 (merged)

resolved all threads

/ci-test

added ci-test-triggered label

• [2024-12-30 14:27] Validation started with lhcb-master-mr#12149
• [2024-12-30 14:57] Validation started with lhcb-master-mr#12150
• [2024-12-30 14:59] Validation started with lhcb-sim11-mr#195
• [2025-01-10 08:43] Validation started with lhcb-master-mr#12190
• [2025-01-10 08:43] Validation started with lhcb-sim11-mr#202
• [2025-01-13 09:42] Validation started with lhcb-sim11-mr#204
• [2025-01-14 08:27] Validation started with lhcb-master-mr#12219
• [2025-01-17 09:40] Validation started with lhcb-sim11-mr#208

/ci-test Detector!662 (merged)

resolved all threads

/ci-test Detector!662 (merged) --model lhcb-sim11

added 1 commit

• ae01f07b - Adapted code following TLatham's suggestions

Compare with previous version

/ci-test Detector!662 (merged)

/ci-test Detector!662 (merged) --model lhcb-sim11

approved this merge request

resolved all threads

@hcroft I fear there is now a conflict on this one. Can you solve it ?

@sponce I'll have a look this morning.

added 6 commits

• ae01f07b...7f2c9afb - 2 commits from branch master
• be149ea2 - Fixed MCHit->Pixel associator issue in VPClusterEffSimDQ
• 93581939 - Removed unneccessary include
• a7c106a2 - Using new pixelID static function in VPChannelID to access pixel bits in integer rep.
• d2a6a595 - Adapted code following TLatham's suggestions

Compare with previous version

@sponce I have updated the MR, so this should be ready to commit now.

closed

reopened

Oops, clicked comment and close not just comment by mistake. This still needs to be merged.

Let's retest after the rebase : /ci-test Detector!662 (merged)

/ci-test Detector!662 (merged) --model lhcb-sim11

/ci-test Detector!662 (merged)

resolved all threads

resolved all threads

@hcroft, @tlatham do we have a monitor in Boole that would use this and show the effect in PRTests? The existing histogram of VP in the Boole dashboard are identical and for me this can go ahead and be merged together with the update of the reference in lhcb-sim11.

@sponce can you trigger the update of the reference for the lhcb-sim11 slot? It has the 'right' platform with respect to master.

let me also hijack this to ask if you @hcroft and @tlatham can take a look at the Boole-VP test on dd4hep and if it makes sense, so that we can also update that reference and make it green

@gcorti @tlatham I think it is OK, the numbers look sensible to me. It would be helpful to clear all of the imminent MRs; mine are all in I think now but I'm not sure what else is being integrated this week. If that is ready at @tltaham can confirm, I'd be happy to update the reference to this version.

/ci-test Detector!662 (merged) --model lhcb-sim11

resolved all threads

merged

mentioned in commit a14f38d6