Implement the ability to tilt the TPC envelope in sPHENIX#4308
Conversation
…pts to handle the fallout in the simulation and reconstruction code.
…esoftware into tpc_tilt_simulation2
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds TPC world↔envelope transforms, introduces PHG4TpcGeomv2, and updates detector, reconstruction, alignment, drift, and truth code to use the new coordinate mapping. ChangesTPC world/envelope coordinate transform framework
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 |
|
I should have mentioned that I temporarily set the default value of the rotation of the TPC around the x axis to be -4 mrad, so that Jenkins is actually checking the effect of adding a tilt. |
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 (2)
simulation/g4simulation/g4eval/SvtxTruthEval.cc (1)
572-586:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftOutput coordinate frame inconsistency for
contributing_hits_entry/contributing_hits_exit.The entry/exit position vectors are populated with envelope-frame coordinates (
xin,yin,zin,xout,yout,zoutderived fromenv0/env1), while the main cluster position outputs (x,y,z) are converted back to world coordinates at lines 670–675. Additionally, the non-TPC branch (lines 697–709) fills these vectors with world coordinates fromthis_g4hit->get_x/y/z().If any caller relies on
contributing_hits_entry/contributing_hits_exitbeing in world coordinates, this will produce incorrect results. Consider either:
- Converting these vectors back to world before returning (consistency with
x,y,z), or- Documenting the coordinate frame contract in the function's interface so callers know TPC entry/exit are envelope-frame while non-TPC are world-frame.
offline/packages/tpc/TpcClusterizer.cc (1)
749-773:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTransform NN envelope coordinates back to world before projecting to the surface.
Line 767 builds
nn_globalfrom envelope-frame values (nn_x/nn_y/nn_z) and then applies a world-frame surface transform. For non-zero TPC tilt this yields biased local coordinates. Convert envelope→world first, then do the local projection.Suggested fix
- Acts::Vector3 nn_global(nn_x, nn_y, nn_z); - nn_global *= Acts::UnitConstants::cm; + Acts::Vector3 nn_env_global(nn_x, nn_y, nn_z); + Acts::Vector3 nn_global = my_data.tGeometry->transformTpcEnvelopeToWorld(nn_env_global); + nn_global *= Acts::UnitConstants::cm; Acts::Vector3 nn_local = surface->localToGlobalTransform(my_data.tGeometry->geometry().geoContext).inverse() * nn_global;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c9918f0-a094-4d38-9630-64c0402e94ab
📒 Files selected for processing (24)
offline/packages/TrackingDiagnostics/TrackResiduals.ccoffline/packages/tpc/TpcClusterMover.ccoffline/packages/tpc/TpcClusterMover.hoffline/packages/tpc/TpcClusterizer.ccoffline/packages/trackbase/ActsGeometry.ccoffline/packages/trackbase/ActsGeometry.hoffline/packages/trackbase/AlignmentTransformation.ccoffline/packages/trackbase_historic/TrackAnalysisUtils.ccoffline/packages/trackreco/MakeActsGeometry.ccoffline/packages/trackreco/MakeActsGeometry.hoffline/packages/trackreco/MakeSourceLinks.ccoffline/packages/trackreco/MakeSourceLinks.hoffline/packages/trackreco/PHActsTrkFitter.ccoffline/packages/trackreco/PHCosmicsTrkFitter.ccsimulation/g4simulation/g4detectors/Makefile.amsimulation/g4simulation/g4detectors/PHG4TpcGeom.hsimulation/g4simulation/g4detectors/PHG4TpcGeomv2.ccsimulation/g4simulation/g4detectors/PHG4TpcGeomv2.hsimulation/g4simulation/g4detectors/PHG4TpcGeomv2LinkDef.hsimulation/g4simulation/g4eval/SvtxTruthEval.ccsimulation/g4simulation/g4tpc/PHG4TpcDetector.ccsimulation/g4simulation/g4tpc/PHG4TpcElectronDrift.ccsimulation/g4simulation/g4tpc/PHG4TpcElectronDrift.hsimulation/g4simulation/g4tpc/TpcClusterBuilder.cc
|
|
||
| _tGeometry = tGeometry; | ||
|
|
There was a problem hiding this comment.
Enforce geometry initialization before transform calls
_tGeometry is dereferenced at Line 87 and Line 150 without a guaranteed non-null invariant. If initialization is skipped or receives null, this becomes a deterministic crash path. Validate inputs in initialize_geometry and fail fast in processTrack before any transform use.
Proposed fix
void TpcClusterMover::initialize_geometry(PHG4TpcGeomContainer* cellgeo, ActsGeometry *tGeometry)
{
+ if (!cellgeo || !tGeometry)
+ {
+ _tGeometry = nullptr;
+ if (_verbosity > 0)
+ {
+ std::cout << "TpcClusterMover::initialize_geometry missing required geometry input" << std::endl;
+ }
+ return;
+ }
+
if (_verbosity > 0)
{
std::cout << "TpcClusterMover: Initializing layer radii for Tpc from cell geometry object" << std::endl;
}
-
_tGeometry = tGeometry;
-
+
int layer = 0;
PHG4TpcGeomContainer::ConstRange layerrange = cellgeo->get_begin_end();
@@
std::vector<std::pair<TrkrDefs::cluskey, Acts::Vector3>> TpcClusterMover::processTrack(const std::vector<std::pair<TrkrDefs::cluskey, Acts::Vector3>>& global_in)
{
+ if (!_tGeometry)
+ {
+ if (_verbosity > 0)
+ {
+ std::cout << "TpcClusterMover::processTrack called before valid geometry initialization" << std::endl;
+ }
+ return global_in;
+ }
+
// Get the global positions of the TPC clusters for this track, already corrected for distortions, and move them to the surfacesAlso applies to: 87-88, 148-150
| auto* geometry = findNode::getClass<ActsGeometry>(topNode, "ActsGeometry"); | ||
|
|
||
| TpcClusterMover mover; | ||
| auto* tpccellgeo = findNode::getClass<PHG4TpcGeomContainer>(topNode, "TPCGEOMCONTAINER"); | ||
| mover.initialize_geometry(tpccellgeo); | ||
| mover.initialize_geometry(tpccellgeo, geometry); |
There was a problem hiding this comment.
Guard ActsGeometry/TPCGEOMCONTAINER lookups before using TpcClusterMover.
Lines 312-316 can initialize the mover with null pointers; subsequent calls dereference geometry and can crash this utility path. Add a precondition check and return empty residuals if prerequisites are missing.
Suggested fix
auto* geometry = findNode::getClass<ActsGeometry>(topNode, "ActsGeometry");
TpcClusterMover mover;
auto* tpccellgeo = findNode::getClass<PHG4TpcGeomContainer>(topNode, "TPCGEOMCONTAINER");
+ if (!geometry || !tpccellgeo)
+ {
+ std::cout << PHWHERE << " missing required geometry node(s) for residual calculation" << std::endl;
+ return residuals;
+ }
mover.initialize_geometry(tpccellgeo, geometry);📝 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.
| auto* geometry = findNode::getClass<ActsGeometry>(topNode, "ActsGeometry"); | |
| TpcClusterMover mover; | |
| auto* tpccellgeo = findNode::getClass<PHG4TpcGeomContainer>(topNode, "TPCGEOMCONTAINER"); | |
| mover.initialize_geometry(tpccellgeo); | |
| mover.initialize_geometry(tpccellgeo, geometry); | |
| auto* geometry = findNode::getClass<ActsGeometry>(topNode, "ActsGeometry"); | |
| TpcClusterMover mover; | |
| auto* tpccellgeo = findNode::getClass<PHG4TpcGeomContainer>(topNode, "TPCGEOMCONTAINER"); | |
| if (!geometry || !tpccellgeo) | |
| { | |
| std::cout << PHWHERE << " missing required geometry node(s) for residual calculation" << std::endl; | |
| return residuals; | |
| } | |
| mover.initialize_geometry(tpccellgeo, geometry); |
| auto *tpccellgeo = findNode::getClass<PHG4TpcGeomContainer>(topNode, "TPCGEOMCONTAINER"); | ||
| m_clusterMover.initialize_geometry(tpccellgeo); | ||
|
|
||
| auto *geometry = findNode::getClass<ActsGeometry>(topNode, "ActsGeometry"); | ||
| m_clusterMover.initialize_geometry(tpccellgeo, geometry); |
There was a problem hiding this comment.
Validate required geometry nodes before initializing m_clusterMover.
Line 120 passes tpccellgeo/geometry without null checks. If either lookup fails, processTrack will dereference a null geometry pointer and crash. Fail fast in InitRun with ABORTRUN.
Suggested fix
auto *tpccellgeo = findNode::getClass<PHG4TpcGeomContainer>(topNode, "TPCGEOMCONTAINER");
auto *geometry = findNode::getClass<ActsGeometry>(topNode, "ActsGeometry");
+ if (!tpccellgeo || !geometry)
+ {
+ std::cout << PHWHERE << " missing required geometry node(s): "
+ << (tpccellgeo ? "" : "TPCGEOMCONTAINER ")
+ << (geometry ? "" : "ActsGeometry") << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
m_clusterMover.initialize_geometry(tpccellgeo, geometry);📝 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.
| auto *tpccellgeo = findNode::getClass<PHG4TpcGeomContainer>(topNode, "TPCGEOMCONTAINER"); | |
| m_clusterMover.initialize_geometry(tpccellgeo); | |
| auto *geometry = findNode::getClass<ActsGeometry>(topNode, "ActsGeometry"); | |
| m_clusterMover.initialize_geometry(tpccellgeo, geometry); | |
| auto *tpccellgeo = findNode::getClass<PHG4TpcGeomContainer>(topNode, "TPCGEOMCONTAINER"); | |
| auto *geometry = findNode::getClass<ActsGeometry>(topNode, "ActsGeometry"); | |
| if (!tpccellgeo || !geometry) | |
| { | |
| std::cout << PHWHERE << " missing required geometry node(s): " | |
| << (tpccellgeo ? "" : "TPCGEOMCONTAINER ") | |
| << (geometry ? "" : "ActsGeometry") << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| m_clusterMover.initialize_geometry(tpccellgeo, geometry); |
Build & test reportReport for commit e08f803676c1f09f9d7eeccd35a72c5ef9841a94:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
left a comment
There was a problem hiding this comment.
The simulation QA looks remarkably consistent so it appears nothing has broken, however the reconstruction run over the data has problems looking up the geometry in the Acts geometry building. This might just be a CDB issue that a different CDB geometry file is being loaded (or one that is inconsistent with the changed code?)
|
There is a problem with the surface maps when the TPC is tilted, I am looking into it.
Tony
…________________________________
From: Joseph (Joe) Osborn ***@***.***>
Sent: Monday, June 22, 2026 9:13 PM
To: sPHENIX-Collaboration/coresoftware ***@***.***>
Cc: Anthony Frawley ***@***.***>; Author ***@***.***>
Subject: Re: [sPHENIX-Collaboration/coresoftware] Implement the ability to tilt the TPC envelope in sPHENIX (PR #4308)
@osbornjd commented on this pull request.
The simulation QA looks remarkably consistent so it appears nothing has broken, however the reconstruction run over the data has problems looking up the geometry in the Acts geometry building. This might just be a CDB issue that a different CDB geometry file is being loaded (or one that is inconsistent with the changed code?)
—
Reply to this email directly, view it on GitHub<#4308?email_source=notifications&email_token=ACHADBNNJMMJ3YYZUMWIULD5BHKSHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJUHEZDIMJQHA22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4549241085>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHADBJEM4SR26BBOOJ63CD5BHKSHAVCNFSNUABEKJSXA33TNF2G64TZHMZTINZUGI2DGMR3JFZXG5LFHM2DOMBVGQZDONZXGWQXMAQ>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/ACHADBNMHBEOHI7EBRMHMED5BHKSHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJUHEZDIMJQHA22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android<https://github.com/notifications/mobile/android/ACHADBM73KUG5WS4JRYRYXL5BHKSHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJUHEZDIMJQHA22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today!
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
…dd ActsGeom for envelope transform. TpcClusterizer, use envelope coords where appropriate. AlignmentTransformation, redo sskey finding. Use envelope center coords in module tilt section. Add PHG4TpcGeom, add g4detectors to makefile. PHG4TpcEndCapDetector, change rot_x etc from deg to rad. TpcClusterBuilder, add check for adc_sum = 0.
…are into tpc_tilt_simulation2
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
simulation/g4simulation/g4detectors/PHG4TpcGeomContainer.h (1)
12-17: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore a real
PHG4TpcGeomdeclaration in this header.
#include <g4detectors/PHG4TpcGeomContainer.h>just re-includes the current header, so the include guard makes it a no-op.PHG4TpcGeomis then undeclared at Line 17, which breaks any translation unit that includes this header without another incidental declaration.Suggested fix
-#include <g4detectors/PHG4TpcGeomContainer.h> +#include <g4detectors/PHG4TpcGeom.h>If you want to keep this header lighter, a forward declaration also works here:
-#include <g4detectors/PHG4TpcGeomContainer.h> +class PHG4TpcGeom;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 566b6433-a24e-4478-bfb2-9e5f82865844
📒 Files selected for processing (21)
offline/QA/Tracking/TpcSeedsQA.ccoffline/packages/tpc/TpcClusterMover.ccoffline/packages/tpc/TpcClusterizer.ccoffline/packages/trackbase/ActsGeometry.ccoffline/packages/trackbase/AlignmentTransformation.ccoffline/packages/trackbase/AlignmentTransformation.hoffline/packages/trackbase/Makefile.amoffline/packages/trackbase_historic/TrackAnalysisUtils.ccoffline/packages/trackreco/MakeActsGeometry.ccoffline/packages/trackreco/PHActsTrkFitter.ccoffline/packages/trackreco/PHCASeeding.ccsimulation/g4simulation/g4detectors/PHG4TpcGeom.hsimulation/g4simulation/g4detectors/PHG4TpcGeomContainer.hsimulation/g4simulation/g4detectors/PHG4TpcGeomv2.ccsimulation/g4simulation/g4detectors/PHG4TpcGeomv2.hsimulation/g4simulation/g4eval/SvtxTruthEval.ccsimulation/g4simulation/g4tpc/PHG4TpcDetector.ccsimulation/g4simulation/g4tpc/PHG4TpcEndCapDetector.ccsimulation/g4simulation/g4tpc/PHG4TpcEndCapSubsystem.ccsimulation/g4simulation/g4tpc/PHG4TpcSubsystem.ccsimulation/g4simulation/g4tpc/TpcClusterBuilder.cc
✅ Files skipped from review due to trivial changes (1)
- offline/packages/trackreco/PHCASeeding.cc
🚧 Files skipped from review as they are similar to previous changes (9)
- offline/packages/trackreco/PHActsTrkFitter.cc
- offline/packages/trackbase_historic/TrackAnalysisUtils.cc
- simulation/g4simulation/g4eval/SvtxTruthEval.cc
- offline/packages/tpc/TpcClusterMover.cc
- simulation/g4simulation/g4tpc/PHG4TpcDetector.cc
- offline/packages/tpc/TpcClusterizer.cc
- offline/packages/trackbase/ActsGeometry.cc
- offline/packages/trackreco/MakeActsGeometry.cc
- simulation/g4simulation/g4detectors/PHG4TpcGeomv2.cc
| rotm_center.rotateX(m_Params->get_double_param("rot_x") * rad); | ||
| rotm_center.rotateY(m_Params->get_double_param("rot_y") * rad); | ||
| rotm_center.rotateZ(m_Params->get_double_param("rot_z") * rad); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Don't silently reinterpret rot_x/rot_y/rot_z as radians.
These parameters are still documented upstream as degrees, but this code now consumes them as radians. That changes the meaning of every existing nonzero config for the same parameter names, so previously valid setups will build the wrong endcap orientation unless all callers are migrated in lockstep.
Suggested fix
- rotm_center.rotateX(m_Params->get_double_param("rot_x") * rad);
- rotm_center.rotateY(m_Params->get_double_param("rot_y") * rad);
- rotm_center.rotateZ(m_Params->get_double_param("rot_z") * rad);
+ rotm_center.rotateX(m_Params->get_double_param("rot_x") * deg);
+ rotm_center.rotateY(m_Params->get_double_param("rot_y") * deg);
+ rotm_center.rotateZ(m_Params->get_double_param("rot_z") * deg);If the new contract really needs radians, rename the parameters accordingly and update the downstream configuration/CDB inputs as part of the same change.
📝 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.
| rotm_center.rotateX(m_Params->get_double_param("rot_x") * rad); | |
| rotm_center.rotateY(m_Params->get_double_param("rot_y") * rad); | |
| rotm_center.rotateZ(m_Params->get_double_param("rot_z") * rad); | |
| rotm_center.rotateX(m_Params->get_double_param("rot_x") * deg); | |
| rotm_center.rotateY(m_Params->get_double_param("rot_y") * deg); | |
| rotm_center.rotateZ(m_Params->get_double_param("rot_z") * deg); |
…ilt hardcoded for this test only (so no macro changes are needed);
Build & test reportReport for commit d97ac63e576ec6cbfe6ab124bfe08adb54fb7b56: Automatically generated by sPHENIX Jenkins continuous integration |
…esoftware into tpc_tilt_simulation2
Build & test reportReport for commit 4d022dbf90c38b6e0def03aaf9bd71e2e339a2fb:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 275ed89cf99c0a8e7e72e6f67c78f2c4ece864ac:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 4b72c343e8c72fdc4083ee2ce48aafa04e82db3d:
Automatically generated by sPHENIX Jenkins continuous integration |
|
This passes all the CI and touches quite a lot so let's get it merged |




Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
This should be considered a draft PR for now. I want to see what the Jenkins QA finds.
It implements the ability to position the TPC envelope in sPHENIX simulations at an arbitrary tilt angle.
Many changes were needed to handle the tilt in simulation and in TPC fake surface handling. Generally, these consist of transforming world coordinates to tpc envelope coordinates so that the positions of the fake surfaces are known.
The tilt angles are added to PHTpcGeom in a new version (v2). The transformation from world to TPC envelope coordinates is added to ActsGeometry.
The basic simulation and reconstruction seems to work. There are likely calibration modules that need attention that I have not looked at yet.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation / context
Key changes
offline/packages/trackbase/ActsGeometry:transformTpcWorldToEnvelope(...)andtransformTpcEnvelopeToWorld(...).get_tpc_surface_from_coords(...)to use envelope-coordinate logic (including transforming the input world point into envelope coordinates) and to select candidate TPC surfaces by scanning surface centers in envelope space with side matching; improved diagnostics when a surface is not found.PHG4TpcGeomv2and extendedPHG4TpcGeomwith rotation/placement getters/setters (virtual API).tpc_envelopeusingG4Transform3Dwith rot/translation and to configure per-layer cell geometry viaPHG4TpcGeomv2(so tilt parameters/placement are threaded consistently).PHG4TpcGeomv2.MakeActsGeometry: reads per-layer rotation (rot_x/rot_y/rot_z), builds and stores the world↔envelope transform(s), and uses envelope-aware surface center handling when generating hitset keys.MakeSourceLinks/PHActsTrkFitter/PHCosmicsTrkFitter: initialization and geometry wiring updated to passActsGeometrythrough and to adjust surface ordering validation to be radius-based.TpcClusterizer: surface lookup uses envelope-coordinates/tilt-removed conventions; NN refinement path converts NN-predicted envelope coordinates back to world/global before downstream local transformations.TpcClusterBuilder/TpcClusterMover/ residual/QA paths: cluster points transformed through envelope coordinates before/after operations (surface association, moved-position construction, and fitting).SvtxTruthEvalboundary/intersection and endpoint ordering now uses envelope-transformed entry/exit; final cluster coordinates are converted back to world.PHG4TpcElectronDriftdrifts in envelope coordinates by transforming start positions world→envelope.Potential risk areas
PHG4TpcGeomv2+ rotation/placement plumbing requires that the geometry artifacts used at runtime match the expected tilt/transform setup.get_tpc_surface_from_coords(...)selection was changed toward an explicit scan over candidate surfaces (per query), which may increase runtime depending on surface count/QA conditions.std::cout/transform-validation prints in some construction/utility code paths; these can affect QA logs and may complicate parallel runs if not gated.ActsGeometry*is passed/stored in several movers/builders; while the usage appears read-only, concurrency safety should be validated under the framework’s threading model.Possible future improvements
ActsGeometrytransforms andget_tpc_surface_from_coords(...)correctness under tilt across multiple layers/sectors/sides.