Skip to content

feat: update to actsv45.0.0#4272

Draft
osbornjd wants to merge 55 commits into
sPHENIX-Collaboration:masterfrom
osbornjd:actsv45.0.0
Draft

feat: update to actsv45.0.0#4272
osbornjd wants to merge 55 commits into
sPHENIX-Collaboration:masterfrom
osbornjd:actsv45.0.0

Conversation

@osbornjd
Copy link
Copy Markdown
Contributor

@osbornjd osbornjd commented May 20, 2026

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work for users)
  • Requiring change in macros repository (Please provide links to the macros pull request in the last section)
  • I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

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

This PR updates the sPHENIX coresoftware to use Acts v45.0.0, which addresses issues observed in v44.0.0 and improves upon the previous integration attempt. The upgrade brings significant API improvements but introduces multiple breaking changes that require systematic refactoring across the tracking, reconstruction, and geometry modules.

Key Changes

Core API Updates:

  • Replaced deprecated Surface::transform(...) calls with Surface::localToGlobalTransform(...) across coordinate conversion operations (>15 files affected: geometry helpers, calibrators, evaluators, residual calculators, track fitters)
  • Updated track index types from Acts::MultiTrajectoryTraits::IndexType to Acts::TrackIndexType in track fitting and evaluation pipelines
  • Refactored trajectory containers from Acts::ConstVectorMultiTrajectory to Acts::VectorMultiTrajectory (non-const) in multiple fitter implementations

Measurement & Calibration:

  • Migrated measurement containers from local std::vector<Acts::BoundVariantMeasurement> definitions to ActsExamples::MeasurementContainer (standardizes container types across modules)
  • Updated measurement access patterns using measurements.getMeasurement(index) and Acts::visit_measurement() for handling variable measurement sizes
  • Refined projector matrix computations using projectorSubspaceHelper().fullProjector() instead of effectiveProjector()

Detector Geometry & Plugin Updates:

  • Switched detector element implementations from Acts::TGeoDetectorElement to ActsPlugins::TGeoDetectorElement in sPHENIX elements
  • Updated magnetic field I/O to use new Acts and ActsPlugins namespaces for field-map factory functions
  • Modified library linking: replaced -lActsPluginTGeo with -lActsPluginRoot throughout build system

Seeding & Space Points:

  • Converted seeding infrastructure to use Acts space-point proxy types (SpacePointProxy_type) instead of direct SpacePoint objects
  • Updated seed container structures and grid binning (changed GridBinFinder from 2 to 3 bins in silicon seeding)
  • Refactored SeedFinderOrthogonal and SeedFilter to operate on proxy-based space point containers

GSF (Gaussian Sum Filter) & Propagation:

  • Temporarily excluded GSF-related modules from default build (PHActsGSF.cc, TrackFittingAlgorithmFunctionsGsf.cc removed from compilation)
  • Added configurable reverseFilteringCovarianceScaling parameter to GSF fitter options (default: 100.0)
  • Refactored propagator options construction using SphenixPropagatorOptions with explicit actor/aborter configuration

Geometry Context Management:

  • Replaced direct alignment map assignments with explicit Acts::GeometryContext{transformMap} construction
  • Added explicit initialization of geometry contexts using Acts::GeometryContext::dangerouslyDefaultConstruct() where applicable
  • Updated AlignmentTransformation to use localToGlobalTransform() for surface transform extraction

Build System & Header Organization:

  • Removed dependency on ActsExamples/Utilities/OptionsFwd.hpp, replacing with direct forward declarations of Boost types
  • Cleaned up include paths from Acts/Plugins/TGeo/ to ActsPlugins/Root/
  • Reorganized library dependencies and header installation order in Makefiles

Risk Areas

Critical Breaking Changes:

  • Surface coordinate transformation API changes affect all detector-element interaction code; incorrect migration would produce systematic reconstruction errors
  • Track index type changes in multi-trajectory containers could cause type mismatches if downstream code not updated consistently
  • Measurement container changes require compatible data structures throughout track fitting pipelines

Reconstruction Behavior:

  • Modified projector matrices and measurement handling could affect residual calculations and alignment-state computations
  • Seeding algorithm changes to proxy-based containers may impact seed finding efficiency and distribution
  • Magnetic field I/O refactoring requires verification that field maps read correctly from both ROOT and text formats

Library Dependencies:

  • Plugin library transition (-TGeo → -Root) requires linked libraries to be available and compatible
  • Changes to ActsExamples namespace usage could affect external dependency builds if not aligned

Performance Considerations:

  • Geometry context construction pattern changes (dangerouslyDefaultConstruct) should be verified for performance impact
  • Space-point proxy container implementation may have different memory/CPU characteristics compared to direct object storage

Notes

  • AI Analysis Caveat: This summary is AI-assisted and may contain interpretation errors regarding specific implementation details. Reviewers should verify complex coordinate transformation and container type migrations carefully.
  • Additional testing beyond initial validation is explicitly planned per PR description
  • Some features (GSF) are temporarily disabled pending full v45.0.0 compatibility verification

@osbornjd osbornjd marked this pull request as draft May 20, 2026 19:01
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 20, 2026

📝 Walkthrough

Walkthrough

This PR implements a large-scale Acts API migration across the sPHENIX tracking software, replacing surface transform calls with the newer localToGlobalTransform() interface, standardizing measurement containers and track indexing types, explicitly initializing geometry contexts, and refactoring geometry construction and seeding infrastructure to use Acts SpacePoint proxies.

Changes

Acts Surface Transform and Geometry Coordinate API Migration

Layer / File(s) Summary
Detector element transforms and residual calculations
offline/QA/Tracking/CosmicTrackQA.cc, offline/packages/TrackerMillepedeAlignment/HelicalFitter.cc, offline/packages/TrackingDiagnostics/TrackResiduals.cc, offline/packages/intt/CylinderGeomInttHelper.cc, offline/packages/micromegas/CylinderGeomMicromegas.cc, offline/packages/mvtx/CylinderGeom_MvtxHelper.cc, offline/packages/tpc/LaserClusterizer.cc, offline/packages/tpc/TpcClusterizer.cc, offline/packages/trackbase/sPHENIXActsDetectorElement.cc, offline/packages/trackbase_historic/TrackAnalysisUtils.cc, offline/packages/trackreco/MakeSourceLinks.cc, offline/packages/trackreco/PHActsSiliconSeeding.cc, offline/packages/trackreco/WeightedFitter.cc, simulation/g4simulation/g4tpc/TpcClusterBuilder.cc
Replace surf->transform(...)/surface->transform(...) with surf->localToGlobalTransform(...) and .inverse() for coordinate conversions in residual calculations, cluster coordinate transformations, and detector geometry operations.
Geometry context explicit initialization and alignment
offline/packages/trackbase/ActsGeometry.h, offline/packages/trackbase/ActsTrackingGeometry.h, offline/packages/trackbase/AlignmentTransformation.cc, offline/packages/trackbase/alignmentTransformationContainer.h, offline/packages/trackbase/TrackFittingAlgorithmFunctionsGsf.cc
Explicitly initialize Acts::GeometryContext using dangerouslyDefaultConstruct() in class member declarations, construct geometry context from alignment transformation maps in AlignmentTransformation, and remove base-class inheritance from alignmentTransformationContainer.
TGeo detector element base class migration to ActsPlugins
offline/packages/trackbase/TGeoDetectorWithOptions.cc, offline/packages/trackbase/TGeoDetectorWithOptions.h, offline/packages/trackbase/sPHENIXActsDetectorElement.cc, offline/packages/trackbase/sPHENIXActsDetectorElement.h
Switch sPHENIXActsDetectorElement from Acts::TGeoDetectorElement to ActsPlugins::TGeoDetectorElement, rename transform() override to localToGlobalTransform(), update include paths from Acts/Plugins/TGeo to ActsPlugins/Root, and add public constructor to TGeoDetectorWithOptions.

Acts Measurement Container and Track Indexing Standardization

