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: Change the MultiLayerSurfacesUpdater to use the local position and direction #3041

Merged
merged 7 commits into from
Apr 10, 2024

Conversation

dimitra97
Copy link
Contributor

This PR introduces some changes in the MultiLayerSurfacesUpdater in order to consider the local position and direction from the navigation state and generates the path in the grid based on them. Also, since this Navigation Delegate is used in the MultiWireStructureBuilder I pass the transform of the multiLayer in the delegate, in order to be taken into account for the global->local transformations.

(I deleted a previous branch that I have created for this and created a new one, because I had messed up with some files from main)

@github-actions github-actions bot added the Component - Core Affects the Core module label Mar 19, 2024
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

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

Project coverage is 48.83%. Comparing base (3203649) to head (c73d299).
Report is 4 commits behind head on main.

❗ Current head c73d299 differs from pull request most recent head b40eb44. Consider uploading reports for the commit b40eb44 to get more accurate results

Files Patch % Lines
...lude/Acts/Navigation/MultiLayerSurfacesUpdater.hpp 20.00% 0 Missing and 8 partials ⚠️
Core/src/Detector/MultiWireStructureBuilder.cpp 33.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3041      +/-   ##
==========================================
- Coverage   49.09%   48.83%   -0.27%     
==========================================
  Files         497      491       -6     
  Lines       29155    28886     -269     
  Branches    13851    13709     -142     
==========================================
- Hits        14314    14106     -208     
- Misses       4908     4954      +46     
+ Partials     9933     9826     -107     

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

Copy link
Member

@LuisFelipeCoelho LuisFelipeCoelho 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 based on the really small knowledge that I have on this

@AJPfleger AJPfleger added this to the next milestone Apr 9, 2024
andiwand
andiwand previously approved these changes Apr 9, 2024
@andiwand
Copy link
Contributor

andiwand commented Apr 9, 2024

@dimitra97 there is one clang tidy failure

╭─ /builds/acts/ci-bridge/src/Core/include/Acts/Navigation/MultiLayerSurfacesU─╮
│ 🟡                                                                           │
│ /builds/acts/ci-bridge/src/Core/include/Acts/Navigation/MultiLayerSurfacesUp │
│ dater.hpp:128:24 WARNING [performance-unnecessary-value-param]               │
│ ╭──────────────────────────────────────────────────────────────────────────╮ │
│ │ parameter 'startPosition' is passed by value and only copied once;       │ │
│ │ consider moving it to avoid unnecessary copies                           │ │
│ │    19 |     Vector3 position = startPosition;                            │ │
│ │       |                        ^                                         │ │
│ │       |                        std::move(   )                            │ │
│ │                                                                          │ │
│ ╰──────────────────────────────────────────────────────────────────────────╯ │
│ ──────────────────────────────────────────────────────────────────────────── │
╰──────────────────────────────────────────────────────────────────────────────╯

@andiwand andiwand removed the automerge label Apr 9, 2024
@kodiakhq kodiakhq bot merged commit e03bcd6 into acts-project:main Apr 10, 2024
51 checks passed
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Apr 11, 2024
@andiwand andiwand removed the Fails Athena tests This PR causes a failure in the Athena tests label Apr 11, 2024
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Apr 19, 2024
…ion and direction (acts-project#3041)

This PR introduces some changes in the `MultiLayerSurfacesUpdater` in order to consider the local position and direction from the navigation state and generates the path in the grid based on them. Also, since this Navigation Delegate is used in the `MultiWireStructureBuilder` I pass the transform of the multiLayer in the delegate, in order to be taken into account for the global->local transformations. 

(I deleted a previous branch that I have created for this and created a new one, because I had messed up with some files from main)
@andiwand andiwand modified the milestones: next, v34.1.0 Apr 25, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…ion and direction (acts-project#3041)

This PR introduces some changes in the `MultiLayerSurfacesUpdater` in order to consider the local position and direction from the navigation state and generates the path in the grid based on them. Also, since this Navigation Delegate is used in the `MultiWireStructureBuilder` I pass the transform of the multiLayer in the delegate, in order to be taken into account for the global->local transformations. 

(I deleted a previous branch that I have created for this and created a new one, because I had messed up with some files from main)
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…ion and direction (acts-project#3041)

This PR introduces some changes in the `MultiLayerSurfacesUpdater` in order to consider the local position and direction from the navigation state and generates the path in the grid based on them. Also, since this Navigation Delegate is used in the `MultiWireStructureBuilder` I pass the transform of the multiLayer in the delegate, in order to be taken into account for the global->local transformations. 

(I deleted a previous branch that I have created for this and created a new one, because I had messed up with some files from main)
@AJPfleger AJPfleger linked an issue May 22, 2024 that may be closed by this pull request
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: MultiWire SurfaceCandidateUpdator
5 participants