Skip to content

refactor!: Move Transform2/Transform3 from Affine to Isometry - #6040

Draft
andiwand wants to merge 2 commits into
acts-project:mainfrom
andiwand:feat-isometry-transform3
Draft

refactor!: Move Transform2/Transform3 from Affine to Isometry#6040
andiwand wants to merge 2 commits into
acts-project:mainfrom
andiwand:feat-isometry-transform3

Conversation

@andiwand

@andiwand andiwand commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

The geometry, navigation and propagation code has always assumed the
linear part of a transform is orthogonal: Surface::freeToBoundJacobian
uses referenceFrame().transpose() as its inverse, and referenceFrame
is typed RotationMatrix3. A scaled or sheared transform therefore
produces a silently wrong Jacobian rather than an error. Make the
assumption part of the type, so that is a compile error instead.

AffineTransform2/AffineTransform3 name the unconstrained form for the
external geometry that legitimately needs it, and
makeTransform3(rotation, translation) replaces the
Translation3 * RotationMatrix3 idiom, which Eigen types as affine.

Picks up #4055 by pbutti.

Co-Authored-By: Pierfrancesco Butti pierfrancesco.butti@gmail.com
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

The geometry, navigation and propagation code has always assumed the
linear part of a transform is orthogonal: `Surface::freeToBoundJacobian`
uses `referenceFrame().transpose()` as its inverse, and `referenceFrame`
is typed `RotationMatrix3`. A scaled or sheared transform therefore
produces a silently wrong Jacobian rather than an error. Make the
assumption part of the type, so that is a compile error instead.

`AffineTransform2`/`AffineTransform3` name the unconstrained form for the
external geometry that legitimately needs it, and
`makeTransform3(rotation, translation)` replaces the
`Translation3 * RotationMatrix3` idiom, which Eigen types as affine.

Picks up acts-project#4055 by @pbutti.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01DGXsyXAdFts7z7YYqCar43

Co-Authored-By: Pierfrancesco Butti <pierfrancesco.butti@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DGXsyXAdFts7z7YYqCar43
@github-actions github-actions Bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Seeding labels Sep 7, 2026
@github-actions github-actions Bot added this to the next milestone Sep 7, 2026
@andiwand

andiwand commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Why

Surface::freeToBoundJacobian inverts the reference frame with .transpose(), DiscSurface::normal reads matrix column 2 without normalising it, and referenceFrame() is typed RotationMatrix3. Orthogonality is already a hard requirement — it just was not expressed anywhere, so violating it gives a wrong Jacobian instead of an error.

Exactly one place in the repo ever violated it: the beveled endcap disc in CylinderVolumeBounds::orientedSurfaces. #4055 disabled that code; here it is diagnosed instead. Its factor is 2 - 1/cos(bevel)cos(bevel) where an elliptical cut needs 1/cos(bevel), and it goes singular at 60°. Nothing outside Core constructs bevels, so the disc is now only tilted.

What changed vs #4055

  • No runtime throws (that was the open question there). One debug assert inside makeTransform3, and the rest is compile-time.
  • Translation3 * RotationMatrix3 is affine in Eigen, so those sites go through makeTransform3(rotation, translation) rather than wrapping in Eigen::Isometry3d(...).
  • Transform2 too, plus AffineTransform2/AffineTransform3 for external geometry. Widening Transform3AffineTransform3 is implicit, narrowing is a static_assert, so every entry into ACTS geometry is an explicit decision.

Not a perf PR. The isolated inverse() * position is 2.8x faster as a transpose, but #4055 measured no end-to-end gain on ODD 1k events and I have no reason to expect otherwise. The case is correctness.

Review guide

25 files, but only the first is a design decision:

  1. Definitions/Algebra.hpp — the four aliases and makeTransform3. Everything else follows mechanically.
  2. CylinderVolumeBounds.cpp — the only behaviour change.
  3. Plugins/GeoModel/.../GeoTransformConverter.hppGeoTrf::Transform3D is Eigen::Affine3d, so this is where external affine meets ACTS rigid.
  4. The remaining 22 are call-site rewrites of one idiom.

What breaks for users

Four compile-error classes, all loud, ~23 sites in this repo: building from Translation3 * RotationMatrix3; passing an Eigen::Affine3d; .scale()/Eigen::Scaling; and mutating rotation(), which is now a const block rather than a matrix by value. One hole the type cannot close: linear() = <non-orthogonal> still compiles and then inverse() is wrong.

Notes for maintainers

  • Results shift at rounding level (~1e-11 mm over a 3 m lever arm), so root hashes and physmon references need regenerating — same single-bin GSF churn as refactor!: Move Affine to Isometry for Acts::Transform3 #4055.
  • The API breaking label will not fire automatically. Everything in Algebra.hpp sits inside a Doxygen @defgroup, and CI/public_api/public_api_surface.py skips compounds whose name does not start with Acts — a group compound is named algebra_types. Running the diff locally reports "No change to the public API surface ✅", which is an artifact. ChargeHypothesis.hpp, ParticleHypothesis.hpp, EstimateTrackParamsFromSeed.hpp and Logger.hpp have the same blind spot. Happy to fix the scanner separately.

Verified: Core + DD4hep/Geant4/GeoModel/Json/Root/ActSVG/EDM4hep + Examples + python bindings build clean, 375/375 ctest with assertions forced on.

@andiwand andiwand added the Breaking change This change breaks backwards compatibility label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Public API surface diff

No change to the public API surface. ✅

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

📊: Physics performance monitoring for 396a6b8

Full contents

physmon summary

@andiwand
andiwand marked this pull request as draft September 7, 2026 14:16
Add Acts::isOrthogonal next to makeTransform3 so the assert and the two
deserialization boundaries share one rule, and make GeoModel placement
conversion and Transform3 from_json throw rather than silently truncate
a scaled or sheared matrix.

That check found two transforms built from uninitialized Eigen storage
via prerotate/pretranslate, in the Python Transform3 binding and in
GridJsonConverterTests, both of which produced a garbage linear part.

Route the five remaining hand-built transforms through makeTransform3,
add a beveled-disc test for CylinderVolumeBounds::orientedSurfaces, and
drop the unused AffineTransform2.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T3yeUHxMX9pyWarxGRCWVc
@sonarqubecloud

sonarqubecloud Bot commented Sep 7, 2026

Copy link
Copy Markdown

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

Labels

Breaking change This change breaks backwards compatibility Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Component - Plugins Affects one or more Plugins Seeding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant