Alignment updates - #4413
Alignment updates#4413gregottino wants to merge 26 commits into
Conversation
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR extends helical fitting, alignment-state export, cosmic seeding, silicon–TPC matching, tracking diagnostics, and source-link configuration. It adds cosmic and MVTX-half handling, track-quality and vertex filtering, pull accumulation, pseudo-measurements, and expanded tracking data. ChangesHelical fitting and Millepede alignment
Cosmic seed reconstruction
Silicon–TPC matching module
Tracking fitter and diagnostics
Sequence Diagram(s)sequenceDiagram
participant PHSiliconTpcTrackMatchingDummy
participant TrackSeedContainers
participant WindowMatcher
PHSiliconTpcTrackMatchingDummy->>TrackSeedContainers: read silicon and TPC seeds
PHSiliconTpcTrackMatchingDummy->>WindowMatcher: evaluate charge- and pT-dependent windows
PHSiliconTpcTrackMatchingDummy->>TrackSeedContainers: insert dummy and combined seed entries
PHSiliconTpcTrackMatchingDummy->>TrackSeedContainers: remove rejected z matches
sequenceDiagram
participant ActsAlignmentStates
participant SvtxAlignmentState_v1
participant MakeMilleFiles
ActsAlignmentStates->>SvtxAlignmentState_v1: store derivatives, pseudo errors, and six ACTS parameters
SvtxAlignmentState_v1->>MakeMilleFiles: provide alignment state data
MakeMilleFiles->>MakeMilleFiles: emit pseudo-measurements and ntuple fields
Merge Risk: 🟠 High · up to The current changes can misweight or discard alignment constraints, produce invalid fits, corrupt diagnostic output, and mishandle several tracking configurations. These issues should be resolved before merge. 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: 11
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Team
Run ID: 99aa5323-deac-4b49-a339-2d4d8f9d7473
📒 Files selected for processing (22)
offline/packages/TrackerMillepedeAlignment/HelicalFitter.ccoffline/packages/TrackerMillepedeAlignment/HelicalFitter.hoffline/packages/TrackerMillepedeAlignment/MakeMilleFiles.ccoffline/packages/TrackerMillepedeAlignment/MakeMilleFiles.hoffline/packages/TrackingDiagnostics/TrackResiduals.ccoffline/packages/TrackingDiagnostics/TrackResiduals.hoffline/packages/TrackingDiagnostics/TrackSeedTrackMapConverter.ccoffline/packages/trackbase/TrackFitUtils.ccoffline/packages/trackbase/TrackFitUtils.hoffline/packages/trackbase_historic/SvtxAlignmentState.ccoffline/packages/trackbase_historic/SvtxAlignmentState.hoffline/packages/trackbase_historic/SvtxAlignmentState_v1.ccoffline/packages/trackbase_historic/SvtxAlignmentState_v1.hoffline/packages/trackreco/ActsAlignmentStates.ccoffline/packages/trackreco/ActsAlignmentStates.hoffline/packages/trackreco/Makefile.amoffline/packages/trackreco/PHActsTrkFitter.ccoffline/packages/trackreco/PHActsTrkFitter.hoffline/packages/trackreco/PHCosmicSeeder.ccoffline/packages/trackreco/PHCosmicSeeder.hoffline/packages/trackreco/PHSiliconTpcTrackMatchingDummy.ccoffline/packages/trackreco/PHSiliconTpcTrackMatchingDummy.h
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| virtual void set_local_derivative_psuedo_matrix(const LocalMatrixPsuedo&) {} | ||
| //setter for eigen vals only called in above func for psuedo matrix | ||
| virtual void set_local_psuedo_measurement_err(const LocalMeasErrPsuedo&) {} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/sphenix-collaboration-coresoftware-9416dca5 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- target header and related implementation files ---'
cat -n offline/packages/trackbase_historic/SvtxAlignmentState.h | sed -n '1,180p'
find offline/packages/trackbase_historic -maxdepth 1 -type f -print | sort | grep 'SvtxAlignmentState'
printf '%s\n' '--- declarations and pseudo API uses ---'
rg -n -C 3 'Local(MatrixPsuedo|MeasErrPsuedo)|set_local_(derivative|psuedo)|get_local_(derivative|psuedo)' offline/packages/trackbase_historicRepository: sPHENIX-Collaboration/coresoftware
Length of output: 11634
🏁 Script executed:
printf '%s\n' '--- repository-wide convention ---'
cat /tmp/coderabbit-repo-knowledge/sphenix-collaboration-coresoftware-9416dca5/conventions/repo-wide.md
printf '%s\n' '--- concrete v1 declaration and implementation ---'
cat -n offline/packages/trackbase_historic/SvtxAlignmentState_v1.h
cat -n offline/packages/trackbase_historic/SvtxAlignmentState_v1.cc
printf '%s\n' '--- pseudo setter/getter call sites and concrete types ---'
rg -n -C 5 'set_local_derivative_psuedo_matrix|set_local_psuedo_measurement_err|get_local_derivative_psuedo_matrix|get_local_psuedo_measurement_err|SvtxAlignmentState_v1' offline | head -240
printf '%s\n' '--- target diff summary and scoped diff ---'
git diff --stat -- offline/packages/trackbase_historic/SvtxAlignmentState.h
git diff -- offline/packages/trackbase_historic/SvtxAlignmentState.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 24185
🏁 Script executed:
printf '%s\n' '--- pseudo-value construction and state insertion ---'
cat -n offline/packages/trackreco/ActsAlignmentStates.cc | sed -n '155,240p'
printf '%s\n' '--- consumer state type and output path ---'
cat -n offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc | sed -n '610,675p'
rg -n -C 4 'get_alignment_state|SvtxAlignmentStateMap|alignmentstate|statevec' offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc offline/packages/trackbase_historic/SvtxAlignmentStateMap_v1.h offline/packages/trackbase_historic/SvtxAlignmentStateMap_v1.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 22825
Store pseudo-measurements in SvtxAlignmentState_v1.
ActsAlignmentStates.cc writes both values, but SvtxAlignmentState_v1 inherits the base no-op setters and zero-valued getters. MakeMilleFiles therefore emits zero pseudo-measurements. Add members and setter/getter overrides to each concrete alignment-state implementation.
Source: Path instructions
| bool cross_mvtx_half = false; | ||
| if ((mvtx_east || mvtx_west)) | ||
| { | ||
| cross_mvtx_half = TrackFitUtils::isTrackCrossMvtxHalf(cluskey_vec); | ||
| } | ||
| else | ||
| { | ||
| circle_fit_pars = TrackFitUtils::circle_fit_by_taubin(global_vec); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The MVTX-half branch never performs the circle fit, so the returned helix has radius 0.
When mvtx_east or mvtx_west is set, the code only evaluates isTrackCrossMvtxHalf. circle_fit_pars keeps its value-initialised contents, so lines 687-689 push R = 0, x0 = 0, y0 = 0 into fitpars. Downstream, get_circle_point_pca(0, 0, 0, global) returns the circle origin, and pca / pca.norm() in get_helix_pca (line 596) then divides by zero, so the PCA and tangent become NaN.
fitClustersZeroField handles the same case correctly: it recomputes xy_fit_pars from global_vec_noINTT after the MVTX-half selection (lines 750-753). Apply the same treatment here so the circle fit uses the selected clusters.
🐛 Proposed fix: fit the circle to the selected clusters
@@
if (global_vec_noINTT.size() < 3)
{
return fitpars;
}
+ if (mvtx_east || mvtx_west)
+ {
+ circle_fit_pars = TrackFitUtils::circle_fit_by_taubin(global_vec_noINTT);
+ }
std::tuple<double, double> line_fit_pars;📝 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.
| bool cross_mvtx_half = false; | |
| if ((mvtx_east || mvtx_west)) | |
| { | |
| cross_mvtx_half = TrackFitUtils::isTrackCrossMvtxHalf(cluskey_vec); | |
| } | |
| else | |
| { | |
| circle_fit_pars = TrackFitUtils::circle_fit_by_taubin(global_vec); | |
| } | |
| if (global_vec_noINTT.size() < 3) | |
| { | |
| return fitpars; | |
| } | |
| if (mvtx_east || mvtx_west) | |
| { | |
| circle_fit_pars = TrackFitUtils::circle_fit_by_taubin(global_vec_noINTT); | |
| } |
| { | ||
| fitpars = TrackFitUtils::fitClusters(global_vec, cluskey_vec, use_intt_zfit); // do helical fit | ||
| fitpars = TrackFitUtils::fitClusters(global_vec, cluskey_vec, use_intt_zfit,false,false,is_cosmics); // do helical fit | ||
| fitpars_mvtx_half = TrackFitUtils::fitClusters(global_vec, cluskey_vec, use_intt_zfit, mvtx_east_only, mvtx_west_only, is_cosmics); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable conventions ---'
find /tmp/coderabbit-repo-knowledge/sphenix-collaboration-coresoftware-9416dca5 -type f -name '*.md' -print
printf '%s\n' '--- target outline ---'
ast-grep outline offline/packages/TrackerMillepedeAlignment/HelicalFitter.cc --view compact
printf '%s\n' '--- relevant source ranges ---'
sed -n '300,355p;400,435p;635,680p;1135,1180p' offline/packages/TrackerMillepedeAlignment/HelicalFitter.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 954
🏁 Script executed:
#!/bin/bash
set -eu
find /tmp/coderabbit-repo-knowledge/sphenix-collaboration-coresoftware-9416dca5 -type f -name '*.md' -print
sed -n '300,355p;400,435p;635,680p' offline/packages/TrackerMillepedeAlignment/HelicalFitter.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 6131
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable conventions ---'
cat /tmp/coderabbit-repo-knowledge/sphenix-collaboration-coresoftware-9416dca5/conventions/repo-wide.md
printf '%s\n' '--- bound declarations and definitions ---'
rg -n -A35 -B8 'fitClusters\(|get_helix_surface_intersection|TrackFitUtils' offline/packages/TrackerMillepedeAlignment --glob '*.{h,cc,cpp,cxx,c}'
printf '%s\n' '--- call sites of the intersection helper ---'
rg -n -A8 -B8 'get_helix_surface_intersection\(' offline/packages/TrackerMillepedeAlignment/HelicalFitter.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact helper locations ---'
rg -n '^(Acts::Vector3 )?HelicalFitter::get_helix_surface_intersection|^std::pair<Acts::Vector3, Acts::Vector3> HelicalFitter::get_helix_tangent' offline/packages/TrackerMillepedeAlignment/HelicalFitter.cc
printf '%s\n' '--- TrackFitUtils files ---'
fd -i 'TrackFitUtils' . | head -20
printf '%s\n' '--- fitClusters declarations/definitions ---'
rg -l 'fitClusters\(' offline | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1411
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1235,1345p;1435,1495p' offline/packages/TrackerMillepedeAlignment/HelicalFitter.cc
printf '%s\n' '--- TrackFitUtils declarations ---'
rg -n -A12 -B5 'fitClusters\(' offline/packages/trackbase/TrackFitUtils.h
printf '%s\n' '--- TrackFitUtils implementations ---'
rg -n 'fitClusters' offline/packages/trackbase/TrackFitUtils.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 9185
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '610,700p' offline/packages/trackbase/TrackFitUtils.cc
rg -n -A55 -B8 'TrackFitUtils::get_helix_tangent' offline/packages/trackbase/TrackFitUtils.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 6304
Add the fallback for fitpars_mvtx_half after the full-track helical refit.
TrackFitUtils::fitClusters returns an empty vector when fewer than three selected points remain. Line 662 then passes this vector to get_helix_surface_intersection, whose TrackFitUtils::get_helix_tangent reads fitpars[0] through fitpars[4] without checks. This can cause out-of-bounds access. Reuse the existing fallback to fitpars.
Source: Path instructions
| abs_cross = INT_MAX; | ||
| if(use_event_vertex) | ||
| { | ||
| for (const auto& [vtxkey, vertex] : *m_vertexmap) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard m_vertexmap before iteration.
GetNodes logs a missing SvtxVertexMap but returns success. When use_event_vertex is enabled, Line 547 dereferences m_vertexmap without a null check. This crashes the event when the node is absent. A fixed vertex does not avoid this access because its override occurs after the loop.
Reject this configuration during node setup or handle the missing map before iterating.
As per path instructions, prioritize correctness, memory safety, and error handling.
Source: Path instructions
|
|
||
| if (!isnan(vtx_residual(0))) | ||
| bool pull_cumulative_pass = true; | ||
| if (pull_cumulative<2000) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the same pull threshold for vertex measurements.
Cluster Mille measurements are disabled when pull_cumulative is above 2000 at Lines 1040-1043. This condition disables vertex measurements when pull_cumulative is below 2000. As a result, clean tracks lose vertex constraints and high-pull tracks retain them.
Invert this comparison.
Proposed fix
-if (pull_cumulative<2000)
+if (pull_cumulative>2000)
pull_cumulative_pass = false;As per path instructions, prioritize correctness and error handling.
📝 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.
| if (pull_cumulative<2000) | |
| if (pull_cumulative>2000) | |
| pull_cumulative_pass = false; |
Source: Path instructions
| m_pcax_vtx_trk.push_back(track->get_x()); | ||
| m_pcay_vtx_trk.push_back(track->get_y()); | ||
| m_pcaz_vtx_trk.push_back(track->get_z()); | ||
| m_px_vtx_trk.push_back(track->get_px()); | ||
| m_py_vtx_trk.push_back(track->get_py()); | ||
| m_pz_vtx_trk.push_back(track->get_pz()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset the vertex-track vectors for each tree entry.
These vectors are appended here, but clearClusterStateVectors() does not clear them. The reset before the vertex loop and the reset after event processing therefore leave data from earlier vertices and events in the vectors. m_ntracks then describes only the current vertex, while the new branches contain unrelated entries.
Clear all six vectors before filling each vertex and at the event boundary.
As per path instructions, this C++ correctness issue is raised at Major severity.
Proposed reset
void TrackResiduals::clearClusterStateVectors()
{
+ m_pcax_vtx_trk.clear();
+ m_pcay_vtx_trk.clear();
+ m_pcaz_vtx_trk.clear();
+ m_px_vtx_trk.clear();
+ m_py_vtx_trk.clear();
+ m_pz_vtx_trk.clear();
}Also clear the six vectors at the start of each vertex iteration.
Source: Path instructions
| auto xyparams = TrackFitUtils::line_fit(xypoints); | ||
| auto yzLineParams = TrackFitUtils::line_fit(yzpoints); | ||
| auto fitpars = TrackFitUtils::fitClusters(global_vec, keys, false); | ||
| auto fitpars = TrackFitUtils::fitClusters(global_vec, keys, false,false,false,true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable repository guidance ---'
find /tmp/coderabbit-repo-knowledge/sphenix-collaboration-coresoftware-9416dca5 -type f -name '*.md' -maxdepth 3 -print
printf '%s\n' '--- target file context ---'
sed -n '500,630p' offline/packages/TrackingDiagnostics/TrackResiduals.cc
printf '%s\n' '--- relevant TrackResiduals definitions and callers ---'
rg -n -C 6 'clearClusterStateVectors|fillResidualTreeSeeds|fitClusters|circleFitClusters|is_cosmics|m_rzslope|m_rzint' offline/packages/TrackingDiagnostics/TrackResiduals.cc offline/packages/trackbaseRepository: sPHENIX-Collaboration/coresoftware
Length of output: 44747
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository convention and scoped learning ---'
cat /tmp/coderabbit-repo-knowledge/sphenix-collaboration-coresoftware-9416dca5/conventions/repo-wide.md
printf '%s\n' '--- target header/configuration declarations ---'
fd -i 'TrackResiduals' offline/packages/TrackingDiagnostics
rg -n -C 5 'cosmic|m_convertSeeds|m_linefitTPCOnly|circleFitClusters|fillClusterBranchesSeeds' offline/packages/TrackingDiagnostics
printf '%s\n' '--- seed-track fit branch and parameter consumption ---'
sed -n '2460,2710p' offline/packages/TrackingDiagnostics/TrackResiduals.cc
sed -n '1625,1670p' offline/packages/TrackingDiagnostics/TrackResiduals.cc
printf '%s\n' '--- resolved helix-intersection contract ---'
rg -n -C 12 'get_helix_surface_intersection' offline/packages/trackbase/TrackFitUtils.h offline/packages/trackbase/TrackFitUtils.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 42268
Pass the cosmic-mode flag to TrackFitUtils::fitClusters().
On the non-zero-field, non-alignment path, circleFitClusters() passes is_cosmics=true for every seed track. fitClusters() then derives m_rzslope and m_rzint from line_fit_xz(), but get_helix_surface_intersection() consumes them through the default non-cosmic radial-z interpretation. Standard collision tracks can therefore produce incorrect state intersections. Add an explicit cosmic flag with a false default, or pass false for standard tracks.
Sources: Path instructions, MCP tools
| if(clusterPositions.size()<3) | ||
| return Fun4AllReturnCodes::ABORTEVENT; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move the minimum-cluster check after MVTX INTT augmentation.
For MVTX processing, this check runs before lines 114-130 add qualified INTT clusters. An event with two MVTX clusters and sufficient INTT clusters aborts before it can satisfy the later combined four-cluster requirement. Evaluate the minimum after building the combined cluster map.
Proposed fix
- if(clusterPositions.size()<3)
- return Fun4AllReturnCodes::ABORTEVENT;
if (Verbosity() > 1)
{
std::cout<<"processing next event2"<<std::endl;
@@
if(m_trackerId==TrkrDefs::TrkrId::mvtxId)
{
// add qualified INTT clusters
}
+ if (clusterPositions.size() < 3)
+ {
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }| tpc_pt = fabs(1. / _tracklet_tpc->get_qOverR()) * (0.3 / 100.) * fieldstrength; | ||
| tpc_z = TrackSeedHelper::get_z(tpc_track); | ||
| tpc_q = _tracklet_tpc->get_charge(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unwired helpers carry pointer preconditions that this module no longer satisfies. checkZMatches and getBunchCrossing were copied from the production matching module, but process_event never calls them and never sets the members they assume. Both would crash on first use. Either delete them or fix the pointer handling now, before a later revision calls them.
offline/packages/trackreco/PHSiliconTpcTrackMatchingDummy.cc#L436-L438: replace_tracklet_tpcwith the localtpc_track, and null-check both seeds obtained from the containers.offline/packages/trackreco/PHSiliconTpcTrackMatchingDummy.cc#L268-L319: null-check_track_map->get(trid)before the cluster-key loop at line 282, and return the undefined-crossing sentinel when the seed is absent.
📍 Affects 1 file
offline/packages/trackreco/PHSiliconTpcTrackMatchingDummy.cc#L436-L438(this comment)offline/packages/trackreco/PHSiliconTpcTrackMatchingDummy.cc#L268-L319
| void set_negQoverpT_maxabs (const Arr3D& _negHi, const double _min_pt=0.25) | ||
| { posLo={100.,0.,0.}; negHi=_negHi; min_pt_negQ = _min_pt; }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
set_negQoverpT_maxabs resets the wrong array.
The function assigns posLo, not negLo. init_bools computes fabs_max_negQ from negLo[0]==100., so the requested |delta| < negHi mode is not activated when negLo was set earlier. The same statement also clobbers posLo, which silently converts an already configured positive-charge range into |delta| mode.
Proposed fix
// max for |deltaX| for neg Q
void set_negQoverpT_maxabs (const Arr3D& _negHi, const double _min_pt=0.25)
- { posLo={100.,0.,0.}; negHi=_negHi; min_pt_negQ = _min_pt; };
+ { negLo={100.,0.,0.}; negHi=_negHi; min_pt_negQ = _min_pt; };📝 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.
| void set_negQoverpT_maxabs (const Arr3D& _negHi, const double _min_pt=0.25) | |
| { posLo={100.,0.,0.}; negHi=_negHi; min_pt_negQ = _min_pt; }; | |
| void set_negQoverpT_maxabs (const Arr3D& _negHi, const double _min_pt=0.25) | |
| { negLo={100.,0.,0.}; negHi=_negHi; min_pt_negQ = _min_pt; }; |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/trackreco/PHActsTrkFitter.cc (1)
1299-1299: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the fitted direction and pz when applying
forceSiOnlyFitTpcSeedPT.When both flags are true,
PHActsTrkFitter::updateSvtxTrackreplaces all momentum components with the TPC seed values. The setter promises to use only the TPC seed pT. Scale the fitted transverse components to that pT and retain the fitted azimuth andparams.momentum()(2), or rename and document the option as a full-momentum replacement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Team
Run ID: 4fb38fc9-81f5-4b86-835b-0497107e63c6
📒 Files selected for processing (4)
offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.ccoffline/packages/trackreco/MakeSourceLinks.ccoffline/packages/trackreco/MakeSourceLinks.hoffline/packages/trackreco/PHActsTrkFitter.cc
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| double phierror = cluster->getRPhiError(); | ||
| double zerror = cluster->getZError(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve calibrated TPC uncertainties.
When m_ignore_tpc is false, these lines bypass _ClusErrPara.get_clusterv5_modified_error(...), which applies TPC layer, edge, overlap, and cluster-size corrections. Restore the calibrated error path, or define and validate the raw-error policy because the current code changes TPC measurement weights and can bias the alignment solution.
| short int crossing, | ||
| bool use_modified_clus_error = true); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge sPHENIX-Collaboration/coresoftware /tmp/coderabbit-repo-knowledge/sphenix-collaboration-coresoftware-9416dca5/conventions /tmp/coderabbit-repo-knowledge/sphenix-collaboration-coresoftware-9416dca5/learnings
Length of output: 3790
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- changed hunk ---'
git diff -- offline/packages/trackreco/MakeSourceLinks.h
printf '%s\n' '--- header context ---'
cat -n offline/packages/trackreco/MakeSourceLinks.h | sed -n '1,130p'
printf '%s\n' '--- declarations and definitions ---'
rg -n -C 4 'getSourceLinksClusterMover' offline/packages
printf '%s\n' '--- direct callers ---'
rg -n -C 3 'getSourceLinksClusterMover[[:space:]]*\\(' --glob '*.{cc,cpp,cxx,h,hpp,h,hh,hxx}' .Repository: sPHENIX-Collaboration/coresoftware
Length of output: 14013
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- implementation signature and affected branch ---'
cat -n offline/packages/trackreco/MakeSourceLinks.cc | sed -n '315,355p'
cat -n offline/packages/trackreco/MakeSourceLinks.cc | sed -n '525,555p'
printf '%s\n' '--- caller context ---'
cat -n offline/packages/trackreco/PHActsTrkFitter.cc | sed -n '600,635p'
printf '%s\n' '--- parent declaration, if available ---'
if git rev-parse --verify HEAD^ >/dev/null 2>&1; then
git show HEAD^:offline/packages/trackreco/MakeSourceLinks.h 2>/dev/null | sed -n '60,82p' || true
fi
printf '%s\n' '--- trackreco build/install bindings ---'
rg -n -C 3 'MakeSourceLinks|trackreco' offline/packages/trackreco/CMakeLists.txt offline/packages/trackreco 2>/dev/null | head -160
printf '%s\n' '--- repository status and commit ---'
git status --short
git rev-parse --short HEADRepository: sPHENIX-Collaboration/coresoftware
Length of output: 18590
Preserve binary compatibility for getSourceLinksClusterMover.
The implementation now provides only the seven-argument symbol. Existing binaries that call the previous six-argument signature can fail to link or load. Retain a compatibility overload that forwards true, or document the required downstream rebuilds.
Source: Path instructions
| this_crossing, | ||
| !m_forceSiOnlyFit); //for alignment tests, if forceSiOnlyFit is true, then we want to turn off the parametrized cluster errors when building source links |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate stored cluster errors through getSourceLinks.
When m_forceSiOnlyFit is true and m_use_clustermover is false, the silicon path still uses ClusterErrorPara::get_clusterv5_modified_error. Add the error-mode parameter to getSourceLinks and pass !m_forceSiOnlyFit; keep the TPC call unchanged.
Build & test reportReport for commit bd5618f5122a002e46d7514df4c68d294f318124:
Automatically generated by sPHENIX Jenkins continuous integration |
2 participants
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)
Motivation / context
This PR addresses INTT errors in silicon-only track fits used for alignment. It adds alignment support for cosmic and silicon tracks while preserving existing workflows where possible.
Key changes
SvtxAlignmentState.TrackResiduals.PHSiliconTpcTrackMatchingDummyfor silicon–TPC matching studies.Potential risk areas
Possible future improvements
psuedoidentifiers.AI-generated summaries can contain mistakes. Contributors should verify these points against the implementation and test results.