Layer / File(s) Summary
Measurement container type aliases and direct emplacement
offline/packages/trackbase/ActsTrackFittingAlgorithm.h, offline/packages/trackbase/Calibrator.h, offline/packages/trackbase/Calibrator.cc, offline/packages/trackreco/ActsEvaluator.h, offline/packages/trackreco/MakeSourceLinks.cc
Update MeasurementContainer typedefs to use ActsExamples::MeasurementContainer instead of locally-defined std::vector<Acts::BoundVariantMeasurement>, replace std::visit over measurements with measurements.getMeasurement(index) calls, and use measurements.emplaceMeasurement<2>(...) for direct insertion.
Track indexing type migration to TrackIndexType
offline/packages/trackreco/ActsAlignmentStates.h, offline/packages/trackreco/ActsAlignmentStates.cc, offline/packages/trackreco/ActsEvaluator.h, offline/packages/trackreco/ActsEvaluator.cc, offline/packages/trackreco/PHActsTrkFitter.h, offline/packages/trackreco/PHActsTrkFitter.cc, offline/packages/trackreco/PHCosmicsTrkFitter.h, offline/packages/trackreco/PHCosmicsTrkFitter.cc
Replace std::vector<Acts::MultiTrajectoryTraits::IndexType> with std::vector<Acts::TrackIndexType> for track tips in fillAlignmentStateMap, process_track, evaluateTrackFit, visitTrackStates, and updateSvtxTrack signatures.
Measurement state filtering and local coordinate extraction
offline/packages/trackreco/ActsAlignmentStates.cc, offline/packages/trackreco/ActsEvaluator.cc, offline/packages/trackbase_historic/ActsTransformations.cc
Switch measurement-state existence checks from TrackStateFlag::MeasurementFlag testing to typeFlags().isMeasurement() shorthand, extract local parameters directly from measurement proxy instead of variant visitation.
Predicted residual and projector matrix updates
offline/packages/trackbase/ResidualOutlierFinder.h, offline/packages/trackreco/ActsAlignmentStates.cc, offline/packages/trackreco/ActsEvaluator.cc
Compute chi² via Acts::calculatePredictedChi2(state) and residuals via calculatePredictedResidual<kMeasurementSize>(state), replace state.effectiveProjector() with state.projectorSubspaceHelper().fullProjector() for residual projectors and derivative computations.

Acts Space Point Proxy and Seeding Infrastructure Modernization

Layer / File(s) Summary
SpacePoint container type aliases and proxy definitions
offline/packages/trackbase/SpacePoint.h, offline/packages/trackreco/PHActsKDTreeSeeding.h, offline/packages/trackreco/PHActsSiliconSeeding.h
Define SpacePointContainerType, proxy_type, and seed_type aliases using ActsExamples::SpacePointContainer and Acts SpacePointContainer proxies; update SeedContainer to hold Acts::Seed<proxy_type> instead of Acts::Seed<SpacePoint>.
KDTree seeding framework proxy-based refactoring
offline/packages/trackreco/PHActsKDTreeSeeding.cc, offline/packages/trackreco/PHActsKDTreeSeeding.h
Construct Acts::SeedFinderOrthogonal<proxy_type> via std::make_unique, build ActsExamples::SpacePointContainer, update seedFilter to Acts::SeedFilter<proxy_type>, extract space point data via seed.sp() indexing and externalSpacePoint()->Id(), and remove toInternalUnits() and calculateDerivedQuantities() conversion calls.
Silicon seeding framework proxy and grid refactoring
offline/packages/trackreco/PHActsSiliconSeeding.cc, offline/packages/trackreco/PHActsSiliconSeeding.h
Switch seed finder and filter to SpacePointProxy_type, change bin-finder dimensions from 2ul to 3ul, refactor runSeeder() to build proxy-backed cylindrical grids, update makeSvtxTracksWithTime and makeSvtxTracks to accept std::vector<seed_type> and extract fixed-index space points from seed.sp(), remove Acts::Extent parameter from getSiliconSpacePoints, and eliminate internal-units conversions.

Geometry Construction and Propagator Options Refactoring

