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: rewrite measurement selector #3083

Merged
merged 21 commits into from
Apr 9, 2024

Conversation

CarloVarni
Copy link
Collaborator

@CarloVarni CarloVarni commented Apr 5, 2024

Changes to improve the measurement selector

/cc @andiwand and @goetzgaycken

@CarloVarni CarloVarni added this to the next milestone Apr 5, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Track Finding labels Apr 5, 2024
@CarloVarni
Copy link
Collaborator Author

First changes from a quick look. Will do more later (that is why this is a draft for now)

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

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

Project coverage is 49.02%. Comparing base (1374baa) to head (f67393d).
Report is 5 commits behind head on main.

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

Files Patch % Lines
Core/src/TrackFinding/MeasurementSelector.cpp 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3083   +/-   ##
=======================================
  Coverage   49.02%   49.02%           
=======================================
  Files         494      494           
  Lines       29058    29059    +1     
  Branches    13797    13797           
=======================================
+ Hits        14246    14247    +1     
  Misses       4929     4929           
  Partials     9883     9883           

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

@CarloVarni
Copy link
Collaborator Author

@andiwand of course @paulgessinger was correct about the map types, so I reverted the changes on passing by reference for those.

I have applied additional changed to the function but I'm afraid this is not coming with an executing time improvement. I still think the new function is cleaner and easier to understand, so I'd like to push it anyway into ACTS

@CarloVarni CarloVarni marked this pull request as ready for review April 9, 2024 12:51
andiwand
andiwand previously approved these changes Apr 9, 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.

Good from my side 👍

I think @goetzgaycken optimized this code recently

Core/include/Acts/TrackFinding/MeasurementSelector.hpp Outdated Show resolved Hide resolved
Core/include/Acts/TrackFinding/MeasurementSelector.ipp Outdated Show resolved Hide resolved
Core/src/TrackFinding/MeasurementSelector.cpp Outdated Show resolved Hide resolved
Carlo Varni and others added 2 commits April 9, 2024 15:56
@kodiakhq kodiakhq bot merged commit 3203649 into acts-project:main Apr 9, 2024
51 checks passed
@github-actions github-actions bot removed the automerge label Apr 9, 2024
@CarloVarni CarloVarni deleted the RewriteMeasurementSelector branch April 10, 2024 07:44
kodiakhq bot pushed a commit that referenced this pull request Apr 10, 2024
The construction of the `MeasurementSelector` is IMO unintuitive right now and the default constructor will build a broken object which cannot select anything. I am trying to improve this here by using the default selections mechanism for the default constructor and adding a constructor which only takes a single set of cuts.

blocked by
- #3083
@andiwand andiwand modified the milestones: next, v34.0.0 Apr 15, 2024
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Apr 19, 2024
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Apr 19, 2024
…3093)

The construction of the `MeasurementSelector` is IMO unintuitive right now and the default constructor will build a broken object which cannot select anything. I am trying to improve this here by using the default selections mechanism for the default constructor and adding a constructor which only takes a single set of cuts.

blocked by
- acts-project#3083
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…3093)

The construction of the `MeasurementSelector` is IMO unintuitive right now and the default constructor will build a broken object which cannot select anything. I am trying to improve this here by using the default selections mechanism for the default constructor and adding a constructor which only takes a single set of cuts.

blocked by
- acts-project#3083
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…3093)

The construction of the `MeasurementSelector` is IMO unintuitive right now and the default constructor will build a broken object which cannot select anything. I am trying to improve this here by using the default selections mechanism for the default constructor and adding a constructor which only takes a single set of cuts.

blocked by
- acts-project#3083
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 Track Finding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants