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

feat: Add possibility to rotate the trapezoid bounds #2583

Merged
merged 15 commits into from
Mar 12, 2024

Conversation

junggjo9
Copy link
Contributor

@junggjo9 junggjo9 commented Oct 26, 2023

Adds the possibility to rotate trapezoid bounds along the center. This is modeled through an additional bounds value eRotationAngle which is defaulted to 0 obtaining the original, unrotated trapezoid.

Summary

  • inside rotates the incoming surface position into the trapezoid frame which leaves the rest of the code unchanged
  • the vertex vector is rotated into surface coordinates before returning
  • the bounding box is adopted in case of a non-zero rotation angle using the ConvexPolygonBounds
  • all implementations are moved to source

@github-actions github-actions bot added the Component - Core Affects the Core module label Oct 26, 2023
@paulgessinger paulgessinger changed the title Feat: Add possibility to rotate the trapezoid bounds feat: Add possibility to rotate the trapezoid bounds Oct 26, 2023
@paulgessinger paulgessinger added this to the next milestone Oct 26, 2023
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: Patch coverage is 63.26531% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 48.87%. Comparing base (28c3133) to head (f8f2ca9).

Files Patch % Lines
Core/src/Surfaces/TrapezoidBounds.cpp 63.26% 1 Missing and 17 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2583      +/-   ##
==========================================
+ Coverage   48.86%   48.87%   +0.01%     
==========================================
  Files         493      493              
  Lines       29058    29074      +16     
  Branches    13798    13805       +7     
==========================================
+ Hits        14200    14211      +11     
  Misses       4962     4962              
- Partials     9896     9901       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulgessinger
Copy link
Member

There are some compilation failures here.

@github-actions github-actions bot added Stale and removed Stale labels Dec 9, 2023
@github-actions github-actions bot added Stale and removed Stale labels Jan 8, 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.

Can you add a PR description @junggjo9 ?

Also I wondered if the rotation of the bounds could also be absorbed in the rotation of the surface / detector element? Maybe I am missing something.

Core/include/Acts/Surfaces/TrapezoidBounds.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Surfaces/TrapezoidBounds.hpp Outdated Show resolved Hide resolved
@paulgessinger
Copy link
Member

If I'm thinking about this the right way, the bounding box is in surface coordinates, not in bound coordinates, so would have to be rotated as well to capture the underlying trapezoid shape.

@andiwand
Copy link
Contributor

I thought in ACTS surface coordinates == bound coordinates?

rotate vertices
get bounding box from convex polygon
remove rotation matrix
move constructor to cpp
paulgessinger
paulgessinger previously approved these changes Mar 12, 2024
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Looks good to me. Clever I idea reusing the ConvexPolygon bounds since it's not perf critical in the constructor.

Core/include/Acts/Surfaces/TrapezoidBounds.hpp Outdated Show resolved Hide resolved
move more functions to source
rotate bounds helper function
@kodiakhq kodiakhq bot merged commit a606f8a into acts-project:main Mar 12, 2024
54 checks passed
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Mar 12, 2024
dimitra97 pushed a commit to dimitra97/acts that referenced this pull request Mar 19, 2024
Adds the possibility to rotate trapezoid bounds along the center. This is modeled through an additional bounds value `eRotationAngle` which is defaulted to `0` obtaining the original, unrotated trapezoid.

Summary
- `inside` rotates the incoming surface position into the trapezoid frame which leaves the rest of the code unchanged
- the vertex vector is rotated into surface coordinates before returning
- the bounding box is adopted in case of a non-zero rotation angle using the `ConvexPolygonBounds`
- all implementations are moved to source

Co-authored-by: Andreas Stefl <487211+andiwand@users.noreply.github.com>
@paulgessinger paulgessinger modified the milestones: next, v33.1.0 Mar 26, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
Adds the possibility to rotate trapezoid bounds along the center. This is modeled through an additional bounds value `eRotationAngle` which is defaulted to `0` obtaining the original, unrotated trapezoid.

Summary
- `inside` rotates the incoming surface position into the trapezoid frame which leaves the rest of the code unchanged
- the vertex vector is rotated into surface coordinates before returning
- the bounding box is adopted in case of a non-zero rotation angle using the `ConvexPolygonBounds`
- all implementations are moved to source

Co-authored-by: Andreas Stefl <487211+andiwand@users.noreply.github.com>
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
Adds the possibility to rotate trapezoid bounds along the center. This is modeled through an additional bounds value `eRotationAngle` which is defaulted to `0` obtaining the original, unrotated trapezoid.

Summary
- `inside` rotates the incoming surface position into the trapezoid frame which leaves the rest of the code unchanged
- the vertex vector is rotated into surface coordinates before returning
- the bounding box is adopted in case of a non-zero rotation angle using the `ConvexPolygonBounds`
- all implementations are moved to source

Co-authored-by: Andreas Stefl <487211+andiwand@users.noreply.github.com>
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.

5 participants