Layer / File(s) Summary
MakeActsGeometry geometry building wiring and material decorator
offline/packages/trackreco/MakeActsGeometry.cc, offline/packages/trackreco/MakeActsGeometry.h
Replace makeGeometry(argc, argv, detector) with makeGeometry(argc, argv, responseFile, materialFile), construct ActsExamples::TGeoDetectorWithOptions directly from response JSON file, select JsonMaterialDecorator or MaterialWiper based on material file extension, add m_TGeoDetector member, initialize m_geoCtxt explicitly, and update INTT/MVTX diagnostics to use surfacePlacement() and thickness().
Propagator options and track parameters initialization
offline/packages/trackreco/ActsPropagator.cc, offline/packages/trackreco/ActsPropagator.h, offline/packages/trackreco/PHActsGSF.cc, offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc
Construct SphenixPropagatorOptions instead of plain Acts::PropagatorOptions, reorder TrackParameters::create() arguments to pass geometry context before surface, introduce ActsPropagator::Actors and SphenixPropagatorOptions type aliases, and construct propagator plain options with transient geometry context.
Track fitting transient geometry context and trajectory management
offline/packages/trackreco/PHActsTrkFitter.cc, offline/packages/trackreco/PHActsTrkFitter.h, offline/packages/trackreco/PHCosmicsTrkFitter.cc, offline/packages/trackreco/PHCosmicsTrkFitter.h
Explicitly construct Acts::GeometryContext from alignment transformation maps for each track fit iteration, update trackTips type to Acts::TrackIndexType, refactor updateSvtxTrack signature, and remove m_trajectories member and related storage/node-wiring logic in PHCosmicsTrkFitter.
GSF fitter configuration and reverseFilteringCovarianceScaling
offline/packages/trackbase/ActsGsfTrackFittingAlgorithm.h, offline/packages/trackbase/TrackFittingAlgorithmFunctionsGsf.cc, offline/packages/trackreco/PHActsGSF.cc, offline/packages/trackreco/PHActsGSF.h
Add reverseFilteringCovarianceScaling member to GsfFitterFunctionImpl and parameter to makeGsfFitterFunction, wire the parameter into Acts::GsfOptions, update PHActsGSF to pass configuration value and construct PropagatorPlainOptions with transient context.

Interface Simplification and Build System Updates

Layer / File(s) Summary
IBaseDetector interface simplification and TGeoDetectorWithOptions
offline/packages/trackbase/IBaseDetector.h, offline/packages/trackbase/TGeoDetectorWithOptions.h, offline/packages/trackbase/TGeoDetectorWithOptions.cc
Remove finalize() pure virtual method from IBaseDetector, add explicit public constructor to TGeoDetectorWithOptions, forward-declare boost::program_options types, define ActsExamples::Options aliases locally.
Magnetic field I/O headers and factory namespacing
offline/packages/trackbase/MagneticFieldOptions.h, offline/packages/trackbase/MagneticFieldOptions.cc
Update includes to use Acts/MagneticField/TextMagneticFieldIo.hpp and ActsPlugins/Root/RootMagneticFieldIo.hpp, qualify field-map factory calls with ActsPlugins:: (ROOT input) and Acts:: (text input), forward-declare boost types and define ActsExamples::Options aliases.
Makefile plugin dependency swaps and header updates
calibrations/tpc/TpcDVCalib/Makefile.am, offline/packages/trackbase/Makefile.am, offline/packages/trackreco/Makefile.am, offline/packages/trackbase_historic/Makefile.am
Replace -lActsPluginTGeo with -lActsPluginRoot in ACTS_LIBS and libtrack*_la_LIBADD, remove -lActsExamplesMagneticField, remove ActsGsfTrackFittingAlgorithm.h and TrackFittingAlgorithmFunctionsGsf.cc from headers/sources, reorder library search paths to prioritize offline paths, remove Acts library dependencies from TPC calibration.
Supporting includes and type alias cleanup
offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.h, offline/packages/trackreco/PHActsGSF.h, offline/packages/trackreco/PHActsTrkFitter.h, offline/packages/trackreco/PHCosmicsTrkFitter.h, simulation/g4simulation/g4tpc/PHG4TpcPadPlaneReadout.cc, simulation/g4simulation/g4tpc/TpcClusterBuilder.cc, offline/packages/trackbase_historic/TrackAnalysisUtils.h
Add <fstream> includes where needed, remove local Measurement type aliases from headers, update const qualification order in function parameters.

Possibly Related PRs

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Add 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 win

Preserve 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 win

Add dimension check before processing measurements.

The code mixes dynamic dimension handling (state.calibratedSize()) with static 2D assumptions. Lines 131–132 unconditionally read measurement.parameters()[0] and [1], while line 146 forces the projected state into Acts::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 win

