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!: Generic grid bin finder #2838

Merged
merged 96 commits into from
Jan 16, 2024
Merged

Conversation

CarloVarni
Copy link
Collaborator

@CarloVarni CarloVarni commented Dec 16, 2023

This will complete the adaptation of the grid to an N-dimentional case. The user is now able to iterate on any axis as they like without the need to rotate their geometry.

Next step is to see how the seed finding routine can be adapted to be more general

This PR is also a bug-fix since in the past we had:

template <typename external_spacepoint_t>
boost::container::small_vector<std::size_t, 9>
Acts::BinFinder<external_spacepoint_t>::findBins(
    std::size_t phiBin, std::size_t zBin,
    const Acts::SpacePointGrid<external_spacepoint_t>* binnedSP) const {
  // if zBinNeighbors is not defined, get the indices using
  // neighborHoodIndices
  if (m_zBinNeighbors->empty()) {
    return binnedSP->neighborHoodIndices({phiBin, zBin}).collect();
  }
  // if the zBinNeighbors is defined, get the indices from there
  std::array<std::pair<int, int>, 2> sizePerAxis;
  sizePerAxis.at(0) = std::make_pair(-m_numPhiNeighbors, m_numPhiNeighbors);
  sizePerAxis.at(1) = (*m_zBinNeighbors)[zBin - 1];
  return binnedSP->neighborHoodIndices({phiBin, zBin}, sizePerAxis).collect();
}

In the case m_zBinNeighbors is empty, the code is instructed to search neighbours in the range {-1, 1} FOR EVERY AXIS (this is what binnedSP->neighborHoodIndices({phiBin, zBin}).collect(); does), thus overwriting the value set by the user for m_numPhiNeighbors.

Now, the axes are treated independently

This requires: #2835

@CarloVarni CarloVarni marked this pull request as draft January 9, 2024 20:07
@CarloVarni CarloVarni modified the milestones: next, v32.0.0 Jan 9, 2024
niermann999
niermann999 previously approved these changes Jan 9, 2024
@CarloVarni CarloVarni added the 🛑 blocked This item is blocked by another item label Jan 10, 2024
@acts-policybot acts-policybot bot dismissed niermann999’s stale review January 10, 2024 21:35

Invalidated by push of 9e408e0

niermann999
niermann999 previously approved these changes Jan 10, 2024
@acts-policybot acts-policybot bot dismissed niermann999’s stale review January 15, 2024 09:48

Invalidated by push of cbe587c

@CarloVarni CarloVarni marked this pull request as ready for review January 15, 2024 09:49
@CarloVarni CarloVarni removed the 🛑 blocked This item is blocked by another item label Jan 15, 2024
@paulgessinger
Copy link
Member

@niermann999 @CarloVarni That's one of the reasons I want to push getting C++20 going as soon as possible so we can use the standard library for this kind of stuff.

@kodiakhq kodiakhq bot merged commit 8bb1bae into acts-project:main Jan 16, 2024
51 checks passed
@CarloVarni CarloVarni deleted the GenericBinFinder branch January 16, 2024 19:53
@acts-project-service
Copy link
Collaborator

🔴 Athena integration test results [8bb1bae]

Build job with this PR failed!

Please investigate the build job for the pipeline!

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Jan 16, 2024
kodiakhq bot pushed a commit that referenced this pull request Jan 18, 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:
- #2838
- #2835
- #2839 (maybe optional?)
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 paulgessinger added Breaks Athena build This PR breaks the Athena build and removed Breaks Athena build This PR breaks the Athena build labels Jan 23, 2024
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
This will complete the adaptation of the grid to an N-dimentional case. The user is now able to iterate on any axis as they like without the need to rotate their geometry. 

Next step is to see how the seed finding routine can be adapted to be more general

This PR is also a bug-fix since in the past we had:
```cpp
template <typename external_spacepoint_t>
boost::container::small_vector<std::size_t, 9>
Acts::BinFinder<external_spacepoint_t>::findBins(
    std::size_t phiBin, std::size_t zBin,
    const Acts::SpacePointGrid<external_spacepoint_t>* binnedSP) const {
  // if zBinNeighbors is not defined, get the indices using
  // neighborHoodIndices
  if (m_zBinNeighbors->empty()) {
    return binnedSP->neighborHoodIndices({phiBin, zBin}).collect();
  }
  // if the zBinNeighbors is defined, get the indices from there
  std::array<std::pair<int, int>, 2> sizePerAxis;
  sizePerAxis.at(0) = std::make_pair(-m_numPhiNeighbors, m_numPhiNeighbors);
  sizePerAxis.at(1) = (*m_zBinNeighbors)[zBin - 1];
  return binnedSP->neighborHoodIndices({phiBin, zBin}, sizePerAxis).collect();
}
```

In the case `m_zBinNeighbors` is empty, the code is instructed to search neighbours in the range `{-1, 1}` FOR EVERY AXIS (this is what `binnedSP->neighborHoodIndices({phiBin, zBin}).collect();` does), thus overwriting the value set by the user for `m_numPhiNeighbors`. 

Now, the axes are treated independently

This requires: acts-project#2835
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
Labels
Breaks Athena build This PR breaks the Athena build Bug Something isn't working Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Seeding Track Finding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants