Skip to content

Implement the ability to tilt the TPC envelope in sPHENIX#4308

Merged
osbornjd merged 10 commits into
sPHENIX-Collaboration:masterfrom
adfrawley:tpc_tilt_simulation2
Jun 27, 2026
Merged

Implement the ability to tilt the TPC envelope in sPHENIX#4308
osbornjd merged 10 commits into
sPHENIX-Collaboration:masterfrom
adfrawley:tpc_tilt_simulation2

Conversation

@adfrawley

@adfrawley adfrawley commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [ x] 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, ...)

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

  • Enable simulation + reconstruction in sPHENIX with an arbitrarily tilted TPC by consistently converting between “TPC world” coordinates and “TPC envelope” coordinates across geometry-dependent steps (surface lookup, boundary intersections, drift, clustering, etc.).
  • This is intended to exercise the tilted-geometry path in QA (e.g., Jenkins) and surface any remaining geometry/CDB/assumption mismatches; calibration/alignment follow-ups may still be required.
  • AI can make mistakes—please use best judgment when reading this summary.

Key changes

  • Added TPC world↔envelope coordinate support in offline/packages/trackbase/ActsGeometry:
    • New helpers transformTpcWorldToEnvelope(...) and transformTpcEnvelopeToWorld(...).
    • Updated 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.
  • Introduced/extended TPC geometry for tilt (“v2”):
    • Added PHG4TpcGeomv2 and extended PHG4TpcGeom with rotation/placement getters/setters (virtual API).
    • Updated TPC detector construction to place the tpc_envelope using G4Transform3D with rot/translation and to configure per-layer cell geometry via PHG4TpcGeomv2 (so tilt parameters/placement are threaded consistently).
    • Build/ROOT dictionary integration added for PHG4TpcGeomv2.
  • Updated reconstruction/geometry-dependent logic to use envelope coordinates where required:
    • 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 pass ActsGeometry through 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).
    • Truth + drift: SvtxTruthEval boundary/intersection and endpoint ordering now uses envelope-transformed entry/exit; final cluster coordinates are converted back to world. PHG4TpcElectronDrift drifts in envelope coordinates by transforming start positions world→envelope.

Potential risk areas

  • Reconstruction behavior changes: surface association and hitset key generation now depend on envelope-coordinate transforms; any mismatch between the geometry loaded via CDB/geometry inputs and the transform conventions can cause incorrect surface matching (consistent with the reported tilted surface-map issue).
  • Geometry/CDB compatibility: new PHG4TpcGeomv2 + rotation/placement plumbing requires that the geometry artifacts used at runtime match the expected tilt/transform setup.
  • Performance: 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.
  • Debug output / determinism: there are new diagnostic std::cout/transform-validation prints in some construction/utility code paths; these can affect QA logs and may complicate parallel runs if not gated.
  • Thread-safety assumptions: 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

  • Resolve remaining follow-ups:
    • Investigate the reported surface-map problem when the TPC is tilted.
    • Address reconstruction geometry lookup issues potentially caused by CDB/geometry inconsistencies.
  • Add targeted validation:
    • Focused tests for ActsGeometry transforms and get_tpc_surface_from_coords(...) correctness under tilt across multiple layers/sectors/sides.
  • Cleanup/tuning for production:
    • Reduce or gate debug/diagnostic output and temporary tilt configurations once QA confirms correctness.
  • Broaden cross-checks:
    • Verify calibration/alignment modules that may still implicitly assume non-tilted/world-only conventions are updated to use the same envelope/world conventions.

@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds TPC world↔envelope transforms, introduces PHG4TpcGeomv2, and updates detector, reconstruction, alignment, drift, and truth code to use the new coordinate mapping.

Changes

TPC world/envelope coordinate transform framework

Layer / File(s) Summary
PHG4TpcGeomv2 interface and build wiring
simulation/g4simulation/g4detectors/PHG4TpcGeom*, simulation/g4simulation/g4detectors/Makefile.am
Extends the TPC geometry interfaces with rotation and placement accessors, defines PHG4TpcGeomv2, implements its binning and validation methods, and registers the new class in the ROOT and build wiring.
TPC detector placement and geometry propagation
simulation/g4simulation/g4tpc/PHG4TpcDetector.cc, simulation/g4simulation/g4tpc/PHG4TpcEndCapDetector.cc, simulation/g4simulation/g4tpc/PHG4Tpc*Subsystem.cc
Switches TPC geometry construction to PHG4TpcGeomv2, applies explicit rotation and placement in detector construction, and changes the default rotation comments used by the TPC subsystems.
ActsGeometry transform API and surface lookup
offline/packages/trackbase/ActsGeometry.*, offline/packages/trackbase/Makefile.am
Adds TPC world/envelope transform storage and conversion helpers to ActsGeometry, updates TPC surface lookup to evaluate candidates in envelope coordinates, and adds the trackbase g4detectors link.
MakeActsGeometry transform construction and TPC map wiring
offline/packages/trackreco/MakeActsGeometry.*
Stores TPC transform members in MakeActsGeometry, builds the envelope/world transforms from layer placement, publishes the inverse to ActsGeometry, uses transformed surface centers when generating TPC hitset keys, and adjusts debug gating in the mapping paths.
TpcClusterMover initialization and cluster transforms
offline/packages/tpc/TpcClusterMover.*, offline/packages/trackreco/MakeSourceLinks.*, offline/packages/TrackingDiagnostics/TrackResiduals.cc, offline/packages/trackbase_historic/TrackAnalysisUtils.cc, offline/packages/trackreco/PHActsTrkFitter.cc, offline/packages/trackreco/PHCosmicsTrkFitter.cc, offline/QA/Tracking/TpcSeedsQA.cc
Propagates ActsGeometry into TpcClusterMover initialization, updates the residual and source-link callers, and converts TPC cluster positions into envelope space for fitting and back to world space for the returned clusters.
TPC clusterization, drift, and truth coordinate updates
offline/packages/tpc/TpcClusterizer.cc, simulation/g4simulation/g4tpc/TpcClusterBuilder.cc, simulation/g4simulation/g4tpc/PHG4TpcElectronDrift.*, offline/packages/trackreco/PHCASeeding.cc, simulation/g4simulation/g4eval/SvtxTruthEval.cc
Converts clusterization, drift, seeding, and truth-evaluation calculations between world and envelope coordinates before surface lookup and downstream accumulation.
Alignment geometry and module centers
offline/packages/trackbase/AlignmentTransformation.*
Fetches TPC cell geometry for alignment, generates fake module surfaces from envelope-space samples, and computes module center radii through world↔envelope conversions.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@adfrawley

Copy link
Copy Markdown
Contributor Author

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.

@coderabbitai coderabbitai Bot left a comment

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.

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 lift

Output 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, zout derived from env0/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 from this_g4hit->get_x/y/z().

If any caller relies on contributing_hits_entry / contributing_hits_exit being in world coordinates, this will produce incorrect results. Consider either:

  1. Converting these vectors back to world before returning (consistency with x, y, z), or
  2. 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 win

Transform NN envelope coordinates back to world before projecting to the surface.

Line 767 builds nn_global from 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

📥 Commits

Reviewing files that changed from the base of the PR and between 21f9fed and d8866f9.

📒 Files selected for processing (24)
  • offline/packages/TrackingDiagnostics/TrackResiduals.cc
  • offline/packages/tpc/TpcClusterMover.cc
  • offline/packages/tpc/TpcClusterMover.h
  • offline/packages/tpc/TpcClusterizer.cc
  • offline/packages/trackbase/ActsGeometry.cc
  • offline/packages/trackbase/ActsGeometry.h
  • offline/packages/trackbase/AlignmentTransformation.cc
  • offline/packages/trackbase_historic/TrackAnalysisUtils.cc
  • offline/packages/trackreco/MakeActsGeometry.cc
  • offline/packages/trackreco/MakeActsGeometry.h
  • offline/packages/trackreco/MakeSourceLinks.cc
  • offline/packages/trackreco/MakeSourceLinks.h
  • offline/packages/trackreco/PHActsTrkFitter.cc
  • offline/packages/trackreco/PHCosmicsTrkFitter.cc
  • simulation/g4simulation/g4detectors/Makefile.am
  • simulation/g4simulation/g4detectors/PHG4TpcGeom.h
  • simulation/g4simulation/g4detectors/PHG4TpcGeomv2.cc
  • simulation/g4simulation/g4detectors/PHG4TpcGeomv2.h
  • simulation/g4simulation/g4detectors/PHG4TpcGeomv2LinkDef.h
  • simulation/g4simulation/g4eval/SvtxTruthEval.cc
  • simulation/g4simulation/g4tpc/PHG4TpcDetector.cc
  • simulation/g4simulation/g4tpc/PHG4TpcElectronDrift.cc
  • simulation/g4simulation/g4tpc/PHG4TpcElectronDrift.h
  • simulation/g4simulation/g4tpc/TpcClusterBuilder.cc

Comment on lines +55 to +57

_tGeometry = tGeometry;

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

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 surfaces

Also applies to: 87-88, 148-150

Comment on lines +312 to +316
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);

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

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

Comment thread offline/packages/trackbase/ActsGeometry.cc Outdated
Comment thread offline/packages/trackbase/ActsGeometry.cc Outdated
Comment on lines 117 to +120
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);

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

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.

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

Comment thread simulation/g4simulation/g4detectors/PHG4TpcGeom.h
Comment thread simulation/g4simulation/g4detectors/PHG4TpcGeomv2.cc
Comment thread simulation/g4simulation/g4detectors/PHG4TpcGeomv2.cc
Comment thread simulation/g4simulation/g4detectors/PHG4TpcGeomv2.h Outdated
@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit d8866f95015265f7247127de1b7cd5ab12a2393e:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit e08f803676c1f09f9d7eeccd35a72c5ef9841a94:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@osbornjd osbornjd left a comment

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.

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?)

@adfrawley

adfrawley commented Jun 23, 2026 via email

Copy link
Copy Markdown
Contributor Author

…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.

@coderabbitai coderabbitai Bot left a comment

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.

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 win

Restore a real PHG4TpcGeom declaration in this header.

#include <g4detectors/PHG4TpcGeomContainer.h> just re-includes the current header, so the include guard makes it a no-op. PHG4TpcGeom is 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

📥 Commits

Reviewing files that changed from the base of the PR and between d8866f9 and f28f854.

📒 Files selected for processing (21)
  • offline/QA/Tracking/TpcSeedsQA.cc
  • offline/packages/tpc/TpcClusterMover.cc
  • offline/packages/tpc/TpcClusterizer.cc
  • offline/packages/trackbase/ActsGeometry.cc
  • offline/packages/trackbase/AlignmentTransformation.cc
  • offline/packages/trackbase/AlignmentTransformation.h
  • offline/packages/trackbase/Makefile.am
  • offline/packages/trackbase_historic/TrackAnalysisUtils.cc
  • offline/packages/trackreco/MakeActsGeometry.cc
  • offline/packages/trackreco/PHActsTrkFitter.cc
  • offline/packages/trackreco/PHCASeeding.cc
  • simulation/g4simulation/g4detectors/PHG4TpcGeom.h
  • simulation/g4simulation/g4detectors/PHG4TpcGeomContainer.h
  • simulation/g4simulation/g4detectors/PHG4TpcGeomv2.cc
  • simulation/g4simulation/g4detectors/PHG4TpcGeomv2.h
  • simulation/g4simulation/g4eval/SvtxTruthEval.cc
  • simulation/g4simulation/g4tpc/PHG4TpcDetector.cc
  • simulation/g4simulation/g4tpc/PHG4TpcEndCapDetector.cc
  • simulation/g4simulation/g4tpc/PHG4TpcEndCapSubsystem.cc
  • simulation/g4simulation/g4tpc/PHG4TpcSubsystem.cc
  • simulation/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

Comment thread offline/packages/trackbase/AlignmentTransformation.cc
Comment thread offline/packages/trackbase/AlignmentTransformation.cc
Comment on lines +97 to +99
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);

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.

🎯 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.

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

Comment thread simulation/g4simulation/g4tpc/PHG4TpcEndCapSubsystem.cc Outdated
Comment thread simulation/g4simulation/g4tpc/PHG4TpcSubsystem.cc Outdated
@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit f28f854877ca10ccde89b0969513349775d09c9c:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

…ilt hardcoded for this test only (so no macro changes are needed);
@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit d97ac63e576ec6cbfe6ab124bfe08adb54fb7b56:
Jenkins on fire


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 4d022dbf90c38b6e0def03aaf9bd71e2e339a2fb:
Jenkins passed


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 275ed89cf99c0a8e7e72e6f67c78f2c4ece864ac:
Jenkins passed


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit 4b72c343e8c72fdc4083ee2ce48aafa04e82db3d:
Jenkins passed


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@osbornjd

Copy link
Copy Markdown
Contributor

This passes all the CI and touches quite a lot so let's get it merged

@osbornjd osbornjd merged commit 0e361eb into sPHENIX-Collaboration:master Jun 27, 2026
23 checks passed
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.

2 participants