Document the IBaseDetector API break for downstream detectors.

Removing finalize(...) from this installed interface is a source-breaking change for any external detector implementation built against trackbase headers. 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 win

The widened MVTX strobe window will duplicate seeds across neighboring passes.

runSeeder() already iterates each strobe independently, but this filter now feeds hits from strobe - 1, strobe, and strobe + 1 into every pass. Because m_seedContainer is 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

📥 Commits

Reviewing files that changed from the base of the PR and between c416f2f and 3364377.

📒 Files selected for processing (60)
  • calibrations/tpc/TpcDVCalib/Makefile.am
  • calibrations/tpc/TpcDVCalib/TrackToCalo.cc
  • offline/QA/Tracking/CosmicTrackQA.cc
  • offline/packages/TrackerMillepedeAlignment/HelicalFitter.cc
  • offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc
  • offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.h
  • offline/packages/TrackingDiagnostics/TrackResiduals.cc
  • offline/packages/intt/CylinderGeomInttHelper.cc
  • offline/packages/micromegas/CylinderGeomMicromegas.cc
  • offline/packages/mvtx/CylinderGeom_MvtxHelper.cc
  • offline/packages/tpc/LaserClusterizer.cc
  • offline/packages/tpc/TpcClusterizer.cc
  • offline/packages/trackbase/ActsGeometry.h
  • offline/packages/trackbase/ActsGsfTrackFittingAlgorithm.h
  • offline/packages/trackbase/ActsTrackFittingAlgorithm.h
  • offline/packages/trackbase/ActsTrackingGeometry.h
  • offline/packages/trackbase/AlignmentTransformation.cc
  • offline/packages/trackbase/Calibrator.cc
  • offline/packages/trackbase/Calibrator.h
  • offline/packages/trackbase/IBaseDetector.h
  • offline/packages/trackbase/MagneticFieldOptions.cc
  • offline/packages/trackbase/MagneticFieldOptions.h
  • offline/packages/trackbase/Makefile.am
  • offline/packages/trackbase/ResidualOutlierFinder.h
  • offline/packages/trackbase/SpacePoint.h
  • offline/packages/trackbase/TGeoDetectorWithOptions.cc
  • offline/packages/trackbase/TGeoDetectorWithOptions.h
  • offline/packages/trackbase/TrackFittingAlgorithmFunctionsGsf.cc
  • offline/packages/trackbase/alignmentTransformationContainer.h
  • offline/packages/trackbase/sPHENIXActsDetectorElement.cc
  • offline/packages/trackbase/sPHENIXActsDetectorElement.h
  • offline/packages/trackbase_historic/ActsTransformations.cc
  • offline/packages/trackbase_historic/ActsTransformations.h
  • offline/packages/trackbase_historic/Makefile.am
  • offline/packages/trackbase_historic/TrackAnalysisUtils.cc
  • offline/packages/trackbase_historic/TrackAnalysisUtils.h
  • offline/packages/trackreco/ActsAlignmentStates.cc
  • offline/packages/trackreco/ActsAlignmentStates.h
  • offline/packages/trackreco/ActsEvaluator.cc
  • offline/packages/trackreco/ActsEvaluator.h
  • offline/packages/trackreco/ActsPropagator.cc
  • offline/packages/trackreco/ActsPropagator.h
  • offline/packages/trackreco/MakeActsGeometry.cc
  • offline/packages/trackreco/MakeActsGeometry.h
  • offline/packages/trackreco/MakeSourceLinks.cc
  • offline/packages/trackreco/MakeSourceLinks.h
  • offline/packages/trackreco/Makefile.am
  • offline/packages/trackreco/PHActsGSF.cc
  • offline/packages/trackreco/PHActsGSF.h
  • offline/packages/trackreco/PHActsKDTreeSeeding.cc
  • offline/packages/trackreco/PHActsKDTreeSeeding.h
  • offline/packages/trackreco/PHActsSiliconSeeding.cc
  • offline/packages/trackreco/PHActsSiliconSeeding.h
  • offline/packages/trackreco/PHActsTrkFitter.cc
  • offline/packages/trackreco/PHActsTrkFitter.h
  • offline/packages/trackreco/PHCosmicsTrkFitter.cc
  • offline/packages/trackreco/PHCosmicsTrkFitter.h
  • offline/packages/trackreco/WeightedFitter.cc
  • simulation/g4simulation/g4tpc/PHG4TpcPadPlaneReadout.cc
  • simulation/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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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 -100

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 105


