Add TPC-only helix/Kalman fitters and V0 candidate diagnostics - #4359
Add TPC-only helix/Kalman fitters and V0 candidate diagnostics#4359997873828mcx wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesTPC tracking and V0 diagnostics
Sequence Diagram(s)sequenceDiagram
participant TpcV0CandidateTree
participant PHCompositeNode
participant TpcTrackHelixFitter
participant TpcTrackKalmanFitter
participant ROOT
TpcV0CandidateTree->>PHCompositeNode: Read event, vertex, truth, and TPC inputs
TpcV0CandidateTree->>TpcTrackHelixFitter: Build helix tracklets and PCA candidates
TpcV0CandidateTree->>TpcTrackKalmanFitter: Fit and propagate configured tracks
TpcV0CandidateTree->>TpcV0CandidateTree: Apply preselection and pair selection
TpcV0CandidateTree->>ROOT: Write pair, track, and residual trees
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}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 06a2883d-69e1-46b4-856f-9de4e6d0a5aa
📒 Files selected for processing (9)
offline/packages/TrackingDiagnostics/Makefile.amoffline/packages/TrackingDiagnostics/TpcV0CandidateTree.ccoffline/packages/TrackingDiagnostics/TpcV0CandidateTree.hoffline/packages/tpctrackreco/Makefile.amoffline/packages/tpctrackreco/TpcTrackFit.hoffline/packages/tpctrackreco/TpcTrackHelixFitter.ccoffline/packages/tpctrackreco/TpcTrackHelixFitter.hoffline/packages/tpctrackreco/TpcTrackKalmanFitter.ccoffline/packages/tpctrackreco/TpcTrackKalmanFitter.h
| static std::array<double, StateDim> propagation_state(const TpcKalmanResult &fit, | ||
| const TpcTrackVec3 &reference_vertex); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
propagation_state takes a reference_vertex it deliberately ignores.
The definition names the parameter /*reference_vertex*/ and the comment at TpcTrackKalmanFitter.cc Line 1034-1036 states the vertex must not influence the choice of end state. Every call site in TpcV0CandidateTree.cc (Lines 664, 826, 893, 1362, 2407, 2423, 2439) passes a vertex — two of them a dummy Vec3{}, the rest a real primary vertex — so the results are identical while the signature suggests otherwise. On a brand-new public API this is worth resolving now, before downstream code starts relying on a vertex dependence that does not exist.
Either drop the parameter, or keep it and document that it is reserved. Dropping it is cleaner and the call sites are all in this PR.
♻️ Proposed signature simplification
- static std::array<double, StateDim> propagation_state(const TpcKalmanResult &fit,
- const TpcTrackVec3 &reference_vertex);
+ // Selects the physical initial state of the fitted sequence from the charge
+ // sign alone; no vertex or transverse-distance heuristic is involved.
+ static std::array<double, StateDim> propagation_state(const TpcKalmanResult &fit);dca_to_vertex at Line 41-43 would then call propagation_state(fit) and the TpcV0CandidateTree helpers (kalman_point/kalman_tangent/kalman_momentum) can drop their reference_vertex argument too.
As per path instructions, "Focus on API clarity/stability, ownership semantics (RAII)" and "If interfaces change, ask for compatibility notes and any needed downstream updates."
Source: Path instructions
| m_file = new TFile(m_filename.c_str(), "RECREATE"); | ||
| if (!m_file || m_file->IsZombie()) | ||
| { | ||
| std::cout << Name() << ": failed to create output file " << m_filename << std::endl; | ||
| return Fun4AllReturnCodes::ABORTRUN; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Failed TFile open leaves a zombie handle that End() will still write to.
When IsZombie() is true the function returns ABORTRUN but m_file stays non-null and the allocated TFile is never released. If the framework still calls End() during teardown, Lines 462-479 will cd(), Write() the trees and Close() a zombie file. Deleting and nulling the pointer here makes the failure state unambiguous and removes the leak.
🛡️ Proposed fix
m_file = new TFile(m_filename.c_str(), "RECREATE");
if (!m_file || m_file->IsZombie())
{
std::cout << Name() << ": failed to create output file " << m_filename << std::endl;
+ delete m_file;
+ m_file = nullptr;
return Fun4AllReturnCodes::ABORTRUN;
}As per path instructions, "Prioritize correctness, memory safety, error handling, and thread-safety."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m_file = new TFile(m_filename.c_str(), "RECREATE"); | |
| if (!m_file || m_file->IsZombie()) | |
| { | |
| std::cout << Name() << ": failed to create output file " << m_filename << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| m_file = new TFile(m_filename.c_str(), "RECREATE"); | |
| if (!m_file || m_file->IsZombie()) | |
| { | |
| std::cout << Name() << ": failed to create output file " << m_filename << std::endl; | |
| delete m_file; | |
| m_file = nullptr; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } |
Source: Path instructions
| std::sort(candidates.begin(), candidates.end(), | ||
| [](const KalmanPca &lhs, const KalmanPca &rhs) | ||
| { return lhs.dca < rhs.dca; }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Non-finite dca values can reach this sort and break the comparator's strict weak ordering.
refine_kalman_pair derives best.dca from kalman_point, which returns a NaN triple whenever propagate_rkn4 exhausts its step budget — a reachable outcome over the ±max_upstream_cm scan, and precisely what rkn_failures counts. A NaN key makes lhs.dca < rhs.dca inconsistent, which is undefined behaviour for std::sort (libstdc++ can run off the end of the range during its insertion-sort phase).
Note the seed loop at Line 2574 already filters !std::isfinite(d2) before sorting; the candidate sort needs the same protection. Dropping non-finite candidates also spares make_pair_row from consuming a NaN pair_dca.
🛡️ Proposed fix
+ candidates.erase(
+ std::remove_if(candidates.begin(), candidates.end(),
+ [](const KalmanPca &candidate)
+ { return !std::isfinite(candidate.dca); }),
+ candidates.end());
+
std::sort(candidates.begin(), candidates.end(),
[](const KalmanPca &lhs, const KalmanPca &rhs)
{ return lhs.dca < rhs.dca; });
return candidates;make_pair_row already treats an empty candidate list as reject_pca (Lines 1059-1063), so no caller change is needed.
As per path instructions, "Prioritize correctness, memory safety, error handling, and thread-safety."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::sort(candidates.begin(), candidates.end(), | |
| [](const KalmanPca &lhs, const KalmanPca &rhs) | |
| { return lhs.dca < rhs.dca; }); | |
| candidates.erase( | |
| std::remove_if(candidates.begin(), candidates.end(), | |
| [](const KalmanPca &candidate) | |
| { return !std::isfinite(candidate.dca); }), | |
| candidates.end()); | |
| std::sort(candidates.begin(), candidates.end(), | |
| [](const KalmanPca &lhs, const KalmanPca &rhs) | |
| { return lhs.dca < rhs.dca; }); |
Source: Path instructions
Build & test reportReport for commit ebd9bfe21b36c0e189c2bfc97dc3ab8d7a23e86c:
Automatically generated by sPHENIX Jenkins continuous integration |
1 participant
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
This PR adds reusable TPC-only track-fitting utilities to
tpctrackrecoand aFun4All V0 reconstruction and diagnostics module to
TrackingDiagnostics.The new functionality includes:
(x, y, z, phi, q/pT, tan(lambda)), with:(r, rphi, z)measurement uncertainties;PHFieldfield map;TpcV0CandidateTree, a Fun4All diagnostics module that:Armenteros-Podolanski observables;
The existing tracking workflow is unchanged unless the new module is explicitly
registered and configured.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation / Context
Adds reusable TPC-only track-fitting utilities and an opt-in Fun4All module for reconstructing and diagnosing two-track V0 candidates, enabling detailed tracking QA without changing existing workflows.
Key Changes
TpcV0CandidateTreefor:Potential Risk Areas
Possible Future Improvements