Conversation
…e into actsv44.0.0
…e into actsv44.0.0
…e into actsv44.0.0
…e into actsv44.0.0
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors the Acts/Acts Plugins integration by migrating measurement data structures from Acts to ActsExamples types, converting detector elements from Acts TGeo to ActsPlugins TGeo classes, simplifying geometry initialization, and adjusting track fitting configurations with new covariance scaling parameters. Changes
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
offline/packages/trackreco/MakeActsGeometry.cc (1)
720-763:⚠️ Potential issue | 🟠 MajorAdd error handling for material decorator initialization and validate before proceeding with geometry building.
The material decorator selection (lines 731–742) falls back to
Acts::MaterialWiper()whenevermaterialFilelacks a.jsonor.cborsubstring. SinceCDBInterface::getUrl()can return paths without standard extensions (e.g., symlinks or database URLs), the code can silently select vacuum geometry (MaterialWiperexplicitly nullifies all material) instead of failing. Additionally, neither theJsonMaterialDecoratorconstruction nor the subsequentTGeoDetectorbuild provides error checking beforeunpackVolumes()proceeds. For detector geometry with physics implications, treat unsupported or unloadable material as a hard error and validate the decorator was created successfully before continuing. This aligns with the coding guidelines: prioritize correctness, error handling, and safety for C++ code.offline/packages/trackbase/ActsGsfTrackFittingAlgorithm.h (1)
150-157:⚠️ Potential issue | 🟠 MajorKeep the new GSF knob source-compatible.
Line 65 already gives
reverseFilteringCovarianceScalinga100.default insideGsfFitterFunctionImpl, but the public declaration now makes every caller pass it explicitly. That is a needless compile-time break for downstream users of this header. Please either add the same default at the declaration boundary or spell out the required downstream updates in this PR.Compatibility-preserving declaration
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));As per coding guidelines,
**/*.{h,hpp,hxx,hh}: Focus on API clarity/stability, ownership semantics (RAII), and avoiding raw new/delete. If interfaces change, ask for compatibility notes and any needed downstream updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5242314e-7733-4a61-8d69-ed2f06d99d23
📒 Files selected for processing (45)
calibrations/tpc/TpcDVCalib/Makefile.amcalibrations/tpc/TpcDVCalib/TrackToCalo.ccoffline/packages/TrackerMillepedeAlignment/MakeMilleFiles.ccoffline/packages/TrackerMillepedeAlignment/MakeMilleFiles.hoffline/packages/trackbase/ActsGsfTrackFittingAlgorithm.hoffline/packages/trackbase/ActsTrackFittingAlgorithm.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/sPHENIXActsDetectorElement.ccoffline/packages/trackbase/sPHENIXActsDetectorElement.hoffline/packages/trackbase_historic/ActsTransformations.ccoffline/packages/trackbase_historic/ActsTransformations.hoffline/packages/trackbase_historic/TrackAnalysisUtils.hoffline/packages/trackreco/ActsAlignmentStates.ccoffline/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.hsimulation/g4simulation/g4tpc/PHG4TpcPadPlaneReadout.ccsimulation/g4simulation/g4tpc/TpcClusterBuilder.cc
💤 Files with no reviewable changes (4)
- offline/packages/trackreco/PHActsGSF.h
- offline/packages/trackreco/PHActsTrkFitter.h
- calibrations/tpc/TpcDVCalib/Makefile.am
- offline/packages/trackreco/PHCosmicsTrkFitter.h
| using SpacePointContainerType = ActsExamples::SpacePointContainer<std::vector<const SpacePoint *>>; | ||
| using proxy_type = typename Acts::SpacePointContainer<SpacePointContainerType, Acts::detail::RefHolder>::SpacePointProxyType; | ||
|
|
||
| using SpacePointPtr = std::unique_ptr<SpacePoint>; | ||
| using SeedContainer = std::vector<Acts::Seed<SpacePoint>>; | ||
| using SpacePointContainer = std::vector<SpacePoint>; | ||
| using SeedContainer = std::vector<Acts::Seed<proxy_type>>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and inspect the SpacePoint.h file
find . -name "SpacePoint.h" -type f 2>/dev/null | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 119
🏁 Script executed:
# Once located, read the relevant lines with context
if [ -f "./offline/packages/trackbase/SpacePoint.h" ]; then
cat -n ./offline/packages/trackbase/SpacePoint.h | sed -n '30,60p'
fiRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1062
🏁 Script executed:
# Search for usage of SeedContainer in the codebase
rg "SeedContainer" --type h --type hpp --type cpp -B 2 -A 2 | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 105
🏁 Script executed:
# Check for any ACTS container/proxy definitions or comments in the codebase
rg "Acts::Seed.*proxy_type\|SpacePointContainer.*RefHolder" --type h --type hpp -B 1 -A 1Repository: sPHENIX-Collaboration/coresoftware
Length of output: 105
🏁 Script executed:
# Look for any lifetime documentation or comments around these aliases
rg "proxy|non-owning|lifetime|dangling" ./offline/packages/trackbase/ --type h --type hpp -i -B 1 -A 1 | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 105
🏁 Script executed:
# Search for SeedContainer usage with correct syntax
rg "SeedContainer" -A 3 -B 1 2>/dev/null | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 5965
🏁 Script executed:
# Look for where SeedContainer is created or populated
rg "SeedContainer.*=" -A 2 2>/dev/null | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3286
🏁 Script executed:
# Check the Acts library types being used - search for RefHolder or proxy usage patterns
rg "RefHolder|SpacePointProxyType" -A 1 -B 1 2>/dev/null | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2086
🏁 Script executed:
# Look for any comments or documentation about lifetime/ownership in the trackbase package
find ./offline/packages/trackbase -type f -name "*.h" | xargs grep -l "lifetime\|non-owning\|dangling\|ownership" 2>/dev/nullRepository: sPHENIX-Collaboration/coresoftware
Length of output: 160
🏁 Script executed:
# Check if there are patterns where the underlying SpacePoint storage is moved/modified after SeedContainer creation
rg "spacePoints\|SpacePoint.*=" ./offline/packages/trackbase --type c -B 2 -A 2 2>/dev/null | head -60Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check the implementation of fillTrackSeedContainer and runSeeder to see if there are lifetime contracts
rg "fillTrackSeedContainer|runSeeder" -A 10 --type c 2>/dev/null | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2099
🏁 Script executed:
# Look at PHActsKDTreeSeeding implementation for how SeedContainer is managed
find . -name "PHActsKDTreeSeeding.cc" -type f -exec head -200 {} \; 2>/dev/null | grep -A 15 "runSeeder\|fillTrackSeedContainer" | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1378
This proxy-based SeedContainer design introduces undefined behavior in actual usage.
In PHActsKDTreeSeeding::runSeeder(), the spacePoints vector is local and destroyed at function return, yet the returned SeedContainer contains Acts::Seed<proxy_type> proxies pointing into that storage. When fillTrackSeedContainer() dereferences these proxies (e.g., sps[spid]->externalSpacePoint()), the underlying memory is already freed. This violates memory safety.
The exported SeedContainer type in SpacePoint.h (lines 44-49) should not use RefHolder proxies at the API boundary. Either keep the proxy type internal to seeding implementation and expose owning containers to callers, or refactor runSeeder() to return owning seeds based on value-type SpacePoint rather than non-owning proxies.
| class sPHENIXActsDetectorElement : public ActsPlugins::TGeoDetectorElement | ||
| { | ||
| public: | ||
| using Identifier = ActsPlugins::TGeoDetectorElement::Identifier; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the header file
find . -name "sPHENIXActsDetectorElement.h" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 135
🏁 Script executed:
# Read the header file to verify the structure
cat -n offline/packages/trackbase/sPHENIXActsDetectorElement.h | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3377
🏁 Script executed:
# Check the file for the factory signature around lines 61-67
sed -n '55,75p' offline/packages/trackbase/sPHENIXActsDetectorElement.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 742
🏁 Script executed:
# Check if ActsPlugins::TGeoDetectorElement exists and what it provides
fd "TGeoDetectorElement" --type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Verify the downstream usage in MakeActsGeometry.cc
rg "sPHENIXElementFactory" -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 311
Add compatibility notes documenting the detector-element API migration.
This header exports a breaking API change: sPHENIXActsDetectorElement now inherits from ActsPlugins::TGeoDetectorElement instead of a previous Acts base, and the public Identifier type alias (line 19) and factory signature (lines 61–67) bind to the ActsPlugins namespace. Downstream packages that consume sPHENIXElementFactory (see MakeActsGeometry.cc:725) must be explicitly informed of the required migration path, type changes, and rebuild requirements. Per the coding guidelines for header files: if interfaces change, ask for compatibility notes and any needed downstream updates.
Also applies to: 61–67
| 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.
Don't silently drop caller-configured overstep limits.
Lines 83-84 and 98-100 in offline/packages/trackreco/ActsPropagator.h still expose setOverstepLimit() and store m_overstepLimit, but makePropagator() now builds Stepper stepper(field);. Any non-default limit a caller set is ignored from here on, which changes propagation behavior without warning. Please either thread m_overstepLimit through the new v44 stepper setup or retire the setter in the same PR.
As per coding guidelines, **/*.{cc,cpp,cxx,c}: Prioritize correctness, memory safety, error handling, and thread-safety.
| ACTS_LIBS = \ | ||
| -lActsCore \ | ||
| -lActsPluginTGeo \ | ||
| -lActsPluginRoot \ | ||
| -lActsExamplesDetectorTGeo \ | ||
| -lActsExamplesFramework |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ACTS_LIBS is referenced anywhere in the Makefile
rg -n 'ACTS_LIBS' offline/packages/trackreco/Makefile.amRepository: sPHENIX-Collaboration/coresoftware
Length of output: 95
🏁 Script executed:
sed -n '120,201p' offline/packages/trackreco/Makefile.am | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2457
Remove or integrate the unused ACTS_LIBS variable.
The ACTS_LIBS variable (lines 120–124) is defined but never referenced with $(ACTS_LIBS) in the Makefile. The libtrack_reco_la_LIBADD section directly lists -lActsCore, -lActsExamplesDetectorTGeo, and -lActsExamplesFramework individually, but does not include -lActsPluginRoot.
Since ACTS_LIBS is not consumed, the change to -lActsPluginRoot has no effect on the build. Either add $(ACTS_LIBS) to LIBADD if these flags are needed, or remove the unused variable entirely to avoid configuration drift.
| 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.
Do not return proxy seeds after destroying their backing container.
SeedContainer now stores Acts::Seed<proxy_type>, and Lines 44-49 in offline/packages/trackbase/SpacePoint.h define proxy_type as a SpacePointProxyType with Acts::detail::RefHolder. Here container/spContainer are stack locals in runSeeder(), but the returned seeds are dereferenced later in fillTrackSeedContainer() via seed.sp() and externalSpacePoint(). After runSeeder() returns, those proxies no longer have a valid backing container, so this is a use-after-free/UB path. Keep the backing container alive until after seed consumption, or finish converting to TrackSeed before leaving runSeeder().
As per coding guidelines, **/*.{cc,cpp,cxx,c}: Prioritize correctness, memory safety, error handling, and thread-safety.
Also applies to: 145-148
| Acts::SpacePointContainerConfig spConfig; | ||
| spConfig.useDetailedDoubleMeasurementInfo = | ||
| m_seedFinderCfg.useDetailedDoubleMeasurementInfo; | ||
| // Options | ||
| // TODO - check beam pos information | ||
| Acts::SpacePointContainerOptions spOptions; | ||
| spOptions.beamPos = {0., 0.}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd offline/packages/trackreco && wc -l PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 106
🏁 Script executed:
head -230 offline/packages/trackreco/PHActsSiliconSeeding.cc | tail -30Repository: sPHENIX-Collaboration/coresoftware
Length of output: 939
🏁 Script executed:
rg "m_beamSpot" offline/packages/trackreco/PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 148
🏁 Script executed:
rg "spOptions\.beamPos" offline/packages/trackreco/PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 111
🏁 Script executed:
rg "class PHActsSiliconSeeding" offline/packages/trackreco/ -A 30Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2381
🏁 Script executed:
rg "m_beamSpot" offline/packages/trackreco/PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 192
🏁 Script executed:
grep -n "float m_beamSpot" offline/packages/trackreco/PHActsSiliconSeeding.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 158
🏁 Script executed:
rg "getSiliconSpacePoints" offline/packages/trackreco/PHActsSiliconSeeding.cc -A 5 -B 2 | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 578
🏁 Script executed:
rg "getSiliconSpacePoints" offline/packages/trackreco/PHActsSiliconSeeding.cc -A 80 | head -120Repository: sPHENIX-Collaboration/coresoftware
Length of output: 4163
🏁 Script executed:
rg "SpacePoint" offline/packages/trackreco/PHActsSiliconSeeding.cc | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1453
Thread the configured beam position into spOptions.beamPos.
The Acts SpacePointContainer::initialize() subtracts beamPos from every space-point x/y coordinate before recomputing radius and phi. Hard-coding {0., 0.} causes the seeding coordinates to disagree with the rest of the module whenever m_beamSpotx or m_beamSpoty are non-zero (defaults: -0.072 cm, 0.141 cm). This mismatch shifts impact parameter and phi cuts, affecting seed acceptance. Change spOptions.beamPos = {0., 0.}; to spOptions.beamPos = {m_beamSpotx, m_beamSpoty}; to ensure consistent coordinate systems throughout.
| ActsExamples::SpacePointContainer container(spVec); | ||
| // Prepare Acts API | ||
| SpacePointContainerRefHolder | ||
| spContainer(spConfig, spOptions, container); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and the specific lines mentioned
cd offline/packages/trackreco && \
wc -l PHActsSiliconSeeding.cc && \
echo "=== Lines 220-240 ===" && \
sed -n '220,240p' PHActsSiliconSeeding.cc && \
echo "=== Lines 270-320 ===" && \
sed -n '270,320p' PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2366
🏁 Script executed:
# Also check the getSiliconSpacePoints function and context around lines 1416-1461
echo "=== Lines 1410-1470 ===" && \
sed -n '1410,1470p' offline/packages/trackreco/PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1773
🏁 Script executed:
# Check the full runSeeder function to understand the scope and lifetime
rg -n "void PHActsSiliconSeeding::runSeeder" offline/packages/trackreco/PHActsSiliconSeeding.cc -A 200 | head -250Repository: sPHENIX-Collaboration/coresoftware
Length of output: 7496
🏁 Script executed:
# Search for manual delete operations on spVec entries
rg -n "delete|spVec" offline/packages/trackreco/PHActsSiliconSeeding.cc -B 2 -A 2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1787
🏁 Script executed:
# Check for beam position hard-coding and beamPos usage
rg -n "beamPos|beamSpot" offline/packages/trackreco/PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 267
🏁 Script executed:
# Check what makeSvtxTracks actually does with the seeds
# Look for whether TrackSeed objects store pointers back to space points
sed -n '328,450p' offline/packages/trackreco/PHActsSiliconSeeding.cc | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 4020
🏁 Script executed:
# Check if BeamSpot initialization happens or if it's always (0,0)
rg -n "m_beamSpot|BeamSpot" offline/packages/trackreco/PHActsSiliconSeeding.cc | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 918
🏁 Script executed:
# Look for the return statement and actual scope of container/spVec
sed -n '300,330p' offline/packages/trackreco/PHActsSiliconSeeding.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 913
Replace manual heap management with RAII to prevent memory leaks on exception.
The SpacePoint objects are allocated via release() (line 1460) and stored in spVec, but deletion at lines 307–311 lacks exception protection. If an exception occurs during seed creation (lines 283–289) or track processing (line 301), the heap is leaked. Use a container type (e.g., std::vector<std::unique_ptr<SpacePoint>>) to manage lifetime automatically, then pass a non-owning view to ActsExamples::SpacePointContainer.
Also verify beam position initialization.
Line 228 hard-codes spOptions.beamPos = {0., 0.}, but the code still references beam position via getPhiFromBeamSpot() (lines 817, 913, 924, 1154, 1191, 1201), which uses m_beamSpotx and m_beamSpoty. Either initialize spOptions.beamPos from the detector state, or remove the hard-coded TODO comment if origin-only is intentional.
Also applies to: lines 276–311, 1416–1461
1 participant
This updates us to Acts v44.0.0. There are a lot of moving parts so I'm opening PRs to try to test with jenkins, but the simulation pipeline runs as normal at least. There is a daily test build that I will encourage people to start trying next week.
This gets us about 1.5 years of code development and some features we actually care about (e.g. bugs/things we found that we communicated back to the developers and they fixed).
This PR requires changes in Acts, our build script, and the CDB, so it is quite a monster
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)
sPHENIX-Collaboration/acts#44
sPHENIX-Collaboration/utilities#430