[alillari]

Use bankDGTZ.trueIndex(i) + 1 for BST hit IDs in fetch_SVTHits, matching BMT. This preserves the original ADC-row identity after filtering and fixes the resulting MC::True.hitn associations.

Focused regression and CVT reconstruction tests pass on the updated branch.

Fixes #1538.

[yugotra]

I verified the change against the RawDataBank implementation and on background-merged MC. The
replacement with bankDGTZ.trueIndex(i) + 1 is correct and matches BMT: i is the filtered-bank
position, while trueIndex(i) is the original BST::adc row.

On 200 background-merged MC events, the base code produced 26,092 of 30,407 BST hit IDs whose
referenced ADC row had a different sector/layer/strip. This PR reduced that count to zero. Final
CVT track tables and matched residual tables were byte-for-byte identical, so the patch repairs
provenance without changing reconstruction.

I find no blocking problem and approve the code change. As a non-blocking follow-up, please commit
the focused regression test mentioned in the PR description: place an order-filtered BST::adc
row before an accepted row and assert that the reconstructed hit ID is trueIndex + 1, including
the cluster hit reference. The existing CVT test does not protect this boundary condition.
PR1539_REVIEW.txt

[yugotra]

I verified the change against the RawDataBank implementation and on background-merged MC. The
replacement with bankDGTZ.trueIndex(i) + 1 is correct and matches BMT: i is the filtered-bank
position, while trueIndex(i) is the original BST::adc row.

On 200 background-merged MC events, the base code produced 26,092 of 30,407 BST hit IDs whose
referenced ADC row had a different sector/layer/strip. This PR reduced that count to zero. Final
CVT track tables and matched residual tables were byte-for-byte identical, so the patch repairs
provenance without changing reconstruction.

I find no blocking problem and approve the code change. As a non-blocking follow-up, please commit
the focused regression test mentioned in the PR description: place an order-filtered BST::adc
row before an accepted row and assert that the reconstructed hit ID is trueIndex + 1, including
the cluster hit reference. The existing CVT test does not protect this boundary condition.

[alillari]

Thanks for the review. I’ve added the focused regression test covering filtered BST hit IDs and cluster hit references. It fails on the unfixed code and passes with the fix.

[yugotra]

I reviewed the updated PR #1539. Recommendation: re-approve after authorizing the pending CI workflow.
I verified:

• Four tests pass with the fix.
• Temporarily restoring id = i + 1 makes exactly the leading- and interior-filtered-row tests fail.
• Tests cover BST::Hits.ID, cluster HitN_ID references, unfiltered rows, and ADC=-1 timing rows.
• No blocking code or test issue remains.

[alillari]

Fixed the test initialization to work with CLAS12DIR unset, as in CI. The full local unit-test workflow now passes. Hopefully it passes the test this time.

[yugotra]

I reviewed the updated PR #1539. Recommendation: re-approve.
I verified:

Four tests pass with the fix.
Temporarily restoring id = i + 1 makes exactly the leading- and interior-filtered-row tests fail.
Tests cover BST::Hits.ID, cluster HitN_ID references, unfiltered rows, and ADC=-1 timing rows.
No blocking code or test issue remains.

[baltzell]

@alillari Thanks for finding and fixing this (my) bug! When you update your branch we'll merge it.