refactor!: General binned group for seeding #2854
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
So: a BinnedGroup now is a grid + an iteration sequence. Could you maybe elaborate, maybe in the tests next to the assertions, what the iteration order is in each case?
Then I have the general point that you're using unique_ptr in ways where I don't think you actually need to. Can you double check this? Are you trying to optimize the number of copies this way, or is there another reason that I'm missing.
paulgessinger
approved these changes
Jan 18, 2024
acts-project-service
added
the
Breaks Athena build
kodiakhq bot
pushed a commit
that referenced
this pull request
Jan 19, 2024
Follow up from #2854 In this PR I am defining a grid geometry for a cylindrical experiment with (phi, z) bins. This is done in a specific file that is included in the GridSpacePoint file. Other specializations can then be easily added in the same fashion (e.g. a pre-defined grid for telescope geometry). Requires: - #2838 - #2854 - #2835
paulgessinger
added
Breaks Athena build
LaraCalic
pushed a commit
to LaraCalic/acts
that referenced
this pull request
Feb 10, 2024
A general class for defining a `BinnedGroup`. This class stores a `grid` (owns it), two `GridBinFinders` and a navigation pattern, optionally defined by the user. The PR also defines its iterator. This will replace the current `BinnedSPGroup` implementation, which is very specific. Also, creating and filling a grid cannot be generalized imho: variable definitions, selection cuts, number of axes and their ordering. As such, the creation and the filling of the grid are now handled externally, and not inside the `BinnedGroup` class, and it's the user's duty to make sure the filling happens. In my mind, all of this should be handled by the user for any custom way of doing it. One thing we can provide (as we currently do but I'm afraid is more a "nice consequence" then a "design") some pre-defined grids for specific geometries. For instance we can define a `CylindricalSpacePointGrid<external_spacepoint_t>` (as well as `CylindricalBinnedGroup<external_spacepoint_t>`) or a `TelescopeSpacePointGrid<external_spacepoint_t>` (and its `TelescopeBinnedGroup<external_spacepoint_t>`). These last points are not covered here, will be addressed in another PR once this one is in. My idea is to define these "specialization" as simple `typedef`s of the above generic classes, as well as defining functions for their creation and filling. This can be located inside a new `Acts/Seeding/detail` folder, so we can add as many specializations as possible. Requires: - acts-project#2838 - acts-project#2835 - acts-project#2839 (maybe optional?)
LaraCalic
pushed a commit
to LaraCalic/acts
that referenced
this pull request
Feb 10, 2024
…#2858) Follow up from acts-project#2854 In this PR I am defining a grid geometry for a cylindrical experiment with (phi, z) bins. This is done in a specific file that is included in the GridSpacePoint file. Other specializations can then be easily added in the same fashion (e.g. a pre-defined grid for telescope geometry). Requires: - acts-project#2838 - acts-project#2854 - acts-project#2835
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A general class for defining a
BinnedGroup. This class stores agrid(owns it), twoGridBinFindersand a navigation pattern, optionally defined by the user. The PR also defines its iterator.This will replace the current
BinnedSPGroupimplementation, which is very specific.Also, creating and filling a grid cannot be generalized imho: variable definitions, selection cuts, number of axes and their ordering. As such, the creation and the filling of the grid are now handled externally, and not inside the
BinnedGroupclass, and it's the user's duty to make sure the filling happens.In my mind, all of this should be handled by the user for any custom way of doing it. One thing we can provide (as we currently do but I'm afraid is more a "nice consequence" then a "design") some pre-defined grids for specific geometries.
For instance we can define a
CylindricalSpacePointGrid<external_spacepoint_t>(as well asCylindricalBinnedGroup<external_spacepoint_t>) or aTelescopeSpacePointGrid<external_spacepoint_t>(and itsTelescopeBinnedGroup<external_spacepoint_t>).These last points are not covered here, will be addressed in another PR once this one is in. My idea is to define these "specialization" as simple
typedefs of the above generic classes, as well as defining functions for their creation and filling. This can be located inside a newActs/Seeding/detailfolder, so we can add as many specializations as possible.Requires:
skipZMiddleBinSearch#2835