🏁 Script executed:

# Look for GeometryContext initialization/setup patterns
rg -n 'GeometryContext\(\)' --type cc --type h -A5 | head -80

Repository: 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 -100

Repository: 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 -5

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 308


🏁 Script executed:

# Search for alignmentTransformationContainer initialization/registration
rg -n 'alignmentTransformationContainer' -B3 -A3 | head -150

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 13707


🏁 Script executed:

# Look at the Calibrator.cc file more carefully
cat offline/packages/trackbase/Calibrator.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 2299


🏁 Script executed:

# Check the AlignmentTransformation.h header
cat offline/packages/trackbase/AlignmentTransformation.h

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 4790


🏁 Script executed:

# Search for set< patterns to find where extensions are registered
rg 'set<.*alignmentTransformationContainer' -B2 -A2

Repository: 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 -80

Repository: 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 -100

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 60


🏁 Script executed:

# Check Calibrator.h header for documentation
cat offline/packages/trackbase/Calibrator.h

Repository: 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 -150

Repository: 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 -100

Repository: 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().

Comment on lines +597 to +598
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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

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.

Suggested change
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;
}

Comment on lines 57 to 60
void ActsAlignmentStates::fillAlignmentStateMap(
const ActsTrackFittingAlgorithm::TrackContainer& tracks,
const std::vector<Acts::MultiTrajectoryTraits::IndexType>& tips,
const std::vector<Acts::TrackIndexType>& tips,
SvtxTrack* track,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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:


🏁 Script executed:

wc -l offline/packages/trackreco/ActsAlignmentStates.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 132


🏁 Script executed:

sed -n '50,75p' offline/packages/trackreco/ActsAlignmentStates.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 971


🏁 Script executed:

sed -n '110,150p' offline/packages/trackreco/ActsAlignmentStates.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 1541


🏁 Script executed:

rg "trackStateContainer|tip|Tip" offline/packages/trackreco/ActsAlignmentStates.cc -n

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 281


🏁 Script executed:

grep -n "TrackContainer\|trackContainer" offline/packages/trackreco/ActsAlignmentStates.cc

Repository: 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 f

Repository: 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 -A2

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 306


🏁 Script executed:

sed -n '56,65p' offline/packages/trackreco/ActsAlignmentStates.cc | cat -A

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 561


🏁 Script executed:

grep -n "index()" offline/packages/trackreco/ActsAlignmentStates.cc | head -10

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 218


🏁 Script executed:

sed -n '128,148p' offline/packages/trackreco/ActsAlignmentStates.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 880


🏁 Script executed:

sed -n '140,150p' offline/packages/trackreco/ActsAlignmentStates.cc

Repository: 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.

Comment on lines 233 to 235
auto trackingGeometry = m_geometry->geometry().tGeometry;
Stepper stepper(field, m_overstepLimit);
Stepper stepper(field);
Acts::Navigator::Config cfg{trackingGeometry};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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.cc

Repository: 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.cc

Repository: 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 -B2

Repository: 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 -B3

Repository: 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.cc

Repository: 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 -B5

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 60


🏁 Script executed:

# Find the makeFastPropagator implementation
sed -n '200,235p' offline/packages/trackreco/ActsPropagator.cc

Repository: 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.cc

Repository: 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 -80

Repository: 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 h

Repository: 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.h

Repository: 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.cc

Repository: 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 -100

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 1852


🏁 Script executed:

# Check Acts includes to understand version and API
head -30 offline/packages/trackreco/ActsPropagator.cc | cat

Repository: 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 -5

Repository: 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 -A3

Repository: 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.cc

Repository: 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/ -i

Repository: 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 -A10

Repository: 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 -A5

Repository: 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 -A5

Repository: 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 -A2

Repository: 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 -20

Repository: 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 -5

Repository: 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 -20

Repository: 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 -A3

Repository: 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/null

Repository: 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.

Comment on lines +728 to +742
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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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;

Comment on lines 118 to 122
ACTS_LIBS = \
-lActsCore \
-lActsPluginTGeo \
-lActsPluginRoot \
-lActsExamplesDetectorTGeo \
-lActsExamplesFramework
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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.am

Repository: 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 \

Comment on lines +118 to +128
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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

# First, let's locate and examine the file
fd -i "phactsKDTreeSeeding" -e cc -e h

Repository: 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 -100

Repository: 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 -50

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 1867


🏁 Script executed:

# Search for SeedContainer definition in Acts includes
rg "SeedContainer" offline/packages/trackreco/PHActsKDTreeSeeding.cc | head -20

Repository: 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 -50

Repository: 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.h

Repository: 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.h

Repository: 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 h

Repository: 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' -e

Repository: 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 -50

Repository: 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().

Comment on lines +213 to 214
auto spVec = getSiliconSpacePoints(strobe);
eventTimer->stop();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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

Comment on lines +261 to +262
std::array<std::vector<std::size_t>, 3ul> navigation;
navigation[1ul] = m_seedFinderCfg.zBinsCustomLooping;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

find . -name "PHActsSiliconSeeding.cc" -o -name "PHActsSiliconSeeding.h" | head -20

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 183


🏁 Script executed:

cd offline/packages/trackreco 2>/dev/null && wc -l PHActsSiliconSeeding.cc && head -30 PHActsSiliconSeeding.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 983


🏁 Script executed:

cd offline/packages/trackreco 2>/dev/null && sed -n '255,270p' PHActsSiliconSeeding.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 732


🏁 Script executed:

cd offline/packages/trackreco 2>/dev/null && sed -n '1475,1550p' PHActsSiliconSeeding.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 2658


🏁 Script executed:

cd offline/packages/trackreco && grep -n "configureSeeder\|configureSPGrid\|toInternalUnits\|calculateDerivedQuantities" PHActsSiliconSeeding.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 226


🏁 Script executed:

cd offline/packages/trackreco && grep -n "zBinsCustomLooping" PHActsSiliconSeeding.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 140


🏁 Script executed:

cd offline/packages/trackreco && grep -n "m_seedFinderCfg\|m_gridCfg\|m_gridOptions" PHActsSiliconSeeding.cc | head -40

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 2263


🏁 Script executed:

cd offline/packages/trackreco && sed -n '80,105p' PHActsSiliconSeeding.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 793


🏁 Script executed:

cd offline/packages/trackreco && grep -n "zBinsCustomLooping" PHActsSiliconSeeding.h

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 60


🏁 Script executed:

cd offline/packages/trackreco && grep -n "m_seedFinderCfg\|m_gridCfg\|m_gridOptions" PHActsSiliconSeeding.h | head -20

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 261


🏁 Script executed:

cd offline/packages/trackreco && sed -n '190,210p' PHActsSiliconSeeding.cc

Repository: 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 -20

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 60


🏁 Script executed:

cd offline/packages/trackreco && head -100 PHActsSiliconSeeding.h | tail -50

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 1260


🏁 Script executed:

cd offline/packages/trackreco && grep -n "Acts::SeedFinder\|Acts::CylindricalSpacePointGrid" PHActsSiliconSeeding.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 436


🏁 Script executed:

cd offline/packages/trackreco && grep -rn "Acts" PHActsSiliconSeeding.h | grep "include" | head -10

Repository: 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 -5

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 60


🏁 Script executed:

cd offline/packages/trackreco && sed -n '235,270p' PHActsSiliconSeeding.cc

Repository: 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.cc

Repository: 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 -10

Repository: 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 -20

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 596


🏁 Script executed:

cd offline/packages/trackreco && sed -n '85,100p' PHActsSiliconSeeding.cc

Repository: 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 -5

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 60


🏁 Script executed:

cd offline/packages/trackreco && grep -B 5 "m_seedFinderCfg\s*=" PHActsSiliconSeeding.h

Repository: 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 -3

Repository: 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.cc

Repository: 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.cc

Repository: 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.

@sphenix-jenkins-ci
Copy link
Copy Markdown

Build & test report

Report for commit 33643778b9ebad971b30a74a1d62e8c403d92f10:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant