test: assert all placement rotations preserve handedness#72
Conversation
Recursively walk the assembled world tree and check that the linear part of every getXToChildVol transform has positive determinant, so a reflection (left-handed frame) fails the build. This pins the fix from #70 permanently and covers any future builder, not just the SBT structure.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a recursive geometry consistency check that asserts every nested placement transform has a strictly positive linear-part determinant, and runs it from a non-null world volume in a new Catch2 test. ChangesGeometry consistency validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
Note: This currently takes several minutes, because the scattering detector is implemented with every single fiber. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_consistency.cpp (1)
71-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the invariant description with what is tested.
det > 0rejects reflections, but it does not prove the transform is a pure rotation; positive-scale or shear transforms also pass. Either describe this as an orientation-preserving check or add orthonormality/unit-scale assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_consistency.cpp` around lines 71 - 74, Update the invariant comment for the recursive placement-transform check to describe it as verifying positive determinant/orientation preservation, not as proving a pure rotation. Keep the existing determinant-based test unchanged unless adding the requested orthonormality and unit-scale assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_consistency.cpp`:
- Around line 75-82: Update checkRightHanded to carry the placement path through
recursive calls, including each child index and logical-volume name. Build the
path from the root to the current child and include it in the INFO diagnostic
alongside the determinant, while preserving the existing handedness check and
recursion.
- Around line 88-94: Update ConsistencyTest.AllRotationsRightHanded so its
several-minute full-geometry traversal is excluded from default Catch2/CI runs.
Add the project’s dedicated slow/integration classification, such as a slow tag
or separate target, and ensure default test configuration excludes it while
retaining the test for explicit execution.
---
Nitpick comments:
In `@tests/test_consistency.cpp`:
- Around line 71-74: Update the invariant comment for the recursive
placement-transform check to describe it as verifying positive
determinant/orientation preservation, not as proving a pure rotation. Keep the
existing determinant-based test unchanged unless adding the requested
orthonormality and unit-scale assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0be9e98-0070-4869-9d0e-bcbd76bfeeec
📒 Files selected for processing (1)
tests/test_consistency.cpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Motivation
Follow-up to #70. That PR fixed a left-handed placement frame in
SBTStructureBuilder(columns(b, n, d)withb = d × n, givingdet < 0), which GeoModel2G4 forwarded toCLHEP::HepRotationas an improper rotation — 180ZMpvImproperRotationwarnings per Geant4 job. Nothing in CI prevents that class of bug from returning, and the frame construction is duplicated across two sites inSBTStructureBuilder.cpp, so any future builder could reintroduce it.Change
Add
ConsistencyTest.AllRotationsRightHanded, which builds the full geometry viaSHiPGeometryBuilderand recursively walks the entire world tree, asserting that the linear part of everygetXToChildVoltransform has positive determinant. A reflection anywhere in the tree fails the test, naming the offending volume path. This covers all subsystems, not just the SBT structure, and pins the 180 → 0 property permanently.Verification
/SHiP/decay_volume/sbt/structure_SF9_Bot_C1_BFat determinant −1) — matching the count fix: make SBT beam placement frames right-handed #70 removed.main(with the fix), the full suite passes 40/40.The walk visits all ~930k nodes and takes ~190 s, dominated by
getXToChildVolmaterializingGeoSerialTransformerreplicas. Global scope is intentional, to guard every current and future builder.Summary by CodeRabbit