feat: update to actsv45.0.0#4272
Conversation
…e into actsv44.0.0
…e into actsv44.0.0
…e into actsv44.0.0
…e into actsv45.0.0
📝 WalkthroughWalkthroughThis PR migrates sPHENIX tracking code to updated Acts APIs: replaces surface transform calls with localToGlobalTransform(), standardizes measurement and seed container types, updates propagator/options and TrackParameters argument ordering, initializes geometry contexts explicitly, refactors seeding to use Acts space-point proxies, and adjusts Makefiles/includes for ActsPlugins. ChangesActs Surface Transform and Geometry Coordinate API Migration
Acts Measurement Container and Track Indexing Standardization
Acts Space Point Proxy and Seeding Infrastructure Modernization
Geometry Construction and Propagator Options Refactoring
Interface Simplification and Build System Updates
Possibly Related PRs
Be concise and safe in follow-up questions — I can expand any layer’s ranges into file/line detail on request. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
offline/packages/trackbase/sPHENIXActsDetectorElement.cc (1)
17-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd null/end guards before dereferencing alignment context and layer map.
Line 17 and Lines 20-23 can dereference invalid iterators/pointers and hard-crash when geometry/context is incomplete. Please fail gracefully before accessing these objects.
As per coding guidelines "**/*.{cc,cpp,cxx,c}: Prioritize correctness, memory safety, error handling, and thread-safety."Proposed fix
- unsigned int sphlayer = base_layer_map.find(volume)->second + layer / 2 - 1; + const auto baseIt = base_layer_map.find(volume); + if (baseIt == base_layer_map.end()) + { + std::cout << PHWHERE << " Missing base_layer_map entry for volume " << volume << std::endl; + exit(1); + } + unsigned int sphlayer = baseIt->second + layer / 2 - 1; @@ - const alignmentTransformationContainer* transformContainer = ctxt.get<alignmentTransformationContainer*>(); + const alignmentTransformationContainer* transformContainer = ctxt.get<alignmentTransformationContainer*>(); + if (!transformContainer) + { + std::cout << PHWHERE << " Missing alignmentTransformationContainer in geometry context" << std::endl; + exit(1); + }Also applies to: 20-23
offline/packages/trackbase/ActsGsfTrackFittingAlgorithm.h (1)
150-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve source compatibility for the new GSF parameter.
This new required parameter breaks every existing caller of
makeGsfFitterFunction(...), including ones that previously relied on the default logger argument. Please either default it here to the existing internal value or document the migration before merge. As per coding guidelines:If interfaces change, ask for compatibility notes and any needed downstream updates.🔧 Compatibility-preserving change
makeGsfFitterFunction( const std::shared_ptr<const Acts::TrackingGeometry>& trackingGeometry, std::shared_ptr<const Acts::MagneticFieldProvider> magneticField, BetheHeitlerApprox betheHeitlerApprox, std::size_t maxComponents, double weightCutoff, MixtureReductionAlgorithm finalReductionMethod, bool abortOnError, - bool disableAllMaterialHandling, double reverseFilteringCovarianceScaling, + bool disableAllMaterialHandling, + double reverseFilteringCovarianceScaling = 100., const Acts::Logger& logger = *Acts::getDefaultLogger("GSF", Acts::Logging::FATAL));offline/packages/trackreco/ActsAlignmentStates.cc (1)
128-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd dimension check before processing measurements.
The code mixes dynamic dimension handling (
state.calibratedSize()) with static 2D assumptions. Lines 131–132 unconditionally readmeasurement.parameters()[0]and[1], while line 146 forces the projected state intoActs::Vector2. Although sPHENIX currently creates only 2D measurements, ACTS supports arbitrary dimensions including 1D. If 1D calibrated measurements ever reach this code path, it will either read past the measurement boundary or trigger an Eigen size assertion.🛡️ Suggested guard
const auto measurement = measurements.getMeasurement(sl.index()); +if (state.calibratedSize() != 2) +{ + return true; +} localMeas(0) = measurement.parameters()[0]; localMeas(1) = measurement.parameters()[1];offline/packages/trackbase/IBaseDetector.h (1)
35-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument the
IBaseDetectorAPI break for downstream detectors.Removing
finalize(...)from this installed interface is a source-breaking change for any external detector implementation built againsttrackbaseheaders. Please add migration notes or a compatibility shim before merge so downstream integrations know how to adapt.As per coding guidelines:
**/*.{h,hpp,hxx,hh}: “If interfaces change, ask for compatibility notes and any needed downstream updates.”offline/packages/trackreco/PHActsSiliconSeeding.cc (1)
204-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe widened MVTX strobe window will duplicate seeds across neighboring passes.
runSeeder()already iterates each strobe independently, but this filter now feeds hits fromstrobe - 1,strobe, andstrobe + 1into every pass. Becausem_seedContaineris only reset once per event and there is no dedup before insert, the same MVTX triplet can be seeded multiple times and paired with different INTT combinations.Suggested fix
- if (abs(strobeId - strobe) > 1) + if (strobeId != strobe) { continue; }Also applies to: 1431-1434
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4bbf95d2-f56f-43f3-b70b-f9e91d9bbfbf
📒 Files selected for processing (60)
calibrations/tpc/TpcDVCalib/Makefile.amcalibrations/tpc/TpcDVCalib/TrackToCalo.ccoffline/QA/Tracking/CosmicTrackQA.ccoffline/packages/TrackerMillepedeAlignment/HelicalFitter.ccoffline/packages/TrackerMillepedeAlignment/MakeMilleFiles.ccoffline/packages/TrackerMillepedeAlignment/MakeMilleFiles.hoffline/packages/TrackingDiagnostics/TrackResiduals.ccoffline/packages/intt/CylinderGeomInttHelper.ccoffline/packages/micromegas/CylinderGeomMicromegas.ccoffline/packages/mvtx/CylinderGeom_MvtxHelper.ccoffline/packages/tpc/LaserClusterizer.ccoffline/packages/tpc/TpcClusterizer.ccoffline/packages/trackbase/ActsGeometry.hoffline/packages/trackbase/ActsGsfTrackFittingAlgorithm.hoffline/packages/trackbase/ActsTrackFittingAlgorithm.hoffline/packages/trackbase/ActsTrackingGeometry.hoffline/packages/trackbase/AlignmentTransformation.ccoffline/packages/trackbase/Calibrator.ccoffline/packages/trackbase/Calibrator.hoffline/packages/trackbase/IBaseDetector.hoffline/packages/trackbase/MagneticFieldOptions.ccoffline/packages/trackbase/MagneticFieldOptions.hoffline/packages/trackbase/Makefile.amoffline/packages/trackbase/ResidualOutlierFinder.hoffline/packages/trackbase/SpacePoint.hoffline/packages/trackbase/TGeoDetectorWithOptions.ccoffline/packages/trackbase/TGeoDetectorWithOptions.hoffline/packages/trackbase/TrackFittingAlgorithmFunctionsGsf.ccoffline/packages/trackbase/alignmentTransformationContainer.hoffline/packages/trackbase/sPHENIXActsDetectorElement.ccoffline/packages/trackbase/sPHENIXActsDetectorElement.hoffline/packages/trackbase_historic/ActsTransformations.ccoffline/packages/trackbase_historic/ActsTransformations.hoffline/packages/trackbase_historic/Makefile.amoffline/packages/trackbase_historic/TrackAnalysisUtils.ccoffline/packages/trackbase_historic/TrackAnalysisUtils.hoffline/packages/trackreco/ActsAlignmentStates.ccoffline/packages/trackreco/ActsAlignmentStates.hoffline/packages/trackreco/ActsEvaluator.ccoffline/packages/trackreco/ActsEvaluator.hoffline/packages/trackreco/ActsPropagator.ccoffline/packages/trackreco/ActsPropagator.hoffline/packages/trackreco/MakeActsGeometry.ccoffline/packages/trackreco/MakeActsGeometry.hoffline/packages/trackreco/MakeSourceLinks.ccoffline/packages/trackreco/MakeSourceLinks.hoffline/packages/trackreco/Makefile.amoffline/packages/trackreco/PHActsGSF.ccoffline/packages/trackreco/PHActsGSF.hoffline/packages/trackreco/PHActsKDTreeSeeding.ccoffline/packages/trackreco/PHActsKDTreeSeeding.hoffline/packages/trackreco/PHActsSiliconSeeding.ccoffline/packages/trackreco/PHActsSiliconSeeding.hoffline/packages/trackreco/PHActsTrkFitter.ccoffline/packages/trackreco/PHActsTrkFitter.hoffline/packages/trackreco/PHCosmicsTrkFitter.ccoffline/packages/trackreco/PHCosmicsTrkFitter.hoffline/packages/trackreco/WeightedFitter.ccsimulation/g4simulation/g4tpc/PHG4TpcPadPlaneReadout.ccsimulation/g4simulation/g4tpc/TpcClusterBuilder.cc
💤 Files with no reviewable changes (3)
- calibrations/tpc/TpcDVCalib/Makefile.am
- offline/packages/trackbase/ActsGeometry.h
- offline/packages/trackreco/PHActsGSF.h
| const auto cov = fixedMeasurement.covariance(); | ||
| const TrkrDefs::cluskey cluskey = sl.cluskey(); | ||
| const uint8_t layer = TrkrDefs::getLayer(cluskey); | ||
| const double misalignmentFactor = gctx.get<alignmentTransformationContainer*>()->getMisalignmentFactor(layer); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that all call paths initializing/using GeometryContext
# consistently populate alignmentTransformationContainer*.
rg -n -C3 'get<\s*alignmentTransformationContainer\s*\*>'
rg -n -C3 'set<\s*alignmentTransformationContainer\s*\*>'
rg -n -C3 'GeometryContext' | rg -n 'alignment|transform|set<|get<'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3130
🏁 Script executed:
# Search for alignmentTransformationContainer initialization, setup, or registration
rg -n 'alignmentTransformationContainer' --type cc --type h -B2 -A2 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 105
🏁 Script executed:
# Look for GeometryContext initialization/setup patterns
rg -n 'GeometryContext\(\)' --type cc --type h -A5 | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 105
🏁 Script executed:
# Search for any null checks or guards on geometry context extensions
rg -n 'get<.*>\(' --type cc --type h | rg -E 'if|nullptr|null|check|guard' -B2 -A2 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 502
🏁 Script executed:
# Check if there's any documentation or header comments about this API
fd -e h -e hpp | xargs grep -l 'alignmentTransformationContainer' | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 308
🏁 Script executed:
# Search for alignmentTransformationContainer initialization/registration
rg -n 'alignmentTransformationContainer' -B3 -A3 | head -150Repository: sPHENIX-Collaboration/coresoftware
Length of output: 13707
🏁 Script executed:
# Look at the Calibrator.cc file more carefully
cat offline/packages/trackbase/Calibrator.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2299
🏁 Script executed:
# Check the AlignmentTransformation.h header
cat offline/packages/trackbase/AlignmentTransformation.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 4790
🏁 Script executed:
# Search for set< patterns to find where extensions are registered
rg 'set<.*alignmentTransformationContainer' -B2 -A2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Search for any defensive checks or nullptr handling with geometry context
rg 'get<.*\*>\(\)' -A1 | grep -E 'if|nullptr|!=|==|assert' -B1 -A1 | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Look for where alignmentTransformationContainer is registered to GeometryContext
rg 'gctx.*set<|GeometryContext.*set<' -B2 -A2 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check Calibrator.h header for documentation
cat offline/packages/trackbase/Calibrator.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1518
🏁 Script executed:
# Search for calls to Calibrator::calibrate to understand preconditions
rg 'Calibrator::calibrate|\.calibrate\(' -B3 -A3 | head -150Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1193
🏁 Script executed:
# Look for where the geometry context with extension is created/passed
rg 'getGeoContext\(\)' -B2 -A5 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 8495
Guard or document the geometry-context extension requirement.
Line 23 dereferences gctx.get<alignmentTransformationContainer*>() unconditionally. This pattern appears in several files (MakeSourceLinks.cc, sPHENIXActsDetectorElement.cc), but lacks explicit checks. Calibrator.h does not document whether this extension is guaranteed to exist. If absent on any calibration path, this becomes a null-pointer dereference.
Either (1) add a safety check with a fallback (e.g., misalignmentFactor = 1.0), or (2) add a header comment documenting that the caller must ensure the extension is registered in gctx before calling calibrate().
| Acts::Vector3 fitpoint_local = surf->localToGlobalTransform(_tGeometry->geometry().getGeoContext()).inverse() * (fitpoint * Acts::UnitConstants::cm); | ||
| Acts::Vector3 fitpoint_mvtx_half_local = surf->localToGlobalTransform(_tGeometry->geometry().getGeoContext()).inverse() * (fitpoint_mvtx_half * Acts::UnitConstants::cm); |
There was a problem hiding this comment.
Guard fitpoint_mvtx_half_local behind straight_line_fit.
fitpoint_mvtx_half is only assigned in the straight-line branch, but Lines 597-598 always transform it. On the helical-fit path this is an uninitialized read and undefined behavior.
🐛 Proposed fix
Acts::Vector3 fitpoint_local = surf->localToGlobalTransform(_tGeometry->geometry().getGeoContext()).inverse() * (fitpoint * Acts::UnitConstants::cm);
-Acts::Vector3 fitpoint_mvtx_half_local = surf->localToGlobalTransform(_tGeometry->geometry().getGeoContext()).inverse() * (fitpoint_mvtx_half * Acts::UnitConstants::cm);
fitpoint_local /= Acts::UnitConstants::cm;
-fitpoint_mvtx_half_local /= Acts::UnitConstants::cm;
+Acts::Vector3 fitpoint_mvtx_half_local = fitpoint_local;
+if (straight_line_fit)
+{
+ fitpoint_mvtx_half_local =
+ surf->localToGlobalTransform(_tGeometry->geometry().getGeoContext()).inverse() *
+ (fitpoint_mvtx_half * Acts::UnitConstants::cm);
+ fitpoint_mvtx_half_local /= Acts::UnitConstants::cm;
+}📝 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.
| Acts::Vector3 fitpoint_local = surf->localToGlobalTransform(_tGeometry->geometry().getGeoContext()).inverse() * (fitpoint * Acts::UnitConstants::cm); | |
| Acts::Vector3 fitpoint_mvtx_half_local = surf->localToGlobalTransform(_tGeometry->geometry().getGeoContext()).inverse() * (fitpoint_mvtx_half * Acts::UnitConstants::cm); | |
| Acts::Vector3 fitpoint_local = surf->localToGlobalTransform(_tGeometry->geometry().getGeoContext()).inverse() * (fitpoint * Acts::UnitConstants::cm); | |
| fitpoint_local /= Acts::UnitConstants::cm; | |
| Acts::Vector3 fitpoint_mvtx_half_local = fitpoint_local; | |
| if (straight_line_fit) | |
| { | |
| fitpoint_mvtx_half_local = | |
| surf->localToGlobalTransform(_tGeometry->geometry().getGeoContext()).inverse() * | |
| (fitpoint_mvtx_half * Acts::UnitConstants::cm); | |
| fitpoint_mvtx_half_local /= Acts::UnitConstants::cm; | |
| } |
| void ActsAlignmentStates::fillAlignmentStateMap( | ||
| const ActsTrackFittingAlgorithm::TrackContainer& tracks, | ||
| const std::vector<Acts::MultiTrajectoryTraits::IndexType>& tips, | ||
| const std::vector<Acts::TrackIndexType>& tips, | ||
| SvtxTrack* track, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ACTS v45 TrackContainer, if a fit result returns std::vectorActs::TrackIndexType, what should be passed to MultiTrajectory::visitBackwards(...)—the track index itself or the track's tip state index?
💡 Result:
In ACTS, the MultiTrajectory::visitBackwards method requires the index of the last state (the tip state index) to begin the backward iteration [1][2][3]. It does not take the track index itself. When working with a container where you have obtained a track index (e.g., from a fit result or a track container), you must first retrieve the corresponding trajectory tip index associated with that track before calling visitBackwards. In the context of ActsExamples::Trajectories, for example, the tip indices are stored separately and used to access the trajectory states within the MultiTrajectory [4]. Therefore, you should pass the track's tip state index, not the track index. [1][2][3]
Citations:
- 1: https://acts.readthedocs.io/en/v28.2.0/api/class/class_acts_1_1_multi_trajectory.html
- 2: https://acts.readthedocs.io/en/v24.0.0/api/file/_multi_trajectory_8hpp.html
- 3: https://acts.readthedocs.io/en/v24.0.0/api/class/class_acts_1_1_multi_trajectory.html
- 4: https://eic-code-browser.sdcc.bnl.gov/lxr/source/acts/Examples/Framework/include/ActsExamples/EventData/Trajectories.hpp
🏁 Script executed:
wc -l offline/packages/trackreco/ActsAlignmentStates.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 132
🏁 Script executed:
sed -n '50,75p' offline/packages/trackreco/ActsAlignmentStates.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 971
🏁 Script executed:
sed -n '110,150p' offline/packages/trackreco/ActsAlignmentStates.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1541
🏁 Script executed:
rg "trackStateContainer|tip|Tip" offline/packages/trackreco/ActsAlignmentStates.cc -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 281
🏁 Script executed:
grep -n "TrackContainer\|trackContainer" offline/packages/trackreco/ActsAlignmentStates.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 140
🏁 Script executed:
sed -n '55,70p' offline/packages/trackreco/ActsAlignmentStates.h 2>/dev/null || echo "Header file not found, searching..."Repository: sPHENIX-Collaboration/coresoftware
Length of output: 620
🏁 Script executed:
fd ActsAlignmentStates offline/packages/trackreco/ -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 249
🏁 Script executed:
find offline/packages/trackreco -name "*ActsAlignmentStates*"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 177
🏁 Script executed:
rg "tips\[|tracks\.track|trackStateContainer" offline/packages/trackreco/ActsAlignmentStates.cc -B2 -A2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 306
🏁 Script executed:
sed -n '56,65p' offline/packages/trackreco/ActsAlignmentStates.cc | cat -ARepository: sPHENIX-Collaboration/coresoftware
Length of output: 561
🏁 Script executed:
grep -n "index()" offline/packages/trackreco/ActsAlignmentStates.cc | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 218
🏁 Script executed:
sed -n '128,148p' offline/packages/trackreco/ActsAlignmentStates.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 880
🏁 Script executed:
sed -n '140,150p' offline/packages/trackreco/ActsAlignmentStates.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 479
Resolve the actual tip-state index before calling visitBackwards.
After switching tips to Acts::TrackIndexType, line 116 passes trackTip (a track index) directly to mj.visitBackwards(...). According to Acts API, visitBackwards requires the tip state index, not the track index. You must first retrieve the trajectory tip index associated with that track before calling visitBackwards.
Also applies to: lines 63–64, 116–123.
Remove hard-coded 2D assumption for measurements and states.
Lines 128–132 hard-code Acts::Vector2 for local measurements, reading exactly two parameters. However, line 143 uses state.calibratedSize() to dynamically determine the projector dimensions, and line 146 casts the result into a fixed Acts::Vector2. This creates a dimension mismatch: if calibratedSize() == 1 (1D strip detector), the extraction at lines 131–132 and the state vector at line 146 will fail or produce incorrect results. Use calibratedSize() consistently for both measurement extraction and state construction.
Also applies to: lines 128–146.
| auto trackingGeometry = m_geometry->geometry().tGeometry; | ||
| Stepper stepper(field, m_overstepLimit); | ||
| Stepper stepper(field); | ||
| Acts::Navigator::Config cfg{trackingGeometry}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ActsPropagator configuration ---'
sed -n '1,220p' offline/packages/trackreco/ActsPropagator.h
echo
echo '--- overstep-related references ---'
rg -n -C2 'm_overstepLimit|overstep' offline/packages/trackreco/ActsPropagator.h offline/packages/trackreco/ActsPropagator.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 4652
🏁 Script executed:
# Read the makePropagator() implementation around lines 233-235
sed -n '225,245p' offline/packages/trackreco/ActsPropagator.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 651
🏁 Script executed:
# Also search for how Stepper is constructed elsewhere and if overstepLimit is used
rg -n 'Stepper\s*\(' offline/packages/trackreco/ActsPropagator.cc -A2 -B2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check if overstepLimit is applied elsewhere in the propagation setup
rg -n 'overstepLimit|overstep' offline/packages/trackreco/ActsPropagator.cc -A3 -B3Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check the full makePropagator() function to see if overstepLimit is used elsewhere
sed -n '220,280p' offline/packages/trackreco/ActsPropagator.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1908
🏁 Script executed:
# Search for overstep in the entire file context
cat offline/packages/trackreco/ActsPropagator.cc | grep -n -i 'overstep'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check Acts Propagator/Stepper options to see if overstepLimit can be set there
rg -i 'stepper.*options|options.*stepper' offline/packages/trackreco/ -A5 -B5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Find the makeFastPropagator implementation
sed -n '200,235p' offline/packages/trackreco/ActsPropagator.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1110
🏁 Script executed:
# Search for any Acts-related includes or documentation that might show Stepper signature
head -50 offline/packages/trackreco/ActsPropagator.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2126
🏁 Script executed:
# Look for any other files that construct EigenStepper to understand the pattern
fd -type f -name '*.cc' -o -name '*.cpp' | head -20 | xargs rg 'EigenStepper' -B2 -A2 2>/dev/null | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 6653
🏁 Script executed:
# Search for PropagatorOptions and overstep in the codebase
rg 'PropagatorOptions|overstep' offline/packages/trackreco/ --type cc --type hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 105
🏁 Script executed:
# Check if m_overstepLimit is actually used anywhere in the class methods
grep -n 'm_overstepLimit' offline/packages/trackreco/ActsPropagator.cc offline/packages/trackreco/ActsPropagator.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 330
🏁 Script executed:
# Look at the propagation calls to see if options are configured with overstep
sed -n '300,500p' offline/packages/trackreco/ActsPropagator.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1074
🏁 Script executed:
# Search for propagate calls to see what options are passed
rg 'propagate\(' offline/packages/trackreco/ActsPropagator.cc -B5 -A10 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1852
🏁 Script executed:
# Check Acts includes to understand version and API
head -30 offline/packages/trackreco/ActsPropagator.cc | catRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1181
🏁 Script executed:
# Look for any Acts propagator examples with stepper construction
fd '\.cc$' offline/packages/trackbase | xargs grep -l 'EigenStepper' | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 145
🏁 Script executed:
# Look at how EigenStepper is used in TrackFittingAlgorithmFunctionsKalman.cc
grep -n 'Stepper\|overstep' offline/packages/trackbase/TrackFittingAlgorithmFunctionsKalman.cc -B3 -A3Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1318
🏁 Script executed:
# Check the full context of Stepper construction in that file
sed -n '1,100p' offline/packages/trackbase/TrackFittingAlgorithmFunctionsKalman.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 3763
🏁 Script executed:
# Search for any configuration of overstep in propagation options across the codebase
rg 'overstep|abortStepSize|maxStepSize' offline/packages/ -iRepository: sPHENIX-Collaboration/coresoftware
Length of output: 437
🏁 Script executed:
# Check PHActsVertexPropagator to see how setOverstepLimit is used
grep -n 'setOverstepLimit' offline/packages/trackreco/PHActsVertexPropagator.cc -B10 -A10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 875
🏁 Script executed:
# Check if there's any documentation or comments about overstep in Navigator config
rg 'Navigator.*Config|Config.*overstep' offline/packages/trackreco/ -B5 -A5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 901
🏁 Script executed:
# Search for PropagatorOptions configuration with any step-related settings
rg 'PropagatorOptions|Options<' offline/packages/trackreco/ActsPropagator.cc -A5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 873
🏁 Script executed:
# Search for any documentation or comments about EigenStepper constructor signature
rg 'EigenStepper.*overstep|stepSize|abortStep' offline/packages/ -i -B2 -A2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 5887
🏁 Script executed:
# Look for any git history or comments about this change
git log --oneline -20 offline/packages/trackreco/ActsPropagator.cc 2>/dev/null | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 176
🏁 Script executed:
# Check if there are any test files that show how to properly use overstepLimit
fd -type f \( -name '*test*.cc' -o -name '*Test*.cc' \) | xargs grep -l 'ActsPropagator\|EigenStepper' 2>/dev/null | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 249
🏁 Script executed:
# Check for Acts version information in the codebase
rg 'Acts.*v|version.*45' offline/packages/ | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2638
🏁 Script executed:
# Look for any comments about the migration or API changes
rg 'v45|migration|deprecated.*Stepper' offline/packages/trackreco/ -B3 -A3Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check if there's any external Acts documentation reference or CMakeLists showing version
find offline/packages/trackreco -name 'CMakeLists.txt' -exec head -40 {} \; 2>/dev/nullRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
The overstep limit configuration is silently ignored.
The m_overstepLimit member is publicly settable (and actively used by PHActsVertexPropagator), but it is never applied during propagation. Both makePropagator() and makeFastPropagator() construct the stepper without threading this parameter, causing any configured overstep tuning to be lost. This silently degrades propagation behavior across all track propagation calls in this class.
The suggested fix location in the Stepper constructor should be verified against the Acts v45 API; however, the core issue—that configured overstep limits are not being applied—is confirmed and requires correction.
| std::shared_ptr<Acts::IMaterialDecorator> matDeco = nullptr; | ||
| if (materialFile.find(".json") != std::string::npos || | ||
| materialFile.find(".cbor") != std::string::npos) | ||
| { | ||
| // Set up the converter first | ||
| Acts::MaterialMapJsonConverter::Config jsonGeoConvConfig; | ||
| // Set up the json-based decorator | ||
| matDeco = std::make_shared<const Acts::JsonMaterialDecorator>( | ||
| jsonGeoConvConfig, fileName, Acts::Logging::FATAL); | ||
| matDeco = std::make_shared<Acts::JsonMaterialDecorator>( | ||
| jsonGeoConvConfig, materialFile, Acts::Logging::FATAL); | ||
| } | ||
| else | ||
| { | ||
| matDeco = std::make_shared<const Acts::MaterialWiper>(); | ||
| matDeco = std::make_shared<Acts::MaterialWiper>(); | ||
| } | ||
| config.materialDecorator = matDeco; |
There was a problem hiding this comment.
Do not silently drop material effects on unknown map paths.
This branch now falls back to Acts::MaterialWiper whenever materialFile lacks a .json/.cbor substring. In this same file, setMaterialResponseFile() can source the map from CDB, and those returned paths are not guaranteed to preserve that suffix. A valid material map can therefore turn into a no-material geometry with no failure signal, which will skew propagation and fitting.
Fail fast on unrecognized formats, or key this off an explicit material type instead of filename sniffing.
Suggested guard
- else
- {
- matDeco = std::make_shared<Acts::MaterialWiper>();
- }
+ else
+ {
+ throw std::runtime_error(
+ "Unsupported Acts material map format: " + materialFile +
+ ". Refusing to build geometry without material decoration.");
+ }📝 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::shared_ptr<Acts::IMaterialDecorator> matDeco = nullptr; | |
| if (materialFile.find(".json") != std::string::npos || | |
| materialFile.find(".cbor") != std::string::npos) | |
| { | |
| // Set up the converter first | |
| Acts::MaterialMapJsonConverter::Config jsonGeoConvConfig; | |
| // Set up the json-based decorator | |
| matDeco = std::make_shared<const Acts::JsonMaterialDecorator>( | |
| jsonGeoConvConfig, fileName, Acts::Logging::FATAL); | |
| matDeco = std::make_shared<Acts::JsonMaterialDecorator>( | |
| jsonGeoConvConfig, materialFile, Acts::Logging::FATAL); | |
| } | |
| else | |
| { | |
| matDeco = std::make_shared<const Acts::MaterialWiper>(); | |
| matDeco = std::make_shared<Acts::MaterialWiper>(); | |
| } | |
| config.materialDecorator = matDeco; | |
| std::shared_ptr<Acts::IMaterialDecorator> matDeco = nullptr; | |
| if (materialFile.find(".json") != std::string::npos || | |
| materialFile.find(".cbor") != std::string::npos) | |
| { | |
| // Set up the converter first | |
| Acts::MaterialMapJsonConverter::Config jsonGeoConvConfig; | |
| // Set up the json-based decorator | |
| matDeco = std::make_shared<Acts::JsonMaterialDecorator>( | |
| jsonGeoConvConfig, materialFile, Acts::Logging::FATAL); | |
| } | |
| else | |
| { | |
| throw std::runtime_error( | |
| "Unsupported Acts material map format: " + materialFile + | |
| ". Refusing to build geometry without material decoration."); | |
| } | |
| config.materialDecorator = matDeco; |
| ACTS_LIBS = \ | ||
| -lActsCore \ | ||
| -lActsPluginTGeo \ | ||
| -lActsPluginRoot \ | ||
| -lActsExamplesDetectorTGeo \ | ||
| -lActsExamplesFramework |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relevant Makefile section ---'
sed -n '118,200p' offline/packages/trackreco/Makefile.am
echo
echo '--- ACTS_LIBS references in this Makefile ---'
rg -n '\$\(ACTS_LIBS\)|^ACTS_LIBS\s*=' offline/packages/trackreco/Makefile.amRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1984
The ActsPluginRoot library is not actually linked.
ACTS_LIBS is defined to include -lActsPluginRoot (line 119), but libtrack_reco_la_LIBADD never references $(ACTS_LIBS). Instead, it manually lists only -lActsCore, -lActsExamplesDetectorTGeo, and -lActsExamplesFramework (lines 128–130), omitting -lActsPluginRoot entirely. If the v45 migration depends on this library, the target will fail to link or behave incorrectly.
Wire ACTS_LIBS into the link line:
Suggested fix
libtrack_reco_la_LIBADD = \
- -lActsCore \
- -lActsExamplesDetectorTGeo \
- -lActsExamplesFramework \
+ $(ACTS_LIBS) \
-lcalo_io \
-lg4eval \
-lg4testbench \| auto finder = std::make_unique<Acts::SeedFinderOrthogonal<proxy_type>>(m_seedFinderConfig); | ||
|
|
||
| auto spacePoints = getMvtxSpacePoints(); | ||
| Acts::SpacePointContainerConfig spConfig; | ||
| Acts::SpacePointContainerOptions spOptions; | ||
| spOptions.beamPos = {0, 0}; | ||
|
|
||
| std::function< | ||
| std::tuple<Acts::Vector3, Acts::Vector2, std::optional<Acts::ActsScalar>>( | ||
| const SpacePoint* sp)> | ||
| create_coordinates = [](const SpacePoint* sp) | ||
| { | ||
| Acts::Vector3 position(sp->x(), sp->y(), sp->z()); | ||
| Acts::Vector2 variance(sp->varianceR(), sp->varianceZ()); | ||
| return std::make_tuple(position, variance, sp->t()); | ||
| }; | ||
|
|
||
| ActsExamples::SpacePointContainer container(spacePoints); | ||
| Acts::SpacePointContainer<decltype(container), Acts::detail::RefHolder> spContainer(spConfig, spOptions, container); | ||
| /// Call acts seeding algo | ||
| SeedContainer seeds = finder.createSeeds(m_seedFinderOptions, | ||
| spacePoints, create_coordinates); | ||
| auto seeds = finder->createSeeds(m_seedFinderOptions, spContainer); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
fd -i "phactsKDTreeSeeding" -e cc -e hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 177
🏁 Script executed:
# Let's examine the header file first to understand proxy_type definition
cat -n offline/packages/trackreco/PHActsKDTreeSeeding.h | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 4316
🏁 Script executed:
# Now let's look at the implementation, specifically lines 118-128 and 145-149
cat -n offline/packages/trackreco/PHActsKDTreeSeeding.cc | sed -n '110,160p'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2077
🏁 Script executed:
# Let's check what SeedContainer is
rg "using SeedContainer|typedef.*SeedContainer" offline/packages/trackreco/Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Let's look at the full runSeeder function and check includes
cat -n offline/packages/trackreco/PHActsKDTreeSeeding.cc | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1867
🏁 Script executed:
# Search for SeedContainer definition in Acts includes
rg "SeedContainer" offline/packages/trackreco/PHActsKDTreeSeeding.cc | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 468
🏁 Script executed:
# Check the complete header file for SeedContainer definition
cat -n offline/packages/trackreco/PHActsKDTreeSeeding.h | tail -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2234
🏁 Script executed:
# Search for SeedContainer definition or using statement
rg "using SeedContainer|SeedContainer\s*=" offline/packages/trackreco/PHActsKDTreeSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Search for SeedContainer in Acts includes or anywhere in the file
rg "SeedContainer" offline/packages/trackreco/PHActsKDTreeSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 291
🏁 Script executed:
# Search for SeedContainer definition across all files in the repo
rg "using SeedContainer|typedef.*SeedContainer" --type cc --type hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 105
🏁 Script executed:
# Search for SeedContainer definition across all files
rg "using SeedContainer|typedef.*SeedContainer"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 177
🏁 Script executed:
# Let's check the Acts includes to understand RefHolder semantics
# Search for any documentation or usage of RefHolder
rg "RefHolder" --type-add 'h:include' -eRepository: sPHENIX-Collaboration/coresoftware
Length of output: 485
🏁 Script executed:
# Let's search for RefHolder in the codebase
rg "RefHolder"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1138
🏁 Script executed:
# Let's check how PHActsSiliconSeeding handles the same pattern
cat -n offline/packages/trackreco/PHActsSiliconSeeding.cc | grep -A 30 "SpacePointContainerRefHolder" | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1347
🏁 Script executed:
# Let's look at what happens after the grid is created in PHActsSiliconSeeding
cat -n offline/packages/trackreco/PHActsSiliconSeeding.cc | sed -n '220,330p'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 4431
🏁 Script executed:
# Let's verify the return type of createSeeds by checking the function signature
# Also check what happens to seeds after they're returned in runSeeder
cat -n offline/packages/trackreco/PHActsKDTreeSeeding.cc | sed -n '100,180p'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3056
Fix memory safety issue: RefHolder proxies outlive their backing containers.
Lines 118-128 create Acts::Seed<proxy_type> where proxy_type uses Acts::detail::RefHolder, meaning each seed holds references into container and spContainer. Both are stack-local variables destroyed at line 136 when runSeeder() returns. However, these seeds are immediately consumed in fillTrackSeedContainer() (called at line 111) where line 148 dereferences the proxy via sps[spid]->externalSpacePoint()->Id(), accessing memory from already-destroyed containers. Either materialize cluster keys and positions before returning from runSeeder(), or keep container and spContainer alive through the seed consumption in fillTrackSeedContainer().
| auto spVec = getSiliconSpacePoints(strobe); | ||
| eventTimer->stop(); |
There was a problem hiding this comment.
Guard empty strobes before building rMiddleSPRange.
If getSiliconSpacePoints(strobe) returns no usable hits, every grid bin stays empty and minRange/maxRange keep their sentinel values. That still constructs an invalid Acts::Range1D<float> and calls createSeedsForGroup, so quiet empty strobes can turn into undefined seeding behavior instead of a clean skip.
Suggested fix
eventTimer->restart();
auto spVec = getSiliconSpacePoints(strobe);
eventTimer->stop();
spTime += eventTimer->get_accumulated_time();
if (m_seedAnalysis)
{
h_nInputMeas->Fill(spVec.size());
}
+ if (spVec.empty())
+ {
+ continue;
+ }
...
float minRange = std::numeric_limits<float>::max();
float maxRange = std::numeric_limits<float>::lowest();
for (const auto& coll : grid)
{
if (coll.empty())
@@
minRange = std::min(firstEl->radius(), minRange);
maxRange = std::max(lastEl->radius(), maxRange);
}
+ if (minRange >= maxRange)
+ {
+ continue;
+ }Also applies to: 247-272
| std::array<std::vector<std::size_t>, 3ul> navigation; | ||
| navigation[1ul] = m_seedFinderCfg.zBinsCustomLooping; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PHActsSiliconSeeding.cc" -o -name "PHActsSiliconSeeding.h" | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 183
🏁 Script executed:
cd offline/packages/trackreco 2>/dev/null && wc -l PHActsSiliconSeeding.cc && head -30 PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 983
🏁 Script executed:
cd offline/packages/trackreco 2>/dev/null && sed -n '255,270p' PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 732
🏁 Script executed:
cd offline/packages/trackreco 2>/dev/null && sed -n '1475,1550p' PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2658
🏁 Script executed:
cd offline/packages/trackreco && grep -n "configureSeeder\|configureSPGrid\|toInternalUnits\|calculateDerivedQuantities" PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 226
🏁 Script executed:
cd offline/packages/trackreco && grep -n "zBinsCustomLooping" PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 140
🏁 Script executed:
cd offline/packages/trackreco && grep -n "m_seedFinderCfg\|m_gridCfg\|m_gridOptions" PHActsSiliconSeeding.cc | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2263
🏁 Script executed:
cd offline/packages/trackreco && sed -n '80,105p' PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 793
🏁 Script executed:
cd offline/packages/trackreco && grep -n "zBinsCustomLooping" PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
cd offline/packages/trackreco && grep -n "m_seedFinderCfg\|m_gridCfg\|m_gridOptions" PHActsSiliconSeeding.h | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 261
🏁 Script executed:
cd offline/packages/trackreco && sed -n '190,210p' PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 664
🏁 Script executed:
cd offline/packages/trackreco && grep -rn "Acts::SeedFinderConfig\|Acts::CylindricalSpacePointGridConfig" --include="*.h" --include="*.cc" | grep -E "(toInternalUnits|calculateDerivedQuantities)" | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
cd offline/packages/trackreco && head -100 PHActsSiliconSeeding.h | tail -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1260
🏁 Script executed:
cd offline/packages/trackreco && grep -n "Acts::SeedFinder\|Acts::CylindricalSpacePointGrid" PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 436
🏁 Script executed:
cd offline/packages/trackreco && grep -rn "Acts" PHActsSiliconSeeding.h | grep "include" | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 446
🏁 Script executed:
cd offline/packages && find . -name "*.cmake" -o -name "CMakeLists.txt" | xargs grep -l "Acts\|ACTS" | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
cd offline/packages/trackreco && sed -n '235,270p' PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1381
🏁 Script executed:
cd offline/packages/trackreco && git log --oneline -20 -- offline/packages/trackreco/PHActsSiliconSeeding.cc 2>/dev/null || echo "Git history not available"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
cd offline/packages/trackreco && grep -n "zBinsCustomLooping\|zBinsTop\|zBinsBottom" PHActsSiliconSeeding.*Repository: sPHENIX-Collaboration/coresoftware
Length of output: 164
🏁 Script executed:
cd offline/packages/trackreco && sed -n '1500,1544p' PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1864
🏁 Script executed:
cd offline/packages && find . -name "*.cc" -o -name "*.h" | xargs grep -l "zBinsCustomLooping" 2>/dev/null | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 114
🏁 Script executed:
# Check if zBinsCustomLooping is a member that should be initialized/derived
cd offline/packages/trackreco && grep -A 10 "SeedFinderConfig<" PHActsSiliconSeeding.h | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 596
🏁 Script executed:
cd offline/packages/trackreco && sed -n '85,100p' PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 487
🏁 Script executed:
# Check if there's any Acts documentation or examples in the codebase
cd offline/packages && find . -name "*.cc" -o -name "*.h" | xargs grep -l "calculateDerivedQuantities\|toInternalUnits" 2>/dev/null | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
cd offline/packages/trackreco && grep -B 5 "m_seedFinderCfg\s*=" PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check the Acts library itself to understand SeedFinderConfig default initialization
cd offline/packages && find . -path "*/Acts/*" -name "*.hpp" 2>/dev/null | xargs grep -l "zBinsCustomLooping" 2>/dev/null | head -3Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Final check: verify if zBinsCustomLooping is ever assigned to m_seedFinderCfg
cd offline/packages/trackreco && grep -n "\.zBinsCustomLooping\s*=" PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check if there's any comment or documentation about config normalization
cd offline/packages/trackreco && sed -n '80,95p' PHActsSiliconSeeding.cc && echo "---" && sed -n '1475,1495p' PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 962
Verify Acts config normalization is applied to m_seedFinderCfg and m_gridCfg before use.
configureSeeder() and configureSPGrid() perform only raw member assignments without calling Acts' normalization methods (toInternalUnits(), calculateDerivedQuantities()). At line 262, navigation[1ul] = m_seedFinderCfg.zBinsCustomLooping consumes this data, but zBinsCustomLooping is never populated in this file and would remain default-initialized (empty). Acts v22.0.0 documentation requires explicit normalization of SeedFinderConfig and CylindricalSpacePointGridConfig. Without it, seeding can silently run with empty derived navigation or incorrect unit scales.
|
Here is the latest QA after the last push, showing that in simulation the tracking QA is identical between the current version and new version |
There was a problem hiding this comment.
Actionable comments posted: 0
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/ActsPropagator.cc (1)
140-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing null check for
m_geometry— inconsistent with sibling overload.The
propagateTrack(params, sphenixLayer)overload at line 105 guards against nullm_geometryand returns a failureResult. This overload dereferencesm_geometryat lines 151, 154, and implicitly inmakePropagator()without any null check, leading to a crash instead of a graceful error return.For defensive consistency:
Proposed fix
ActsPropagator::BTPPairResult ActsPropagator::propagateTrack(const Acts::BoundTrackParameters& params, const SurfacePtr& surface) { + if (!m_geometry) + { + return Acts::Result<BoundTrackParamPair>::failure(std::error_code(0, std::generic_category())); + } + if (m_verbosity > 1) { printTrackParams(params); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6429eed6-ebe5-4ef9-b17c-bdfe38b55750
📒 Files selected for processing (1)
offline/packages/trackreco/ActsPropagator.cc
1 participant
This PR updates to Acts v45.0.0, which seems to fix the issues we saw in v44.0.0. Still some testing to do but initial tests suggest this is in better shape than the previous attempt
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)
Acts v45.0.0 Integration Update
Motivation / context
Upgrade the sPHENIX coresoftware to Acts v45.0.0 to pick up fixes from Acts (issues observed in v44.x) and to complete a broader API migration begun in prior integration attempts. Initial QA (simulation tracking QA images provided by the author) shows tracking QA identical to the current baseline for simulation; further testing is planned.
Key changes
Potential risk areas
Possible future improvements
Notes: