Update TrkrNtuplizer: refine ntuple filling and fix variable handling#4252
Open
Mughal789 wants to merge 2 commits intosPHENIX-Collaboration:masterfrom
Open
Update TrkrNtuplizer: refine ntuple filling and fix variable handling#4252Mughal789 wants to merge 2 commits intosPHENIX-Collaboration:masterfrom
Mughal789 wants to merge 2 commits intosPHENIX-Collaboration:masterfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughVertex NTuple filling in TrkrNtuplizer was changed to fetch Changes
sequenceDiagram
participant Trkr as TrkrNtuplizer
participant Top as topNode
participant Map as SvtxVertexMapActs
participant Ntp as _ntp_vertex
Trkr->>Top: findNode::getClass<SvtxVertexMap>("SvtxVertexMapActs")
alt vertex map missing
Top-->>Trkr: nullptr
Trkr->>Trkr: emit warning, skip vertex fills
else vertex map present
Top-->>Trkr: SvtxVertexMap*
Trkr->>Trkr: start timer (if verbose)
Trkr->>Map: iterate vertices
loop per vertex
Trkr->>Trkr: init fx_vertex to quiet_NaN()
Trkr->>Trkr: populate id, vx/vy/vz, ntracks, chi2, ndof
Trkr->>Ntp: assemble vertex_data (fx_event+fx_vertex+fx_info)
Trkr->>Ntp: _ntp_vertex->Fill(vertex_data)
end
Trkr->>Trkr: stop timer (if verbose) and log vertex time
end
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Refines TrkrNtuplizer vertex ntuple filling to populate real vertex quantities from SvtxVertexMapActs (instead of writing placeholder NaNs) and includes additional vertex fields.
Changes:
- Replaces placeholder-only vertex ntuple filling with per-vertex filling from
SvtxVertexMapActs. - Initializes vertex ntuple fields to
quiet_NaN()and fillsvertexID, position, track count,chi2, andndof. - Adds a warning when
SvtxVertexMapActsis missing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1391
to
+1431
| @@ -1402,11 +1402,8 @@ void TrkrNtuplizer::fillOutputNtuples(PHCompositeNode* topNode) | |||
| { | |||
| i = 0; | |||
| } | |||
|
|
|||
| // SvtxVertexMap* vertexmap = nullptr; | |||
|
|
|||
| // vertexmap = findNode::getClass<SvtxVertexMap>(topNode, "SvtxVertexMapActs"); // Acts vertices | |||
|
|
|||
| float vx = std::numeric_limits<float>::quiet_NaN(); | |||
| float vy = std::numeric_limits<float>::quiet_NaN(); | |||
| float vz = std::numeric_limits<float>::quiet_NaN(); | |||
| @@ -1425,12 +1422,83 @@ void TrkrNtuplizer::fillOutputNtuples(PHCompositeNode* topNode) | |||
| std::copy(fx_info, fx_info + ((int) (n_info::infosize)), vertex_data + n_event::evsize + n_vertex::vtxsize); | |||
| _ntp_vertex->Fill(vertex_data); | |||
| delete[] vertex_data; | |||
|
|
|||
| } | |||
| if (Verbosity() > 1) | |||
| { | |||
| _timer->stop(); | |||
| std::cout << "vertex time: " << _timer->get_accumulated_time() / 1000. << " sec" << std::endl; | |||
| }*/ | |||
There was a problem hiding this comment.
The previous vertex-ntuple implementation is kept as a large commented-out block. This makes the function harder to read/maintain and risks it becoming stale. Please remove the dead commented code (it’s already replaced by the new implementation); use version control history if it needs to be referenced later.
Contributor
There was a problem hiding this comment.
I would agree, we should just remove the old version
Contributor
Author
There was a problem hiding this comment.
Thanks Joe, I removed the older version and made a PR again.
Comment on lines
+1434
to
+1435
| // Fix Nan placeholders for vertex variables | ||
| // //----------------------- |
There was a problem hiding this comment.
The header comment has a couple of issues: "Nan" should be the standard "NaN", and the separator line below is double-commented (// //-----------------------). Please fix the comment text/formatting to match the surrounding section headers.
Suggested change
| // Fix Nan placeholders for vertex variables | |
| // //----------------------- | |
| // Fix NaN placeholders for vertex variables | |
| //----------------------- |
|
|
||
| if (!vertexmap) | ||
| { | ||
| std::cout << PHWHERE << " WARNING: SvtxVertexMapActs not found. Writing no vertex entries for this event." << std::endl; |
There was a problem hiding this comment.
This warning is printed unconditionally when the node is missing, which can spam logs on jobs/configurations that don’t produce Acts vertices. Consider gating it behind a verbosity level (or emitting it once per run) so production output isn’t flooded.
Suggested change
| std::cout << PHWHERE << " WARNING: SvtxVertexMapActs not found. Writing no vertex entries for this event." << std::endl; | |
| static bool s_vertexmap_missing_warning_printed = false; | |
| if (!s_vertexmap_missing_warning_printed) | |
| { | |
| std::cout << PHWHERE << " WARNING: SvtxVertexMapActs not found. Writing no vertex entries for this event." << std::endl; | |
| s_vertexmap_missing_warning_printed = true; | |
| } |
Comment on lines
+1484
to
+1488
| float* vertex_data = new float[((int) (n_info::infosize)) + n_event::evsize + n_vertex::vtxsize]; | ||
| std::copy(fx_event, fx_event + n_event::evsize, vertex_data); | ||
| std::copy(fx_vertex, fx_vertex + n_vertex::vtxsize, vertex_data + n_event::evsize); | ||
| std::copy(fx_info, fx_info + ((int) (n_info::infosize)), vertex_data + n_event::evsize + n_vertex::vtxsize); | ||
|
|
There was a problem hiding this comment.
vertex_data is heap-allocated and freed for every vertex. This adds avoidable per-vertex overhead; you can allocate the buffer once outside the vertex loop and reuse it (or use a fixed-size stack buffer / std::array) since the size is known from the enum constants.
Build & test reportReport for commit 5cd077d06bf21763bd08706b904e099861f2a5a8:
Automatically generated by sPHENIX Jenkins continuous integration |
Mughal789
force-pushed
the
trkrntuplizer_update
branch
from
5cd077d to
d1507d2
Compare
Build & test reportReport for commit d1507d23a3a30c46251e7c5d668120b02eddaf0e:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 6ffe19a5f9f80ab03a7e0ad8c39458a60ecf6b67:
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
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.
comment: <> Fixed NaN placeholders for vertex variables
Types of changes
What kind of change does this PR introduce? Bug Fix
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
PR Summary: TrkrNtuplizer — refine vertex ntuple filling and fix variable handling
Motivation / context
Key changes
Potential risk areas
Possible future improvements
Caveat