Skip to content

refactor!: Seed Finders create collection of Seed<external_spacepoint_t> instead of Seed<Proxy<external_spacepoint_t>> #3729

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.

Sign up for GitHub

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

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

refactor!: Seed Finders create collection of Seed<external_spacepoint_t> instead of Seed<Proxy<external_spacepoint_t>> #3729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e32e326
First Implementatio
Oct 13, 2024
a58f946
some changes but I need to fix the orthogonal
Oct 13, 2024
f06d0a6
can produce seed of externals
Oct 13, 2024
1fbb530
orthogonal works
Oct 13, 2024
6e45e75
remove line
Oct 13, 2024
bfdfd30
no double definition
Oct 31, 2024
d422e6e
change concept
Oct 31, 2024
1cd6974
rvalue
Oct 31, 2024
77217dd
nonesense
Oct 31, 2024
9412f22
compiles but orthognonal does not run
Nov 1, 2024
979f1e6
changes
Nov 2, 2024
319e1ed
properly works
Nov 2, 2024
b7aa9f8
include order
Nov 2, 2024
d0c0a6e
only seed of externals
Nov 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions Core/include/Acts/EventData/Seed.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ class Seed {
(std::same_as<external_spacepoint_t, args_t> && ...)
explicit Seed(const args_t&... points);

void setVertexZ(float vertex);
void setQuality(float seedQuality);
void setVertexZ(float vertex) noexcept;
void setQuality(float seedQuality) noexcept;

const std::array<const external_spacepoint_t*, N>& sp() const;
float z() const;
float seedQuality() const;
const std::array<const external_spacepoint_t*, N>& sp() const noexcept;
float z() const noexcept;
float seedQuality() const noexcept;

private:
std::array<const external_spacepoint_t*, N> m_spacepoints{};
Expand Down
10 changes: 5 additions & 5 deletions Core/include/Acts/EventData/Seed.ipp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,28 @@ Seed<external_spacepoint_t, N>::Seed(const args_t&... points)
: m_spacepoints({&points...}) {}

template <typename external_spacepoint_t, std::size_t N>
void Seed<external_spacepoint_t, N>::setVertexZ(float vertex) {
void Seed<external_spacepoint_t, N>::setVertexZ(float vertex) noexcept {
m_vertexZ = vertex;
}

template <typename external_spacepoint_t, std::size_t N>
void Seed<external_spacepoint_t, N>::setQuality(float seedQuality) {
void Seed<external_spacepoint_t, N>::setQuality(float seedQuality) noexcept {
m_seedQuality = seedQuality;
}

template <typename external_spacepoint_t, std::size_t N>
const std::array<const external_spacepoint_t*, N>&
Seed<external_spacepoint_t, N>::sp() const {
Seed<external_spacepoint_t, N>::sp() const noexcept {
return m_spacepoints;
}

template <typename external_spacepoint_t, std::size_t N>
float Seed<external_spacepoint_t, N>::z() const {
float Seed<external_spacepoint_t, N>::z() const noexcept {
return m_vertexZ;
}

template <typename external_spacepoint_t, std::size_t N>
float Seed<external_spacepoint_t, N>::seedQuality() const {
float Seed<external_spacepoint_t, N>::seedQuality() const noexcept {
return m_seedQuality;
}

Expand Down
12 changes: 4 additions & 8 deletions Core/include/Acts/EventData/SpacePointContainer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@ class SpacePointContainer {

using iterator =
ContainerIndexIterator<Acts::SpacePointContainer<container_t, holder_t>,
SpacePointProxyType&, false>;
SpacePointProxyType, false>;
using const_iterator =
ContainerIndexIterator<Acts::SpacePointContainer<container_t, holder_t>,
const SpacePointProxyType&, true>;
const SpacePointProxyType, true>;

using ValueType = typename container_t::ValueType;
using ProxyType = SpacePointProxyType;
Expand Down Expand Up @@ -126,17 +126,14 @@ class SpacePointContainer {
const_iterator begin() const;
const_iterator end() const;

ProxyType& at(const std::size_t n);
const ProxyType& at(const std::size_t n) const;
ProxyType at(const std::size_t n);
ProxyType at(const std::size_t n) const;
const ValueType& sp(const std::size_t n) const;

private:
void initialize();

const container_t& container() const;
const ProxyType& proxy(const std::size_t n) const;
std::vector<ProxyType>& proxies();
const std::vector<ProxyType>& proxies() const;

float x(const std::size_t n) const;
float y(const std::size_t n) const;
Expand All @@ -155,7 +152,6 @@ class SpacePointContainer {
Acts::SpacePointContainerOptions m_options;
Acts::SpacePointData m_data;
holder_t<const container_t> m_container;
std::vector<ProxyType> m_proxies;
};

} // namespace Acts
Expand Down
32 changes: 5 additions & 27 deletions Core/include/Acts/EventData/SpacePointContainer.ipp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ SpacePointContainer<container_t, holder_t>::SpacePointContainer(
template <typename container_t, template <typename> class holder_t>
void SpacePointContainer<container_t, holder_t>::initialize() {
m_data.resize(size(), m_config.useDetailedDoubleMeasurementInfo);
m_proxies.reserve(size());

const auto& external_container = container();
for (std::size_t i(0); i < size(); ++i) {
m_data.setX(i, external_container.x_impl(i) - m_options.beamPos[0]);
Expand All @@ -47,8 +47,6 @@ void SpacePointContainer<container_t, holder_t>::initialize() {
m_data.setPhi(i, std::atan2(m_data.y(i), m_data.x(i)));
m_data.setVarianceR(i, external_container.varianceR_impl(i));
m_data.setVarianceZ(i, external_container.varianceZ_impl(i));

m_proxies.emplace_back(*this, i);
}

// Dynamic variables
Expand Down Expand Up @@ -146,15 +144,15 @@ const container_t& SpacePointContainer<container_t, holder_t>::container()
}

template <typename container_t, template <typename> class holder_t>
typename SpacePointContainer<container_t, holder_t>::ProxyType&
typename SpacePointContainer<container_t, holder_t>::ProxyType
SpacePointContainer<container_t, holder_t>::at(const std::size_t n) {
return proxies().at(n);
return {*this, n};
}

template <typename container_t, template <typename> class holder_t>
const typename SpacePointContainer<container_t, holder_t>::ProxyType&
typename SpacePointContainer<container_t, holder_t>::ProxyType
SpacePointContainer<container_t, holder_t>::at(const std::size_t n) const {
return proxies().at(n);
return {*this, n};
}

template <typename container_t, template <typename> class holder_t>
Expand Down Expand Up @@ -202,24 +200,4 @@ float SpacePointContainer<container_t, holder_t>::varianceZ(
return m_data.varianceZ(n);
}

template <typename container_t, template <typename> class holder_t>
const typename SpacePointContainer<container_t, holder_t>::ProxyType&
SpacePointContainer<container_t, holder_t>::proxy(const std::size_t n) const {
assert(n < proxies().size());
return proxies()[n];
}

template <typename container_t, template <typename> class holder_t>
std::vector<typename SpacePointContainer<container_t, holder_t>::ProxyType>&
SpacePointContainer<container_t, holder_t>::proxies() {
return m_proxies;
}

template <typename container_t, template <typename> class holder_t>
const std::vector<
typename SpacePointContainer<container_t, holder_t>::ProxyType>&
SpacePointContainer<container_t, holder_t>::proxies() const {
return m_proxies;
}

} // namespace Acts
12 changes: 6 additions & 6 deletions Core/include/Acts/Seeding/Neighbour.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,14 @@ Neighbour<grid_t>::Neighbour(const grid_t& grid, std::size_t idx,

/// First check that the first element is not already above the lower bound
/// If so, avoid any computation and set the iterator to begin()
if (collection.front()->radius() > lowerBound) {
if (collection.front().radius() > lowerBound) {
itr = collection.begin();
}
/// In case the last element is below the lower bound, that means that there
/// can't be any element in that collection that can be considered a valuable
/// candidate.
/// Set the iterator to end() so that we do not run on this collection
else if (collection.back()->radius() < lowerBound) {
else if (collection.back().radius() < lowerBound) {
itr = collection.end();
}
/// Cannot decide a priori. We need to find the first element such that it's
Expand All @@ -79,18 +79,18 @@ Neighbour<grid_t>::Neighbour(const grid_t& grid, std::size_t idx,
std::size_t stop = collection.size() - 1;
while (start <= stop) {
std::size_t mid = (start + stop) / 2;
if (collection[mid]->radius() == lowerBound) {
if (collection[mid].radius() == lowerBound) {
itr = collection.begin() + mid;
return;
} else if (collection[mid]->radius() > lowerBound) {
if (mid > 0 && collection[mid - 1]->radius() < lowerBound) {
} else if (collection[mid].radius() > lowerBound) {
if (mid > 0 && collection[mid - 1].radius() < lowerBound) {
itr = collection.begin() + mid;
return;
}
stop = mid - 1;
} else {
if (mid + 1 < collection.size() &&
collection[mid + 1]->radius() > lowerBound) {
collection[mid + 1].radius() > lowerBound) {
itr = collection.begin() + mid + 1;
return;
}
Expand Down
26 changes: 26 additions & 0 deletions Core/include/Acts/Seeding/SeedFilter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "Acts/Seeding/CandidatesForMiddleSp.hpp"
#include "Acts/Seeding/IExperimentCuts.hpp"
#include "Acts/Seeding/SeedFilterConfig.hpp"
#include "Acts/Seeding/detail/UtilityFunctions.hpp"
#include "Acts/Utilities/Logger.hpp"

#include <memory>
Expand All @@ -22,6 +23,22 @@
#include <vector>

namespace Acts {
template <typename collection_t, typename external_t, std::size_t N = 3ul>
concept CollectionStoresSeedsTo =
requires(collection_t coll, Acts::Seed<external_t, N> seed) {
Acts::detail::pushBackOrInsertAtEnd(coll, seed);
};

template <typename collection_t, typename external_t, std::size_t N = 3ul>
concept CollectionStoresSeedsToProxied =
requires(external_t sp) {
typename external_t::ValueType;
sp.externalSpacePoint();
} && CollectionStoresSeedsTo<collection_t,
std::remove_const_t<std::remove_pointer_t<
typename external_t::ValueType> >,
N>;

struct SeedFilterState {
// longitudinal impact parameter as defined by bottom and middle space point
float zOrigin = 0;
Expand Down Expand Up @@ -105,6 +122,15 @@ class SeedFilter final {
private:
const Logger& logger() const { return *m_logger; }

template <typename collection_t>
requires Acts::CollectionStoresSeedsToProxied<collection_t,
external_spacepoint_t, 3ul>
void createAndStoreSeeds(collection_t& outputCollection,
const external_spacepoint_t& bottom,
const external_spacepoint_t& middle,
const external_spacepoint_t& top, float zOrigin,
float bestSeedQuality) const;

const SeedFilterConfig m_cfg;
std::unique_ptr<const Acts::Logger> m_logger =
Acts::getDefaultLogger("SeedFilter", Logging::Level::INFO);
Expand Down
27 changes: 22 additions & 5 deletions Core/include/Acts/Seeding/SeedFilter.ipp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,18 +303,35 @@ void SeedFilter<external_spacepoint_t>::filterSeeds_1SpFixed(
mutableData.setQuality(medium->index(), bestSeedQuality);
mutableData.setQuality(top->index(), bestSeedQuality);

Acts::Seed<external_spacepoint_t> seed{*bottom, *medium, *top};
seed.setVertexZ(zOrigin);
seed.setQuality(bestSeedQuality);

ACTS_VERBOSE("Adding seed: [b=" << bottom->index() << ", m="
<< medium->index() << ", t=" << top->index()
<< "], quality=" << bestSeedQuality
<< ", vertexZ=" << zOrigin);
Acts::detail::pushBackOrInsertAtEnd(outputCollection, std::move(seed));

createAndStoreSeeds(outputCollection, *bottom, *medium, *top,
bestSeedQuality, zOrigin);
++numTotalSeeds;
}
ACTS_VERBOSE("Identified " << numTotalSeeds << " seeds");
}

template <typename external_spacepoint_t>
template <typename collection_t>
requires Acts::CollectionStoresSeedsToProxied<collection_t,
external_spacepoint_t, 3ul>
void SeedFilter<external_spacepoint_t>::createAndStoreSeeds(
collection_t& outputCollection, const external_spacepoint_t& bottom,
const external_spacepoint_t& middle, const external_spacepoint_t& top,
float zOrigin, float bestSeedQuality) const {
using seed_type = Acts::Seed<std::remove_const_t<std::remove_pointer_t<
typename external_spacepoint_t::ValueType> >,
3ul>;
seed_type seed(*bottom.externalSpacePoint(), *middle.externalSpacePoint(),
*top.externalSpacePoint());
Comment on lines +329 to +330
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure externalSpacePoint() does not return null

Before dereferencing, confirm that externalSpacePoint() returns valid pointers to prevent potential null pointer dereferences.

Apply this diff to add assertions:

+  ACTS_ASSERT(bottom.externalSpacePoint() != nullptr);
+  ACTS_ASSERT(middle.externalSpacePoint() != nullptr);
+  ACTS_ASSERT(top.externalSpacePoint() != nullptr);

   seed_type seed(*bottom.externalSpacePoint(), *middle.externalSpacePoint(),
                  *top.externalSpacePoint());
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
seed_type seed(*bottom.externalSpacePoint(), *middle.externalSpacePoint(),
*top.externalSpacePoint());
ACTS_ASSERT(bottom.externalSpacePoint() != nullptr);
ACTS_ASSERT(middle.externalSpacePoint() != nullptr);
ACTS_ASSERT(top.externalSpacePoint() != nullptr);
seed_type seed(*bottom.externalSpacePoint(), *middle.externalSpacePoint(),
*top.externalSpacePoint());

seed.setVertexZ(zOrigin);
seed.setQuality(bestSeedQuality);

Acts::detail::pushBackOrInsertAtEnd(outputCollection, std::move(seed));
}

} // namespace Acts
10 changes: 2 additions & 8 deletions Core/include/Acts/Seeding/SeedFinder.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,6 @@ concept GridBinCollection =
std::ranges::random_access_range<Coll> &&
std::same_as<typename Coll::value_type, std::size_t>;

template <typename collection_t, typename external_t, std::size_t N = 3ul>
concept CollectionStoresSeedsTo =
requires(collection_t coll, Acts::Seed<external_t, N> seed) {
Acts::detail::pushBackOrInsertAtEnd(coll, seed);
};

enum class SpacePointCandidateType : short { eBottom, eTop };

enum class DetectorMeasurementInfo : short { eDefault, eDetailed };
Expand Down Expand Up @@ -119,8 +113,8 @@ class SeedFinder {
/// @note Ranges must return pointers.
/// @note Ranges must be separate objects for each parallel call.
template <typename container_t, Acts::GridBinCollection sp_range_t>
requires Acts::CollectionStoresSeedsTo<container_t, external_spacepoint_t,
3ul>
// requires Acts::CollectionStoresSeedsTo<container_t, external_spacepoint_t,
// 3ul>
void createSeedsForGroup(const Acts::SeedFinderOptions& options,
SeedingState& state, const grid_t& grid,
container_t& outputCollection,
Expand Down
Loading
Loading