Skip to content

Fix Track Sharing in PHSimpleVertexFinder - #4444

Merged
osbornjd merged 1 commit into
sPHENIX-Collaboration:masterfrom
adeebsaed:Fix_SVF_Track_Sharing
Sep 21, 2026
Merged

osbornjd merged 1 commit into
sPHENIX-Collaboration:masterfrom
adeebsaed:Fix_SVF_Track_Sharing

Conversation

@adeebsaed

@adeebsaed adeebsaed commented Sep 17, 2026 •

Copy link
Copy Markdown

comment: <

The original findConnectedTracks implementation used a single scan of the track-pair map to extend each connected group. During that scan, a pair could add tracks to the group when at least one of its tracks was already present. However, a pair rejected earlier because neither track belonged to the group was not reconsidered after subsequent pairs expanded the group. Consequently, the group could be finalized before all indirectly connected tracks had been included, making the grouping dependent on the order in which pairs were visited.

For example, consider a group initially containing tracks A and B. If the scan encounters the pair C–D before B–C, it initially skips C–D. The later pair B–C adds C, making C–D eligible to extend the group, but the single scan never returns to it. Track D therefore remains outside the group despite being connected to A and B through C. Subsequent grouping can then produce overlapping groups that share tracks, contributing to track sharing between vertex candidates.

This change wraps the existing inner pair scan in a loop that repeats until a complete scan adds no new tracks. At the start of each pass, the size of the connected set is recorded. After the pass, the scan is repeated only if that size has increased. This allows previously skipped pairs to be reconsidered as the group grows and follows indirect connections until no further expansion is possible. Because connected is a set, inserting tracks already present does not increase its size or unnecessarily prolong the loop. The finite number of available tracks guarantees termination.

The modification is confined to repeating the existing inner scan and checking whether the connected set has grown. The existing pair-selection conditions, track insertions, used bookkeeping, and group-finalization logic are retained. No reconstruction cuts or downstream vertex-position calculations are changed. The fix addresses incomplete connected-group construction.

(Please tell us something about this pull request)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work for users)
  • Requiring change in macros repository (Please provide links to the macros pull request in the last section)
  • I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

What kind of change does this PR introduce? (Bug fix, feature, ...)

TODOs (if applicable)

Links to other PRs in macros and calibration repositories (if applicable)

track_sharing_pull_request_plots.pdf

Uploading SAED_PPG_07_25_2026.pdf…

Motivation / Context

This change fixes primary track sharing in PHSimpleVertexFinder. It ensures that chained track connections form complete connected groups.

Key Changes

  • findConnectedTracks now repeats the pair scan until a full scan adds no tracks.
  • Existing pair-selection conditions remain unchanged.
  • No public interface or I/O format changes were made.

Potential Risk Areas

  • Reconstruction output can change for chained track connections.
  • Repeated scans can increase processing time for large track groups.
  • No thread-safety impact is indicated.

Possible Future Improvements

  • Add regression tests for multi-step track-sharing chains.
  • Measure performance for high track multiplicities.

Test results were not provided. AI-generated summaries can contain mistakes; verify this summary against the implementation and validation results.

@coderabbitai

coderabbitai Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c19f0885-7e96-48f5-ab02-03cc6e9975d4

📥 Commits

Reviewing files that changed from the base of the PR and between 7726f1a and 3157e7f.

📒 Files selected for processing (1)
  • offline/packages/trackreco/PHSimpleVertexFinder.cc

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

findConnectedTracks now rescans the track-pair map until no new track joins the connected set. This resolves chained connections before the set is closed and emitted as a vertex.

Changes

Track connection resolution

Layer / File(s) Summary
Repeat pair scans until convergence
offline/packages/trackreco/PHSimpleVertexFinder.cc
findConnectedTracks repeats pair-map scans while the connected set grows. The scan stops when its size remains unchanged.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 3157e

This change completes chained track grouping while preserving existing selection behavior, with no material merge-blocking risk identified.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 3157e7f6f87f65ea82fc7a5d440a2ca962d379b6:
Jenkins passed


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@osbornjd

Copy link
Copy Markdown
Contributor

I'm not sure I understand what this PR accomplishes from the plots that are added - can you provide some context/detail?

@osbornjd
osbornjd merged commit 8e7a501 into sPHENIX-Collaboration:master Sep 21, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants