New tpc transforms #4109

adfrawley · 2026-01-14T22:12:23Z

Types of changes

[x ] 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, ...)

Mariia found a bug in the logic for backward compatibility with existing alignment parameter sets. This PR fixes it.

TODOs (if applicable)

Links to other PRs in macros and calibration repositories (if applicable)

Summary

Motivation

This pull request addresses a bug in the backward compatibility logic for TPC alignment transforms, specifically in how module tilt transforms are composed. The fix ensures that existing alignment parameter sets continue to work correctly while enabling support for newer transform conventions.

Key Changes

New state flag: Added use_module_tilt private boolean member to track whether TPC module tilt transforms should be applied during transform composition
TPC surface processing: Initialize use_module_tilt = false for each surface; set it to true after computing local frame translation (for layers 0-3 or when use_module_tilt_always is enabled)
Transform composition refactoring:
- TPC transforms (non-survey): Conditionally apply one of two transform orders:
  - If use_module_tilt == true: Apply mpLocalTranslationAffine * mpLocalRotationAffine (new order)
  - Otherwise: Apply mpLocalRotationAffine * actsRotationAffine (backward-compatible legacy order)
- Silicon transforms: Use existing use_new_silicon_rotation_order flag to select between new and legacy transform composition orders
- Survey mode: No change to transform composition
Code cleanup: Reformatted verbosity debugging block with improved indentation

Potential Risk Areas

Reconstruction behavior changes: Altering the composition order of alignment transforms affects how track reconstruction corrections are applied. The change only takes effect when use_module_tilt = true (limited to specific TPC layers), so the impact may be localized to lower TPC layers. Validation against test data is important to verify correctness of the new transform path.
Backward compatibility: The fix introduces conditional logic to maintain compatibility with existing alignment parameter sets. A critical element is that use_module_tilt defaults to false, preserving legacy behavior for parameter sets that do not set use_module_tilt_always. However, there is a risk of subtle alignment differences if the legacy and new transform orders produce materially different results.
State management: The use_module_tilt flag is reset for each TPC surface in createMap() and then checked in newMakeTransform(). Ensure the flag is always set before it is checked in the transform composition logic.
Test coverage: The fix touches core alignment transform logic used in track reconstruction. Comprehensive validation across different detector regions and alignment parameter sets is recommended.

Possible Future Improvements

Consider a deprecation path or migration timeline for moving all alignment parameter sets to the newer transform order
Document the difference between legacy and new transform orders for future maintainers
Add configuration options to explicitly select between legacy and new modes rather than relying on implicit layer-based logic

Note: AI-generated summaries can contain inaccuracies. Review the actual code changes and consult domain experts for detailed understanding of alignment transform implications.

…esoftware into new_tpc_transforms

coderabbitai · 2026-01-14T22:12:41Z

📝 Walkthrough

Walkthrough

The changes introduce a new use_module_tilt flag to control TPC module tilt handling in transform composition. The code refactors newMakeTransform to apply different transformation paths for survey mode, TPC detectors, and silicon detectors, with conditional logic determining whether to use updated transform orders or preserve backward compatibility.

Changes

Cohort / File(s)	Summary
TPC Module Tilt Control `offline/packages/trackbase/AlignmentTransformation.h`	Added private boolean member `use_module_tilt` (initialized to false) to track TPC module tilt state across transform operations
Transform Composition Refactoring `offline/packages/trackbase/AlignmentTransformation.cc`	Refactored `newMakeTransform` method with conditional pathways: (1) initializes `use_module_tilt` flag and conditionally computes local frame translation based on test layer and `use_module_tilt_always`; (2) applies different transform compositions for survey mode, TPC, and silicon detectors; (3) introduces `use_new_silicon_rotation_order` flag for backward-compatible silicon transform ordering; (4) reworks verbosity debugging block with recomputed transforms and expanded diagnostic output

✨ Finishing touches

📝 Docstrings were successfully generated.

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f402d4 and 2cbb506.

📒 Files selected for processing (2)

offline/packages/trackbase/AlignmentTransformation.cc
offline/packages/trackbase/AlignmentTransformation.h

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{h,hpp,hxx,hh}

⚙️ CodeRabbit configuration file

**/*.{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.

Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.

Files:

offline/packages/trackbase/AlignmentTransformation.h

**/*.{cc,cpp,cxx,c}

⚙️ CodeRabbit configuration file

