Minor updates to PHActsSiliconSeeder, TrackResiduals, PHSimpleVertexFinder#4135
Merged
osbornjd merged 9 commits intosPHENIX-Collaboration:masterfrom Jan 30, 2026
Merged
Conversation
…hi relative to beamspot, circle uses MVTX barrel center and not zero, changes strobe check to +/- 1, reduce MVTX cluster requirement to 2.
…esoftware into new_tpc_transforms
Contributor
📝 WalkthroughWalkthroughCompute cluster phi relative to a configurable beam spot and MVTX center in silicon seeding, widen strobe-consistency checks, expose setters for beam-spot and MVTX center, lower required MVTX vertices from 3 to 2, and add a Changes
✨ 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 |
Build & test reportReport for commit 9ec298fb7d3b94a243e8056445b7879d11f58659:
Build & test reportReport for commit 18c5a2c56900618bcf9798bb091f064b628994a4:
Build & test reportReport for commit f704495167ff5a179e3b400f49850351b7084714:
Automatically generated by sPHENIX Jenkins continuous integration Automatically generated by sPHENIX Jenkins continuous integration Automatically generated by sPHENIX Jenkins continuous integration |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
offline/packages/trackreco/PHActsSiliconSeeding.cc (1)
1158-1163: Same avgtripletphi concern as above.
The averaging logic here mirrors the earlier block; please apply the same verification decision.
🧹 Nitpick comments (2)
offline/packages/trackreco/PHSimpleVertexFinder.h (1)
93-95: Confirm the physics impact of lowering the default MVTX requirement.
Line 94 changes the default to 2, which can alter vertexing selection and downstream outputs. Please confirm this is intended for all production modes and update any reconstruction notes/config defaults accordingly.offline/packages/trackreco/PHActsSiliconSeeding.h (1)
370-377: Verify and document run-specific defaults to avoid silent physics shifts.
The hard-coded Run-3 pp defaults (Lines 370-377) will be applied whenever callers don’t override them. Please confirm that production configs (and simulation) explicitly set these per dataset and document expected analysis impact/reprocessing needs.Based on learnings, please include explicit analysis-impact and reprocessing guidance when reconstruction defaults change.
Build & test reportReport for commit c38e2000cb52d9d00fe8eee961b398aff745a349:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
reviewed
Jan 28, 2026
Contributor
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
This makes the INTT matching efficiency in the simulation a little worse at high pT, which is confusing given that it should be more correct. I'm not sure I understand why
Comment on lines
+370
to
+377
| // collision point in sPHENIX coordinates, from vertex finder (pp run 3) | ||
| float m_beamSpotx = -0.072; // cm | ||
| float m_beamSpoty = 0.141; // cm | ||
|
|
||
| // center of MVTX barrel in sPHENIX coordinates - default is for Run 3 pp | ||
| float m_mvtx_x0 = 0.6; // cm | ||
| float m_mvtx_y0 = -0.1; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and context around these lines
wc -l offline/packages/trackreco/PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 131
🏁 Script executed:
# Examine the code around these defaults to understand their purpose and usage
sed -n '350,390p' offline/packages/trackreco/PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1473
🏁 Script executed:
# Search for where these member variables are used within the class
rg "m_beamSpot|m_mvtx_" offline/packages/trackreco/PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 288
🏁 Script executed:
# Check if there's a corresponding .cc file to see how these are used
fd "PHActsSiliconSeeding\.cc" offline/packages/trackreco/Repository: sPHENIX-Collaboration/coresoftware
Length of output: 128
🏁 Script executed:
# Search for usage of these beam-spot/MVTX variables in the .cc file
rg "m_beamSpot|m_mvtx_" offline/packages/trackreco/PHActsSiliconSeeding.cc -A 3 | head -60Repository: sPHENIX-Collaboration/coresoftware
Length of output: 409
🏁 Script executed:
# Check the git log to see if these defaults recently changed
git log --oneline -20 -- offline/packages/trackreco/PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 107
🏁 Script executed:
# Check git diff to see what changed in this PR for these specific lines
git diff HEAD~1 -- offline/packages/trackreco/PHActsSiliconSeeding.h | grep -A 5 -B 5 "beamSpot\|mvtx_"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 106
🏁 Script executed:
# Check git status to understand the current state
git statusRepository: sPHENIX-Collaboration/coresoftware
Length of output: 143
🏁 Script executed:
# Try to get the full git log with more context
git log --oneline -50 -- offline/packages/trackreco/PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 107
🏁 Script executed:
# Check if there's a PR description or commit message
git log -1 --format="%B"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 98
🏁 Script executed:
# Search for any recent changes to these defaults across the repository
git log -p --all -S "m_beamSpotx = -0.072" -- offline/packages/trackreco/PHActsSiliconSeeding.h | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check if there's more commit history we can access
git log --all --oneline | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2908
🏁 Script executed:
# Try to see the current file state and search for when these specific values were set
git log --all -p -- offline/packages/trackreco/PHActsSiliconSeeding.h | grep -B 10 -A 10 "0.6.*cm\|0.141" | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 164
🏁 Script executed:
# Look for any documentation about these parameters in comments
grep -B 5 -A 5 "Run 3 pp\|beam.spot" offline/packages/trackreco/PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 456
🏁 Script executed:
# Check if there are any related pull request files or documentation
find . -name "*README*" -o -name "*CHANGES*" -o -name "*NOTES*" | grep -i silicon | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check what the current HEAD is and what changed
git show --stat 8bc0f1f7cRepository: sPHENIX-Collaboration/coresoftware
Length of output: 50391
🏁 Script executed:
# Check if the beam spot/mvtx defaults were changed in this commit
git show 8bc0f1f7c -- offline/packages/trackreco/PHActsSiliconSeeding.h | grep -B 5 -A 5 "beamSpot\|mvtx_"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1020
🏁 Script executed:
# Look at the full diff for this file in this commit
git show 8bc0f1f7c -- offline/packages/trackreco/PHActsSiliconSeeding.h | head -150Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3868
🏁 Script executed:
# Try to find PR-related files or documentation
find . -name "*.md" -o -name "*.txt" | grep -i "pull\|pr\|request" | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 112
🏁 Script executed:
# Check the commit message more thoroughly
git log -1 8bc0f1f7c --format="%B" | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 98
🏁 Script executed:
# Look for any PR description files in the repo root
ls -la | grep -i "pull\|pr\|description"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check the PR template to understand what should be documented
cat ./.github/pull_request_template.mdRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1238
Document reconstruction impact and reprocessing requirements in PR description.
The beam-spot and MVTX center defaults are used directly in reconstruction (fitting calculations and phi angle computations). Changes to these geometry parameters affect track seeding outputs. State the expected analysis impact and whether existing data requires reprocessing.
…esoftware into new_tpc_transforms
…e into new_tpc_transforms
Build & test reportReport for commit 8bc0f1f7ceaa9e5342674eca1691d8db28bc0628:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 344e32c717695c8605ac9b1092f62d80f5bfc27b:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
The INTT matching efficiency still degrades at high pT, actually it appears in exactly the same way as behavior. So apparently coderabbit wasn't exactly correct (at least, the problem it points out is not directly responsible for the loss in matching efficiency at high pT) |
Contributor
|
Interestingly even after setting the beamspot to 0 (unless I made a mistake or omitted something here), the matching efficiency in the simulation still gets worse at high pT It does improve the data reconstruction, so maybe we just merge this and keep trying to understand why the sim matching gets worse? |
Contributor
Author
|
Yes, I think we should merge it. The overall tracking efficiency in simulations is not affected.
…________________________________
From: Joseph (Joe) Osborn ***@***.***>
Sent: Friday, January 30, 2026 8:43 AM
To: sPHENIX-Collaboration/coresoftware ***@***.***>
Cc: Anthony Frawley ***@***.***>; Author ***@***.***>
Subject: Re: [sPHENIX-Collaboration/coresoftware] Minor updates to PHActsSiliconSeeder, TrackResiduals, PHSimpleVertexFinder (PR #4135)
[https://avatars.githubusercontent.com/u/53052717?s=20&v=4]osbornjd left a comment (sPHENIX-Collaboration/coresoftware#4135)<#4135 (comment)>
Interestingly even after setting the beamspot to 0 (unless I made a mistake or omitted something here<sPHENIX-Collaboration/macros#1279>), the matching efficiency in the simulation still gets worse at high pT
https://nbviewer.sphenix.bnl.gov/github/sPHENIX-Collaboration/QA-gallery/blob/jenkins-sPHENIX-test-tracking-low-occupancy-qa-8452-test-tracking_Event400_Sum28/QA-tracking.ipynb
It does improve the data reconstruction, so maybe we just merge this and keep trying to understand why the sim matching gets worse?
—
Reply to this email directly, view it on GitHub<#4135 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHADBLEMH2VW53ICOKBZZT4JNNXTAVCNFSM6AAAAACSSRMXWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTQMRTHAZTANBZHA>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
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
2 participants
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.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
Modifies PHActsSiliconSeeder to
a) Calculate cluster phi positions relative to the collision point, instead of (x,y) = (0,0).
b) Estimate the seed/layer intersection for the MVTX assuming the layers are centered at the actual barrel center.
c) Changes the strobe matching check to require +/- 1 instead of zero. This produces a small increase in efficiency for fully fitted tracks.
Notes: The default beam spot is set for Run 3 pp, but should be close enough for seeding in AuAu and Run 2 pp. The default beam spot and MVTX barrel positions are set for data, but I think they should work for simulation, let's see what Jenkins thinks. In any case, the simulations should use values that match data.
Adds the cluster layer to clustertree in TrackResiduals.
Changes the default minimum number of MVTX clusters required for a track to be used for vertexing in PHSimpleVertexFinder to 2, instead of 3.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Minor Updates to Silicon Track Seeding and Vertex Finding
Motivation / Context
Improve silicon-based seeding and vertexing fidelity by computing cluster azimuths relative to a realistic collision point and MVTX geometry, and recover a small amount of reconstruction efficiency by relaxing timing/hit-count requirements. Defaults are set for Run 3 pp but should be overridden to match specific run/simulation conditions.
Key Changes
Potential Risk Areas
Possible Future Improvements
Note: This summary was produced with AI assistance. AI can make mistakes—please verify code diffs, public API changes, and validate reconstruction performance before deploying.