Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(geometry): Surface merging returns ordering #3468

Merged

Conversation

paulgessinger
Copy link
Member

This PR updates the surface merging for Cylinders and Discs:

  1. Surface merging returns their relative "ordering", e.g. for two cylinders a and b, the merging will return whether a was at lower $z$ or $r\phi$ values than b. The same applies for discs. For merged surfaces in $\phi$ or $r\phi$ direction which cover the full azimuth after merging, the ordering is preserving: a + b will return a, b, and b + a will return b, a accordingly.
  2. Cylinders and discs support external rotation: with this option set, the merged surface will always have an average $\phi = 0$, meaning that no additional rotation is applied on top of the rotation by the transform. This is needed for well-defined portal merging
  3. Remove the geometry context argument from merging. Surface merging is strictly only supported for surfaces without detector elements, which is checked by the methods. After this is established, it's safe to access the nominal transform directly, without using the geometry context mechanism (it won't be used anyway without detector elements).
  4. Improved coverage of test cases, including a number of edge cases, and associated bug fixes.

paulgessinger and others added 7 commits August 1, 2024 14:55
Basically, this makes it so that the resulting merged CylinderSurface
always has average phi == 0, i.e. symmetric phi sector around the local
x axis, without any extra phi shift.

Equivalent update of DiscSurface still pending
This is not actually needed, because we require the surfaces to not have
detector elements anyway, and in that case, the transform is local to
the surface anyway (and not alignable)
@paulgessinger paulgessinger added this to the next milestone Aug 1, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Aug 1, 2024
andiwand
andiwand previously approved these changes Aug 6, 2024
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had a quick look - no objections from my side 👍

Core/include/Acts/Surfaces/CylinderSurface.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Surfaces/Surface.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Surfaces/DiscSurface.hpp Outdated Show resolved Hide resolved
Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Aug 8, 2024

@kodiakhq kodiakhq bot merged commit cc3f620 into acts-project:main Aug 8, 2024
46 checks passed
@github-actions github-actions bot removed the automerge label Aug 8, 2024
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Aug 8, 2024
@paulgessinger paulgessinger modified the milestones: next, v36.1.0 Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Fails Athena tests This PR causes a failure in the Athena tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants