Skip to content

ITSMFT/ITS3: fix MC labels for clusters after digit squashing - #16098

Open
mpuccio wants to merge 2 commits into
AliceO2Group:devfrom
mpuccio:fix-single-pixel-cluster-squashing-label
Open

mpuccio wants to merge 2 commits into
AliceO2Group:devfrom
mpuccio:fix-single-pixel-cluster-squashing-label

Conversation

@mpuccio

@mpuccio mpuccio commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Digit squashing can remove earlier digits on a chip and reorder the remaining pixels. Looking up a single-pixel cluster's MC label as chipStart + hit can therefore select a discarded digit's label.

Use a shared ChipPixelData::getDigitID() accessor for every cluster-label lookup in ITSMFT and ITS3. It selects the stored mapping when present and otherwise uses contiguous digit indices, independently of the clusterer's squashing setting. Also skip label fetching when the reader has no MC truth.

@sawenzel

Copy link
Copy Markdown
Collaborator

This is a PR comment generated by Claude Opus:

This PR changes Clusterer::ClustererThread::finishChipSingleHitFast in ITSMFT so that, under squashing, the MC label of a single-pixel cluster is looked up through getOrderedPixId(hit) instead of getStartID() + hit.

The fix looks correct for the ITSMFT DPL workflow, because hit indexes the sorted pixel vector and mPixelsOrder is sorted with the same comparator. It is incomplete, though, because the ITS3 copy keeps the old lookup and the new branch can read empty vectors. Nothing was built or run; the review is from reading the PR head 714cf05.

I suggest fixing ITS3 in the same PR and moving the index choice into ChipPixelData, for example a getDigitID(pos) that returns mStartID + pos when mPixIds is empty and mPixIds[mPixelsOrder[pos]] otherwise. That one change would cover the first three findings below.

  1. ITS3 not fixed. Detectors/Upgrades/ITS3/reconstruction/src/Clusterer.cxx:319 still calls fetchMCLabels(curChipData->getStartID() + hit, ...), while its finishChip already uses getOrderedPixId. ITS3 MC clusterization with squashing keeps producing wrong labels for single-pixel clusters.

  2. Out-of-bounds read without MC truth in the reader. In Detectors/ITSMFT/common/reconstruction/src/Clusterer.cxx:320 the new path is gated on labelsClusPtr and Clusterer::mSquashingDepth only. DigitPixelReader fills mPixIds/mPixelsOrder only when its mDigitsMCTruth is non-null. When ClustererSpec calls reader.setDigitsMCTruth(nullptr) for an empty label container, a single-pixel chip indexes two empty vectors. This may not occur in practice, and before the PR the same case already crashed on the null labelsDigPtr.

  3. Index scheme depends on a copy of the deciding state. Clusterer::mSquashingDepth is kept in sync with DigitPixelReader::mSquashOverflowsDepth only by ClustererSpec.

    • A caller that sets setMaxROFDepthToSquash(n > 0) without reader.setSquashingDepth (a macro, a test, a non-DPL driver) worked before and now indexes empty vectors.
    • The opposite mismatch silently keeps the wrong labels.
  4. Duplication. The squashing-versus-contiguous choice now exists three times in this file (finishChip lines 238-242 and 259-263, and here), plus ITS3.

  5. No regression test, empty PR description. A test with two ROFs, one chip, the first digit squashed away and one survivor, asserting the label equals that digit's label, would guard this.

A minor point: the added two-line comment explains the reasoning, whereas the existing one-liner at finishChip line 238 is the precedent in that file.

@mpuccio mpuccio changed the title ITSMFT: fix MC labels for single pixel clusters in the labels ITSMFT/ITS3: fix MC labels for clusters after digit squashing Nov 12, 2026
@mpuccio
mpuccio force-pushed the fix-single-pixel-cluster-squashing-label branch from f4cb578 to 642f101 Compare November 12, 2026 21:46

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants