fix: duplicate seed removal bug#4165
Merged
osbornjd merged 6 commits intosPHENIX-Collaboration:masterfrom Feb 6, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughConsolidates per-seed verbose diagnostics into a new helper Changes
Possibly related PRs
✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
+217
to
237
| for (const auto& [trackKey, mvtxKeys] : matches) | ||
| { | ||
| std::cout << "original track: " << std::endl; | ||
| track->identify(); | ||
| } | ||
| auto* track = m_siliconTracks->get(trackKey); | ||
| if (Verbosity() > 2) | ||
| { | ||
| std::cout << "original track: " << std::endl; | ||
| track->identify(); | ||
| } | ||
|
|
||
| for (const auto& key : mvtxKeys) | ||
| { | ||
| if (track->find_cluster_key(key) == track->end_cluster_keys()) | ||
| for (const auto& key : mvtxKeys) | ||
| { | ||
| track->insert_cluster_key(key); | ||
| if (Verbosity() > 2) | ||
| if (track->find_cluster_key(key) == track->end_cluster_keys()) | ||
| { | ||
| std::cout << "adding " << key << std::endl; | ||
| track->insert_cluster_key(key); | ||
| if (Verbosity() > 2) | ||
| { | ||
| std::cout << "adding " << key << std::endl; | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing null check on track can cause a crash.
m_siliconTracks->get(trackKey) can return nullptr (as evidenced by the null check at line 128 for track2). If track is null here, lines 223 and 228 will dereference it, causing undefined behavior.
Proposed fix: add null guard
for (const auto& [trackKey, mvtxKeys] : matches)
{
auto* track = m_siliconTracks->get(trackKey);
+ if (!track)
+ {
+ continue;
+ }
if (Verbosity() > 2)
{
std::cout << "original track: " << std::endl;
track->identify();
}
Build & test reportReport for commit 77ed93afaafa2f84882b5811d78d4bb6f920ff5f:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit cb381d6853ee81bf7b8d9d25d55aa42596706d1b:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 756f1ffafab76a35c4c7a7d2f5a4e0af89a2576d:
Automatically generated by sPHENIX Jenkins continuous integration |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a subtle one line bug found by Yuko and Dennis where some exact duplicate seeds were not removed from the seed pool. It also adds a diagnostic function to check for this
Types of changes
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)
Pull Request Summary: Fix Duplicate Seed Removal Bug
Motivation & Context
A logic bug caused some exact duplicate silicon seed tracks to survive the seed-merging step, producing redundant seed candidates and potentially duplicate reconstructed tracks. This PR fixes the duplicate-detection/merging logic and adds a diagnostic to surface any remaining duplicates.
Key Changes
Potential Risk Areas
Possible Future Improvements
Note: this summary was prepared with AI assistance—AI can err; please verify the patch and behavior during review.