refactor: Create and use Acts::SpacePoint as input for public interface of Seedfinder!

[robertlangenberg]

BREAKING CHANGE: Seedfinder requires use of Acts::SpacePoint as input. Seedfinder does not create InternalSpacePoints anymore. Instead, it requires the Acts::SpacePoint to be non-const, as it stores intermediate information in them.

Since Seedfinder shares the SeedFilters with SeedfinderOrthogonal, I hacked the conversion from InternalSpacePoint to SpacePoint into the SeedfinderOrthogonal at the point the SeedFilters are called. To be changed before un-WIP-ing

So far, Acts::SpacePoint have to be created from SimSpacePoint in the SeedingAlgorithm, as SimSpacePoint come from the eventStore. Change to Acts::SpacePoint and retrieve info directly from measurements?

Should Seed output contain a vector of SourceLinks, which point to the SimSpacePoints, or the full Acts::SpacePoint? If the former, destructor should garbage collect SourceLinks as well. If the latter, eventStore should contain Acts::SpacePoint.

[stale[bot]]

This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.

[tboldagh]

Is this still PR relevant? Before reviewing, it seems such approach would require conversion from/to an ATLAS version of SP if ATLAS chooses to use a different backend. Can we do better?

[robertlangenberg]

Hi @tboldagh, this PR aims at replacing the internal spacepoint with Acts::SpacePoint and offering to provide the Acts::SpacePoint directly. Currently, the InternalSpacePoints are created using data from the provided spacepoints, i.e. there is always a conversion. If ATLAS decides to use Acts::SpacePoint, this conversion won't be necessary anymore, if ATLAS wants to use something else the conversion to Acts::SpacePoint is now client-side, which does not incur extra overhead.

[tboldagh]

Hi @tboldagh, this PR aims at replacing the internal spacepoint with Acts::SpacePoint and offering to provide the Acts::SpacePoint directly. Currently, the InternalSpacePoints are created using data from the provided spacepoints, i.e. there is always a conversion. If ATLAS decides to use Acts::SpacePoint, this conversion won't be necessary anymore, if ATLAS wants to use something else the conversion to Acts::SpacePoint is now client-side, which does not incur extra overhead.

Right. But can we avoid the existence of explicitly defined SpacePoint class entirely? I.e. make code in the Core templated on SP? I believe seed finder already is agnostic to actual SP type. I.e. the type it operates on needs to provide x/y/z etc.

Also, is the pattern in which the input object is modified safe in MT age?

[paulgessinger]

As long as the SP container is not accessed by more than one thread, it should be fine I think.

[robertlangenberg]

Right. But can we avoid the existence of explicitly defined SpacePoint class entirely? I.e. make code in the Core templated on SP? I believe seed finder already is agnostic to actual SP type. I.e. the type it operates on needs to provide x/y/z etc.

The current Seedfinder currently accepts any type of SpacePoint and only expects x,y,z, that's correct. The problem is that this isn't properly communicated in the interface. So instead of introducing an Acts::SpacePoint, accessor functions could be put in the config as is done for other things (e.g. the strip information). If we completely want to avoid having a structure where we store x,y,z, we would need some other helper struct that contains "quality" and other properties of the SpacePoint that are being written by the Seedfinder. It would also require calculating x,y for each access, as these are offset by the beam spot, which makes this rather costly and it would probably be preferrable to keep the InternalSpacePoint instead.

Also, is the pattern in which the input object is modified safe in MT age?

The reason I suggested to do this is that SpacePoints are (afaik) only created for the Seeding, the other algorithms work directly on the measurements. So editing these should not affect other algorithms (unless there is a use case where the SpacePoints are to be kept for different purposes afterwards). Parallel processing within a single event pretty much went out the window a while ago, so editing the SP should not be a problem.

[stale[bot]]

This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.

[asalzburger]

I don't think we can build upon this development any further, closing this PR.