refactor!: Seeding runs on const inputs

[CarloVarni]

Changes to the code so that the seeding can run now on a collection of const space points.

This implies that some additional variables of the space points (that the seeding algorithm set and uses) are now stored in a specific vector of a struct containing these variables. For now these variables are quality and deltaR

The (internal) space points have now an internal index that acts as an identifier. This index will point the code to the correct element of the above-mentioned vector. Once we move to the SpacePointProxy strategy, this index will already be available

[paulgessinger]

So this is a vector in the side that you index into? Honestly I don't think that's a bad idea at all to separate mutable from immutable information. Would it be possible to Not store the index in the spaceport mint itself somehow?

Also how does this interplay with the split between internal and external spacepoints and you EDM work?

[CarloVarni]

Concerning the EDM my reasoning is the following: Since the SpacePointProxy will have an internal index (that will represent the space point position in the external space-point container), then we'll have automatically a way to navigate and retrieve the mutable variables.
In this PR that is done by introducing in the internal space point class the same concept (the index).
In the SpacePointProxy case the index comes for free, for the InternalSpacePoint we have to add it.

now... is there a way to avoid the index completely? I do not see an easy solution tbh. And I would argue that the index would be the best approach since we foresee to have it in the future anyway.
Also, the advantage of using the index internally is that we can simply retrieve it anywhere in the code by just having the space point.

[CarloVarni]

Just to be clear (and adding @noemina @tboldagh and @LuisFelipeCoelho to the discussion) this PR serves two purposes:

1. Run on a collection of const space-points, which is needed if we want to move to SpacePointProxy instead of keep using the InternalSpacePoint
2. Eventually (not in this PR) extends this concept to the dynamic variables we have for the strip space points (somehow... I still have to figure this out). This is to reduce the SSS timing since in xyzCoordinateCheck we keep creating several Acts::Vector3 objects on the same space points (and the xyzCoordinateCheck takes 86% of the seeding time for strips btw). See https://github.com/acts-project/acts/blob/main/Core/include/Acts/Seeding/SeedFinderUtils.ipp#L155-163

template <typename external_spacepoint_t>
inline bool xyzCoordinateCheck(
    const Acts::SeedFinderConfig<external_spacepoint_t>& m_config,
    const Acts::InternalSpacePoint<external_spacepoint_t>& sp,
    const double* spacepointPosition, double* outputCoordinates) {
  // check the compatibility of SPs coordinates in xyz assuming the
  // Bottom-Middle direction with the strip measurement details
  const float topHalfStripLength = m_config.getTopHalfStripLength(sp.sp());
  const float bottomHalfStripLength =
      m_config.getBottomHalfStripLength(sp.sp());
  const Acts::Vector3 topStripDirection =
      m_config.getTopStripDirection(sp.sp());
  const Acts::Vector3 bottomStripDirection =
      m_config.getBottomStripDirection(sp.sp());
  const Acts::Vector3 stripCenterDistance =
      m_config.getStripCenterDistance(sp.sp());

[...]

And no... these getters return the objects by value, so we cannot go for a Acts::Vector3&

[codecov]

Codecov Report

Merging #1948 (17ab95b) into main (44eff4f) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1948      +/-   ##
==========================================
- Coverage   49.59%   49.56%   -0.04%     
==========================================
  Files         410      411       +1     
  Lines       23302    23319      +17     
  Branches    10634    10636       +2     
==========================================
  Hits        11557    11557              
- Misses       4295     4312      +17     
  Partials     7450     7450              

Impacted Files	Coverage Δ
Core/include/Acts/EventData/SpacePointData.hpp	0.00% <0.00%> (ø)
Core/include/Acts/Seeding/BinnedSPGroup.ipp	0.00% <0.00%> (ø)
Core/include/Acts/Seeding/IExperimentCuts.hpp	0.00% <ø> (ø)
Core/include/Acts/Seeding/InternalSpacePoint.hpp	0.00% <0.00%> (ø)
Core/include/Acts/Seeding/Neighbour.hpp	0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SeedFilter.hpp	0.00% <ø> (ø)
Core/include/Acts/Seeding/SeedFilter.ipp	0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SeedFinder.hpp	0.00% <ø> (ø)
Core/include/Acts/Seeding/SeedFinder.ipp	0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SeedFinderUtils.ipp	0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[github-actions]

📊 Physics performance monitoring for 17ab95b

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu

IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

[CarloVarni]

@paulgessinger I only need to fix the documentation. Are you OK with the design of Acts::SpacePointData? I suppose that eventually this class may be an internal attribute of the Acts::SpacePointContainer so that it can automatically be called by the space point proxy, but for the time being I'm simply passing it as an input to the different methods

[tboldagh]

So I made some comments on the code level but wonder more about the structure of the change. Technically what is needed here is an option to decorate const SP data.
The InternalSpacePoint does that already, without extra indirection through indices. It is not clear to me what SPData is for then?

[CarloVarni]

So I made some comments on the code level but wonder more about the structure of the change. Technically what is needed here is an option to decorate const SP data. The InternalSpacePoint does that already, without extra indirection through indices. It is not clear to me what SPData is for then?

My point is that we want to move away from the InternalSpacePoint and switch to use SpacePointProxy, thus directly using the external space points provided by the user. That means that we'll end up dealing with a collection of const external space points, without the possibility of decorating them with additional variables (since we'll remove the InternalSpacePoint class completely).
The SpacePointData class will act similarly to the internal space point in the sense that will allow to add variables to space point. These variables will be collected in this class, and the index will let us navigate from the space point to the variable

[tboldagh]

Hi Carlo,
My point is following. What we want is kind of decoration of the SP to avoid costly recalculation of th data. Right?
It can be done in two ways:

class DecoratedSP {
  SP& sp;
  + SP methods forwarding
  + extra quantities that would be calculated only once 
};

and then everywhere in seeding code we would use:
std::vector. Something like InternalSP.

or we go with two vectors, one with UPs and another with decorating quantities (SPData in this PR).

I would argue the the first approach is cleaner, i.e. indices that are always easy to mix.

What do you think?

[tboldagh]

So we discussed this live and Carlo clarified that this PR is only one of the steps. In the next steps the SpacePointData and backend collection will be all proxied by the SpacePointProxy and the indexing will not be exposed anywhere beyond the EDM (will be an internal thing). This addresses my concerns.

[CarloVarni]

@tboldagh and I have discussed this PR and the final design I'm envisioning for the ACTS Space Point class. I'm attaching here a scheme of the final design and will open an Issue to collect all the PRs required to reach this design

[tboldagh]

Small additional suggestion (realy small).
After clarifications the PR makes perfect sense.

[CarloVarni]

@tboldagh @LuisFelipeCoelho @paulgessinger if we are ok with this then can we add the automerge (could not find a way to reference a label with markdown in github 😔) label?