**/*.{cc,cpp,cxx,c}: Prioritize correctness, memory safety, error handling, and thread-safety.
Flag hidden global state, non-const singletons, and unclear lifetime assumptions.

Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.

Files:

offline/packages/trackbase/AlignmentTransformation.cc

🧠 Learnings (2)

📓 Common learnings

Learnt from: CR
Repo: sPHENIX-Collaboration/coresoftware PR: 0
File: .github/pull_request_template.md:0-0
Timestamp: 2026-01-06T20:45:49.686Z
Learning: Pull request must specify the types of changes being introduced (Bug fix, New feature, Breaking change, or Macros repository change)

Learnt from: CR
Repo: sPHENIX-Collaboration/macros PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-01-08T19:57:36.996Z
Learning: If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required

Learnt from: CR
Repo: sPHENIX-Collaboration/coresoftware PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-01-06T20:45:54.192Z
Learning: If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required

📚 Learning: 2026-01-08T18:39:48.023Z

Learnt from: emclaughlin2
Repo: sPHENIX-Collaboration/coresoftware PR: 4098
File: offline/QA/Calorimeters/CaloValid.cc:270-270
Timestamp: 2026-01-08T18:39:48.023Z
Learning: In C++ code that parses or traverses data nodes, implement a dual-key fallback when looking up a GL1Packet-like node: first try the version-specific key (e.g., "14001"), then fall back to the legacy/general key (e.g., "GL1Packet"). This ensures compatibility with multiple data versions where the node naming may have changed over time. Apply this pattern to files under the offline/ Calorimeters area (and similar modules) to maintain data-structure resilience across versions.

Applied to files:

offline/packages/trackbase/AlignmentTransformation.cc

🔇 Additional comments (4)

offline/packages/trackbase/AlignmentTransformation.h (1)

131-132: LGTM!

The new use_module_tilt flag is properly scoped as private and initialized to false for backward compatibility. This follows the pattern established by the other control flags in this class.

offline/packages/trackbase/AlignmentTransformation.cc (3)

280-295: Correct state management for backward compatibility.

The use_module_tilt flag is explicitly set at the start of each surface iteration (line 280) and conditionally enabled (line 291) only when processing module-level alignment parameters (test_layer < 4) or when forced via use_module_tilt_always. This ensures the correct transform path is selected in newMakeTransform for both legacy layer-level and newer module-level alignment parameter sets.

430-461: Transform composition logic correctly separates new vs backward-compatible paths.

The key difference is the placement of mpLocalRotationAffine:

New path: actsRotationAffine * mpLocalTranslationAffine * mpLocalRotationAffine — local rotation applied first, then translation accounts for module tilt

Backward-compatible path: mpLocalRotationAffine * actsRotationAffine — rotation applied after acts rotation, matching existing alignment parameter conventions

This separation ensures existing alignment calibrations continue to produce correct transforms while enabling the improved module tilt handling for new parameter sets.

463-499: Enhanced diagnostic output for debugging alignment transforms.

The additional verbosity output will be helpful for diagnosing alignment parameter issues — showing the full transform decomposition and element-wise comparison against the ideal transform is valuable when validating new calibration constants.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai · 2026-01-14T22:39:49Z

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #4110

@blackcathj

Docstrings generation was requested by @blackcathj. * #4109 (comment) The following files were modified: * `offline/packages/trackbase/AlignmentTransformation.cc` * `offline/packages/trackbase/AlignmentTransformation.h`

sphenix-jenkins-ci · 2026-01-15T03:52:26Z

Build & test report

Report for commit 2cbb5063f26e7a1157890b4ed29f3a7557d1b168:

builds and tests overall are SUCCESS.
Build with configuration of alma9.2-gcc-14.2.0 / clang is SUCCESS, 📊clang report (full)/(new), build log
Build with configuration of alma9.2-gcc-14.2.0 / new is SUCCESS, 📊Compiler report (full)/(new), build log
- Generating DST and readback: build is SUCCESS
- Calorimeter QA: build is SUCCESS, 📊 trends
  - 📊 QA-calorimeter for e- at p_T=4GeV : combined Chi2/nDoF = 27.45 / 72, and combined p-Value = 1
  - 📊 QA-calorimeter for pi+ at p_T=30GeV : combined Chi2/nDoF = 41.6183 / 72, and combined p-Value = 0.998442
  - 📊 QA-calorimetric-jet for e- at p_T=4GeV : combined Chi2/nDoF = 0.4559 / 42, and combined p-Value = 1
  - 📊 QA-calorimetric-jet for pi+ at p_T=30GeV : combined Chi2/nDoF = 14.2531 / 42, and combined p-Value = 0.999981
- Tracking QA with Distortions: build is SUCCESS, 📊 trends
  - 📊 QA-distortions
- Tracking QA at low occupancy: build is SUCCESS, 📊 trends
  - 📊 QA-Intt : combined Chi2/nDoF = -0 / 72, and combined p-Value = 1
  - 📊 QA-Micromegas : combined Chi2/nDoF = -0 / 20, and combined p-Value = 1
  - 📊 QA-Mvtx : combined Chi2/nDoF = -0 / 54, and combined p-Value = 1
  - 📊 QA-Tpc : combined Chi2/nDoF = 1045.7 / 56, and combined p-Value = 2.05201e-182
  - 📊 QA-tracking : combined Chi2/nDoF = 879.409 / 56, and combined p-Value = 2.48103e-148
  - 📊 QA-vertexing : combined Chi2/nDoF = 1680 / 112, and combined p-Value = 8.98848e-278
- Tracking QA for Pythia D0-jet: build is SUCCESS, 📊 trends
  - 📊 QA-Intt : combined Chi2/nDoF = -0 / 72, and combined p-Value = 1
  - 📊 QA-Micromegas : combined Chi2/nDoF = -0 / 20, and combined p-Value = 1
  - 📊 QA-Mvtx : combined Chi2/nDoF = -0 / 54, and combined p-Value = 1
  - 📊 QA-Tpc : combined Chi2/nDoF = 886.52 / 56, and combined p-Value = 8.80212e-150
  - 📊 QA-tracking : combined Chi2/nDoF = 635.474 / 50, and combined p-Value = 1.99328e-102
  - 📊 QA-vertexing : combined Chi2/nDoF = 1680 / 112, and combined p-Value = 8.98848e-278
- Tracking QA from run2pp 53877: build is SUCCESS, 📊 trends
  - 📊 QA-intt : combined Chi2/nDoF = -0 / 18, and combined p-Value = 1
  - 📊 QA-micromegas : combined Chi2/nDoF = -0 / 6, and combined p-Value = 1
  - 📊 QA-mvtx : combined Chi2/nDoF = -0 / 18, and combined p-Value = 1
  - 📊 QA-siliconseeds : combined Chi2/nDoF = 0.00727412 / 20, and combined p-Value = 1
  - 📊 QA-tpc : combined Chi2/nDoF = -0 / 48, and combined p-Value = 1
  - 📊 QA-tpcseeds : combined Chi2/nDoF = 0.00169328 / 28, and combined p-Value = 1
  - 📊 QA-trackfit : combined Chi2/nDoF = 5.51613 / 32, and combined p-Value = 1
- system alma9.2-gcc-14.2.0, build new: run the default sPHENIX macro: build is SUCCESS, output, trends 📊
- system alma9.2-gcc-14.2.0, build new: run the default CaloProduction/Fun4All_Year2.C macro: build is SUCCESS, output, trends 📊
- system alma9.2-gcc-14.2.0, build new: run the default StreamingProduction/Fun4All_Stream_Combiner.C macro: build is SUCCESS, output, trends 📊
- system alma9.2-gcc-14.2.0, build new: Valgrind test of CaloProduction/Fun4All_Year2.C: build is SUCCESS, 📊valgrind report, trends 📊
- system alma9.2-gcc-14.2.0, build new: Valgrind test of StreamingProduction/Fun4All_Stream_Combiner.C: build is SUCCESS, 📊valgrind report, trends 📊
- system alma9.2-gcc-14.2.0, build new: run the overlap check for sPHENIX macro: build is SUCCESS, output
- system alma9.2-gcc-14.2.0, build new: Valgrind test: build is SUCCESS, 📊valgrind report, trends 📊
Build with configuration of alma9.2-gcc-14.2.0 / scan is SUCCESS, 📊scan-build report (full)/(new), build log
clang-tidy is SUCCESS, 📊clang-tidy report (full)/(new)
cpp-check is SUCCESS, 📊cppcheck report (full)/(new)

Automatically generated by sPHENIX Jenkins continuous integration

osbornjd

This changes the QA really dramatically for the worse. There must be something that is not yet understood

sPHENIX-GitHub-l · 2026-01-15T22:28:41Z

Well, the run2 pp data QA looks fine. The simulations QA looks bad, but I can find no way that this PR could do that. The simulations jobs read a file from the CDB with all alignment parameters set to zero, so effectively they use only the ACTS geometry transformations. I am running simulations to try to understand this. Tony

…

________________________________ From: ***@***.*** ***@***.***> on behalf of "Joseph (Joe) Osborn" ***@***.***> Sent: Thursday, January 15, 2026 9:33 AM To: sPHENIX-Collaboration/coresoftware ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [[Sphenix-github-l] ] [sPHENIX-Collaboration/coresoftware] New tpc transforms (PR #4109) @ osbornjd commented on this pull request. This changes the QA really dramatically for the worse. There must be something that is not yet understood — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this ZjQcmQRYFpfptBannerStart This Message Is From an External Sender This message came from outside your organization. ZjQcmQRYFpfptBannerEnd @osbornjd commented on this pull request. This changes the QA really dramatically for the worse. There must be something that is not yet understood — Reply to this email directly, view it on GitHub<#4109 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEALMH2RVAAHPAUMC572R6L4G6QMTAVCNFSM6AAAAACRXFCM6OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMNRVHEZTANZRGM>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

adfrawley · 2026-01-16T00:36:47Z

Looking at the simulations using the nightly build code and this PR, I see identical results - screwed up in both cases. The attached plot shows at least one problem. It is a plot from SvtxEval of pcaz-gvz vs eta for 250 pion simulation. It looks like either the drift velocity or some time offset is wrong. I note that the Jenkins simulation QA for a different PR just came in, and it is bad also. Tony

…

________________________________ From: sPHENIX Software Repository Email List ***@***.***> Sent: Thursday, January 15, 2026 5:29 PM To: sPHENIX-Collaboration/coresoftware ***@***.***> Cc: Anthony Frawley ***@***.***>; Author ***@***.***> Subject: Re: [sPHENIX-Collaboration/coresoftware] New tpc transforms (PR #4109) [https://avatars.githubusercontent.com/u/16823839?s=20&v=4]sPHENIX-GitHub-l left a comment (sPHENIX-Collaboration/coresoftware#4109)<#4109 (comment)> Well, the run2 pp data QA looks fine. The simulations QA looks bad, but I can find no way that this PR could do that. The simulations jobs read a file from the CDB with all alignment parameters set to zero, so effectively they use only the ACTS geometry transformations. I am running simulations to try to understand this. Tony

________________________________ From: ***@***.*** ***@***.***> on behalf of "Joseph (Joe) Osborn" ***@***.***> Sent: Thursday, January 15, 2026 9:33 AM To: sPHENIX-Collaboration/coresoftware ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [[Sphenix-github-l] ] [sPHENIX-Collaboration/coresoftware] New tpc transforms (PR #4109) @ osbornjd commented on this pull request. This changes the QA really dramatically for the worse. There must be something that is not yet understood — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this ZjQcmQRYFpfptBannerStart This Message Is From an External Sender This message came from outside your organization. ZjQcmQRYFpfptBannerEnd @osbornjd commented on this pull request. This changes the QA really dramatically for the worse. There must be something that is not yet understood — Reply to this email directly, view it on GitHub<#4109 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEALMH2RVAAHPAUMC572R6L4G6QMTAVCNFSM6AAAAACRXFCM6OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMNRVHEZTANZRGM>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***> — Reply to this email directly, view it on GitHub<#4109 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACHADBMRGM7YRCQKXMYG2VT4HAIC7AVCNFSM6AAAAACRXFCM6OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTONJXGE4TSNJSGU>. You are receiving this because you authored the thread.Message ID: ***@***.***>

osbornjd · 2026-01-16T01:23:55Z

You are right, this is the last jenkins PR that has p values of unity in the QA
#4101

No idea what would have happened unless something changed in the cdb or macros. I will investigate further

adfrawley added 2 commits January 14, 2026 15:04

Fixed bug in logic for using existing parameter sets

e3fffce

Merge branch 'master' of https://github.com/sPHENIX-Collaboration/cor…

2cbb506

…esoftware into new_tpc_transforms

coderabbitai bot mentioned this pull request Jan 14, 2026

📝 Add docstrings to new_tpc_transforms #4110

Closed

osbornjd reviewed Jan 15, 2026

View reviewed changes

osbornjd merged commit 7b60428 into sPHENIX-Collaboration:master Jan 16, 2026
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

New tpc transforms #4109

New tpc transforms #4109

Uh oh!

adfrawley commented Jan 14, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 14, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai bot commented Jan 14, 2026 •

edited

Loading

Uh oh!

sphenix-jenkins-ci bot commented Jan 15, 2026

Uh oh!

osbornjd left a comment

Uh oh!

sPHENIX-GitHub-l commented Jan 15, 2026 via email

Uh oh!

adfrawley commented Jan 16, 2026 via email

Uh oh!

osbornjd commented Jan 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

New tpc transforms #4109

New tpc transforms #4109

Uh oh!

Conversation

adfrawley commented Jan 14, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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)

Summary

Motivation

Key Changes

Potential Risk Areas

Possible Future Improvements

Uh oh!

coderabbitai bot commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai bot commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sphenix-jenkins-ci bot commented Jan 15, 2026

Build & test report

Uh oh!

osbornjd left a comment

Choose a reason for hiding this comment

Uh oh!

sPHENIX-GitHub-l commented Jan 15, 2026 via email

Uh oh!

adfrawley commented Jan 16, 2026 via email

Uh oh!

osbornjd commented Jan 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adfrawley commented Jan 14, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 14, 2026 •

edited

Loading

coderabbitai bot commented Jan 14, 2026 •

edited

Loading