From 7de28e229b9f78ad645f3bf8061893f403ffc614 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 17 Feb 2025 15:58:17 +0100 Subject: [PATCH 01/14] refactor: Improved GeometryIdentifier python bindings (#4085) This PR converts the getters and setters for the geometry id components into python *properties*. Also converts methods `Surface::geometryId()`, `Surface::type()` and `Surface::surfaceMaterial()` into readonly properties. --- Core/include/Acts/Geometry/GeometryObject.hpp | 4 +-- Examples/Python/python/acts/examples/odd.py | 8 ++--- Examples/Python/src/Geometry.cpp | 36 ++++++++++--------- Examples/Python/tests/test_detectors.py | 6 ++-- Examples/Scripts/Python/detector_creation.py | 2 +- ...elescope_track_params_lookup_generation.py | 2 +- 6 files changed, 30 insertions(+), 28 deletions(-) diff --git a/Core/include/Acts/Geometry/GeometryObject.hpp b/Core/include/Acts/Geometry/GeometryObject.hpp index 191d2234cd5..e76b27b4bfb 100644 --- a/Core/include/Acts/Geometry/GeometryObject.hpp +++ b/Core/include/Acts/Geometry/GeometryObject.hpp @@ -48,7 +48,7 @@ class GeometryObject { } /// @return the geometry id by reference - const GeometryIdentifier& geometryId() const; + GeometryIdentifier geometryId() const; /// Force a binning position method /// @@ -77,7 +77,7 @@ class GeometryObject { GeometryIdentifier m_geometryId; }; -inline const GeometryIdentifier& GeometryObject::geometryId() const { +inline GeometryIdentifier GeometryObject::geometryId() const { return m_geometryId; } diff --git a/Examples/Python/python/acts/examples/odd.py b/Examples/Python/python/acts/examples/odd.py index e0edef15776..79afcdcb29a 100644 --- a/Examples/Python/python/acts/examples/odd.py +++ b/Examples/Python/python/acts/examples/odd.py @@ -76,13 +76,13 @@ def getOpenDataDetector( def geoid_hook(geoid, surface): gctx = acts.GeometryContext() - if geoid.volume() in volumeRadiusCutsMap: + if geoid.volume in volumeRadiusCutsMap: r = math.sqrt(surface.center(gctx)[0] ** 2 + surface.center(gctx)[1] ** 2) - geoid.setExtra(1) - for cut in volumeRadiusCutsMap[geoid.volume()]: + geoid.extra = 1 + for cut in volumeRadiusCutsMap[geoid.volume]: if r > cut: - geoid.setExtra(geoid.extra() + 1) + geoid.extra += 1 return geoid diff --git a/Examples/Python/src/Geometry.cpp b/Examples/Python/src/Geometry.cpp index 0b7834e78db..d86fa05cb6c 100644 --- a/Examples/Python/src/Geometry.cpp +++ b/Examples/Python/src/Geometry.cpp @@ -103,19 +103,20 @@ void addGeometry(Context& ctx) { py::class_(m, "GeometryIdentifier") .def(py::init<>()) .def(py::init()) - .def("setVolume", &Acts::GeometryIdentifier::setVolume) - .def("setLayer", &Acts::GeometryIdentifier::setLayer) - .def("setBoundary", &Acts::GeometryIdentifier::setBoundary) - .def("setApproach", &Acts::GeometryIdentifier::setApproach) - .def("setSensitive", &Acts::GeometryIdentifier::setSensitive) - .def("setExtra", &Acts::GeometryIdentifier::setExtra) - .def("volume", &Acts::GeometryIdentifier::volume) - .def("layer", &Acts::GeometryIdentifier::layer) - .def("boundary", &Acts::GeometryIdentifier::boundary) - .def("approach", &Acts::GeometryIdentifier::approach) - .def("sensitive", &Acts::GeometryIdentifier::sensitive) - .def("extra", &Acts::GeometryIdentifier::extra) - .def("value", &Acts::GeometryIdentifier::value) + + .def_property("volume", &Acts::GeometryIdentifier::volume, + &Acts::GeometryIdentifier::setVolume) + .def_property("layer", &Acts::GeometryIdentifier::layer, + &Acts::GeometryIdentifier::setLayer) + .def_property("boundary", &Acts::GeometryIdentifier::boundary, + &Acts::GeometryIdentifier::setBoundary) + .def_property("approach", &Acts::GeometryIdentifier::approach, + &Acts::GeometryIdentifier::setApproach) + .def_property("sensitive", &Acts::GeometryIdentifier::sensitive, + &Acts::GeometryIdentifier::setSensitive) + .def_property("extra", &Acts::GeometryIdentifier::extra, + &Acts::GeometryIdentifier::setExtra) + .def_property_readonly("value", &Acts::GeometryIdentifier::value) .def("__str__", [](const Acts::GeometryIdentifier& self) { std::stringstream ss; ss << self; @@ -126,12 +127,13 @@ void addGeometry(Context& ctx) { { py::class_>(m, "Surface") // Can't bind directly because GeometryObject is virtual base of Surface - .def("geometryId", - [](const Surface& self) { return self.geometryId(); }) + .def_property_readonly( + "geometryId", [](const Surface& self) { return self.geometryId(); }) .def("center", &Surface::center) - .def("type", &Surface::type) + .def_property_readonly("type", &Surface::type) .def("visualize", &Surface::visualize) - .def("surfaceMaterial", &Acts::Surface::surfaceMaterialSharedPtr); + .def_property_readonly("surfaceMaterial", + &Acts::Surface::surfaceMaterialSharedPtr); } { diff --git a/Examples/Python/tests/test_detectors.py b/Examples/Python/tests/test_detectors.py index bbfb715697f..569f9c6c43c 100644 --- a/Examples/Python/tests/test_detectors.py +++ b/Examples/Python/tests/test_detectors.py @@ -21,8 +21,8 @@ def visit(srf): def check_extra_odd(srf): - if srf.geometryId().volume() in [28, 30, 23, 25, 16, 18]: - assert srf.geometryId().extra() != 0 + if srf.geometryId.volume in [28, 30, 23, 25, 16, 18]: + assert srf.geometryId.extra != 0 return @@ -183,7 +183,7 @@ def test_coordinate_converter(trk_geo): def test_surface(surface): gctx = acts.GeometryContext() - geo_id = surface.geometryId().value() + geo_id = surface.geometryId.value geo_center = surface.center(gctx) x, y, z = geo_center[0], geo_center[1], geo_center[2] diff --git a/Examples/Scripts/Python/detector_creation.py b/Examples/Scripts/Python/detector_creation.py index 8c90078d8d5..c30fdfe8dda 100644 --- a/Examples/Scripts/Python/detector_creation.py +++ b/Examples/Scripts/Python/detector_creation.py @@ -53,7 +53,7 @@ viewConfig.nSegments = 100 for vol in detector.volumePtrs(): for surf in vol.surfacePtrs(): - if surf.geometryId().sensitive() > 0: + if surf.geometryId.sensitive > 0: surfaces.append(surf) acts.examples.writeSurfacesObj(surfaces, geoContext, viewConfig, "odd-surfaces.obj") diff --git a/Examples/Scripts/Python/telescope_track_params_lookup_generation.py b/Examples/Scripts/Python/telescope_track_params_lookup_generation.py index 11247297c6d..02c91e15585 100644 --- a/Examples/Scripts/Python/telescope_track_params_lookup_generation.py +++ b/Examples/Scripts/Python/telescope_track_params_lookup_generation.py @@ -59,7 +59,7 @@ def estimateLookup(trackingGeometry, numEvents, outputPath): # Set up the track estimation algorithm surfaces = list(trackingGeometry.geoIdSurfaceMap().values()) refSurface = surfaces[0] - refGeometryId = refSurface.geometryId() + refGeometryId = refSurface.geometryId trackEstConfig = acts.examples.TrackParamsLookupEstimation.Config( refLayers={refGeometryId: refSurface}, From 83a9f632dd1ca2b36599c7dce826836c11647786 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 18 Feb 2025 10:31:50 +0100 Subject: [PATCH 02/14] ci: Use repo slug instead of git url for cache key (#4093) This changes to use the newly added REPO_SLUG variable for the ccache cache key in GitLab. --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 67789fdb8a4..3d19e27c5de 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -8,7 +8,7 @@ variables: .ccache_base: cache: - - key: ccache-${CI_JOB_NAME}-${CCACHE_KEY_SUFFIX}-${CLONE_URL}_${HEAD_REF} + - key: ccache-${CI_JOB_NAME}-${CCACHE_KEY_SUFFIX}-${REPO_SLUG}_${HEAD_REF} fallback_keys: - ccache-${CI_JOB_NAME}-${CCACHE_KEY_SUFFIX}-https://github.com/acts-project/acts.git-main when: always From 6b9bf3176b01ed301e405c13b7dd9eafff630624 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 18 Feb 2025 11:55:47 +0100 Subject: [PATCH 03/14] build: Remove `ACTS_BUILD_NONCOMPILE_TESTS` (#4092) Seems like I forgot this in https://github.com/acts-project/acts/pull/4076 --- CMakePresets.json | 1 - 1 file changed, 1 deletion(-) diff --git a/CMakePresets.json b/CMakePresets.json index fc7275cbd4e..b8226b30911 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -51,7 +51,6 @@ "ACTS_FORCE_ASSERTIONS": "ON", "ACTS_ENABLE_LOG_FAILURE_THRESHOLD": "ON", "ACTS_BUILD_BENCHMARKS": "ON", - "ACTS_BUILD_NONCOMPILE_TESTS": "ON", "ACTS_BUILD_ANALYSIS_APPS": "ON", "ACTS_BUILD_ALIGNMENT": "ON", "ACTS_BUILD_FATRAS_GEANT4": "ON", From 1729984e22fe11a56074108bba44b6ed1a1b088f Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 18 Feb 2025 13:33:44 +0100 Subject: [PATCH 04/14] refactor: GeometryIdentifier immutable modification (#4095) This adds `withXXX` methods that return a new GeometryIdentifier with one of the components replaced with a different value. This is currently in addition to the existing `setXXX` methods, which are not modified to remain API compatible. In future, I'm planning to make the `setXXX` methods not return a reference to it's original value, to encourage using the `withXXX` methods and decrease the likelihood of accidental misuse. --- .../Acts/Geometry/GeometryHierarchyMap.hpp | 47 ++++++------ .../Acts/Geometry/GeometryIdentifier.hpp | 75 ++++++++++++++++++- Core/src/Geometry/Layer.cpp | 4 +- Core/src/Geometry/TrackingVolume.cpp | 10 +-- .../src/DigitizationConfigurator.cpp | 4 +- .../EventData/GeometryContainers.hpp | 16 ++-- .../GeometryHierarchyMapJsonConverter.hpp | 13 ++-- .../UnitTests/Core/Detector/DetectorTests.cpp | 4 +- .../EventData/MultiTrajectoryHelpersTests.cpp | 5 +- .../Geometry/GeometryHierarchyMapTests.cpp | 2 +- .../Core/Geometry/GeometryIdentifierTests.cpp | 52 ++++++------- .../BinnedSurfaceMaterialAccumulaterTests.cpp | 12 +-- .../MaterialInteractionAssignmentTests.cpp | 17 +++-- .../Core/Material/MaterialMapperTests.cpp | 2 +- .../PropagatorMaterialAssignerTests.cpp | 6 +- .../Core/Propagator/DirectNavigatorTests.cpp | 4 +- .../SpacePointBuilderTests.cpp | 10 +-- .../CombinatorialKalmanFilterTests.cpp | 10 +-- .../Core/TrackFinding/TrackSelectorTests.cpp | 36 ++++----- .../Core/TrackFitting/FitterTestsCommon.hpp | 10 +-- .../UnitTests/Core/TrackFitting/Gx2fTests.cpp | 16 ++-- .../Io/Json/JsonDigitizationConfigTests.cpp | 2 +- .../UncorrelatedHitSmearerTests.cpp | 2 +- Tests/UnitTests/Fatras/EventData/HitTests.cpp | 2 +- .../Detray/DetrayGeometryConverterTests.cpp | 2 +- ...GeometryHierarchyMapJsonConverterTests.cpp | 2 +- 26 files changed, 222 insertions(+), 143 deletions(-) diff --git a/Core/include/Acts/Geometry/GeometryHierarchyMap.hpp b/Core/include/Acts/Geometry/GeometryHierarchyMap.hpp index 61cdcdb5e6d..0c2e6f996aa 100644 --- a/Core/include/Acts/Geometry/GeometryHierarchyMap.hpp +++ b/Core/include/Acts/Geometry/GeometryHierarchyMap.hpp @@ -132,18 +132,19 @@ class GeometryHierarchyMap { // NOTE this class assumes that it knows the ordering of the levels within // the geometry id. if the geometry id changes, this code has to be // adapted too. the asserts ensure that such a change is caught. - static_assert(GeometryIdentifier().setVolume(1).value() < - GeometryIdentifier().setVolume(1).setBoundary(1).value(), + static_assert(GeometryIdentifier().withVolume(1).value() < + GeometryIdentifier().withVolume(1).withBoundary(1).value(), "Incompatible GeometryIdentifier hierarchy"); - static_assert(GeometryIdentifier().setBoundary(1).value() < - GeometryIdentifier().setBoundary(1).setLayer(1).value(), + static_assert(GeometryIdentifier().withBoundary(1).value() < + GeometryIdentifier().withBoundary(1).withLayer(1).value(), "Incompatible GeometryIdentifier hierarchy"); - static_assert(GeometryIdentifier().setLayer(1).value() < - GeometryIdentifier().setLayer(1).setApproach(1).value(), - "Incompatible GeometryIdentifier hierarchy"); - static_assert(GeometryIdentifier().setApproach(1).value() < - GeometryIdentifier().setApproach(1).setSensitive(1).value(), + static_assert(GeometryIdentifier().withLayer(1).value() < + GeometryIdentifier().withLayer(1).withApproach(1).value(), "Incompatible GeometryIdentifier hierarchy"); + static_assert( + GeometryIdentifier().withApproach(1).value() < + GeometryIdentifier().withApproach(1).withSensitive(1).value(), + "Incompatible GeometryIdentifier hierarchy"); using Identifier = GeometryIdentifier::Value; @@ -156,31 +157,31 @@ class GeometryHierarchyMap { /// Construct a mask where all leading non-zero levels are set. static constexpr Identifier makeLeadingLevelsMask(GeometryIdentifier id) { // construct id from encoded value with all bits set - auto allSet = GeometryIdentifier(~GeometryIdentifier::Value{0u}); + const auto allSet = GeometryIdentifier(~GeometryIdentifier::Value{0u}); // manually iterate over identifier levels starting from the lowest if (id.sensitive() != 0u) { // all levels are valid; keep all bits set. - return allSet.setExtra(0u).value(); + return allSet.withExtra(0u).value(); } if (id.approach() != 0u) { - return allSet.setExtra(0u).setSensitive(0u).value(); + return allSet.withExtra(0u).withSensitive(0u).value(); } if (id.layer() != 0u) { - return allSet.setExtra(0u).setSensitive(0u).setApproach(0u).value(); + return allSet.withExtra(0u).withSensitive(0u).withApproach(0u).value(); } if (id.boundary() != 0u) { - return allSet.setExtra(0u) - .setSensitive(0u) - .setApproach(0u) - .setLayer(0u) + return allSet.withExtra(0u) + .withSensitive(0u) + .withApproach(0u) + .withLayer(0u) .value(); } if (id.volume() != 0u) { - return allSet.setExtra(0u) - .setSensitive(0u) - .setApproach(0u) - .setLayer(0u) - .setBoundary(0u) + return allSet.withExtra(0u) + .withSensitive(0u) + .withApproach(0u) + .withLayer(0u) + .withBoundary(0u) .value(); } // no valid levels; all bits are zero. @@ -189,7 +190,7 @@ class GeometryHierarchyMap { /// Construct a mask where only the highest level is set. static constexpr Identifier makeHighestLevelMask() { - return makeLeadingLevelsMask(GeometryIdentifier(0u).setVolume(1u)); + return makeLeadingLevelsMask(GeometryIdentifier(0u).withVolume(1u)); } /// Compare the two identifiers only within the masked bits. diff --git a/Core/include/Acts/Geometry/GeometryIdentifier.hpp b/Core/include/Acts/Geometry/GeometryIdentifier.hpp index 6b43e510f78..e6222422eb3 100644 --- a/Core/include/Acts/Geometry/GeometryIdentifier.hpp +++ b/Core/include/Acts/Geometry/GeometryIdentifier.hpp @@ -11,6 +11,7 @@ #include #include #include +#include namespace Acts { @@ -90,6 +91,76 @@ class GeometryIdentifier { return setBits(kExtraMask, extra); } + /// Return a new identifier with the volume set to @p volume + /// @param volume the new volume identifier + /// @return a new identifier with the volume set to @p volume + [[nodiscard]] + constexpr GeometryIdentifier withVolume(Value volume) const { + GeometryIdentifier id = *this; + id.setVolume(volume); + return id; + } + + /// Return a new identifier with the boundary set to @p boundary + /// @param boundary the new boundary identifier + /// @return a new identifier with the boundary set to @p boundary + [[nodiscard]] + constexpr GeometryIdentifier withBoundary(Value boundary) const { + GeometryIdentifier id = *this; + id.setBoundary(boundary); + return id; + } + + /// Return a new identifier with the layer set to @p layer + /// @param layer the new layer identifier + /// @return a new identifier with the layer set to @p layer + [[nodiscard]] + constexpr GeometryIdentifier withLayer(Value layer) const { + GeometryIdentifier id = *this; + id.setLayer(layer); + return id; + } + + /// Return a new identifier with the approach set to @p approach + /// @param approach the new approach identifier + /// @return a new identifier with the approach set to @p approach + [[nodiscard]] + constexpr GeometryIdentifier withApproach(Value approach) const { + GeometryIdentifier id = *this; + id.setApproach(approach); + return id; + } + + /// Return a new identifier with the passive set to @p passive + /// @param passive the new passive identifier + /// @return a new identifier with the passive set to @p passive + [[nodiscard]] + constexpr GeometryIdentifier withPassive(Value passive) const { + GeometryIdentifier id = *this; + id.setPassive(passive); + return id; + } + + /// Return a new identifier with the sensitive set to @p sensitive + /// @param sensitive the new sensitive identifier + /// @return a new identifier with the sensitive set to @p sensitive + [[nodiscard]] + constexpr GeometryIdentifier withSensitive(Value sensitive) const { + GeometryIdentifier id = *this; + id.setSensitive(sensitive); + return id; + } + + /// Return a new identifier with the extra set to @p extra + /// @param extra the new extra identifier + /// @return a new identifier with the extra set to @p extra + [[nodiscard]] + constexpr GeometryIdentifier withExtra(Value extra) const { + GeometryIdentifier id = *this; + id.setExtra(extra); + return id; + } + private: // clang-format off /// (2^8)-1 = 255 volumes @@ -118,6 +189,7 @@ class GeometryIdentifier { // WARNING undefined behaviour for mask == 0 which we should not have. return __builtin_ctzll(mask); } + /// Extract the masked bits from the encoded value. constexpr Value getBits(Value mask) const { return (m_value & mask) >> extractShift(mask); @@ -125,7 +197,8 @@ class GeometryIdentifier { /// Set the masked bits to id in the encoded value. constexpr GeometryIdentifier& setBits(Value mask, Value id) { m_value = (m_value & ~mask) | ((id << extractShift(mask)) & mask); - // return *this here that we need to write fewer lines in the setXXX methods + // return *this here that we need to write fewer lines in the setXXX + // methods return *this; } diff --git a/Core/src/Geometry/Layer.cpp b/Core/src/Geometry/Layer.cpp index 024f99e9c42..2224f093517 100644 --- a/Core/src/Geometry/Layer.cpp +++ b/Core/src/Geometry/Layer.cpp @@ -76,7 +76,7 @@ void Acts::Layer::closeGeometry(const IMaterialDecorator* materialDecorator, // loop through the approachSurfaces and assign unique GeomeryID GeometryIdentifier::Value iasurface = 0; for (auto& aSurface : m_approachDescriptor->containedSurfaces()) { - auto asurfaceID = GeometryIdentifier(layerID).setApproach(++iasurface); + auto asurfaceID = GeometryIdentifier(layerID).withApproach(++iasurface); auto mutableASurface = const_cast(aSurface); mutableASurface->assignGeometryId(asurfaceID); if (materialDecorator != nullptr) { @@ -95,7 +95,7 @@ void Acts::Layer::closeGeometry(const IMaterialDecorator* materialDecorator, // loop sensitive surfaces and assign unique GeometryIdentifier GeometryIdentifier::Value issurface = 0; for (auto& sSurface : m_surfaceArray->surfaces()) { - auto ssurfaceID = GeometryIdentifier(layerID).setSensitive(++issurface); + auto ssurfaceID = GeometryIdentifier(layerID).withSensitive(++issurface); ssurfaceID = hook.decorateIdentifier(ssurfaceID, *sSurface); auto mutableSSurface = const_cast(sSurface); mutableSSurface->assignGeometryId(ssurfaceID); diff --git a/Core/src/Geometry/TrackingVolume.cpp b/Core/src/Geometry/TrackingVolume.cpp index ac56a59ee13..2981c3495fa 100644 --- a/Core/src/Geometry/TrackingVolume.cpp +++ b/Core/src/Geometry/TrackingVolume.cpp @@ -381,7 +381,7 @@ void TrackingVolume::closeGeometry( } // we can construct the volume ID from this - auto volumeID = GeometryIdentifier().setVolume(++vol); + auto volumeID = GeometryIdentifier().withVolume(++vol); // assign the Volume ID to the volume itself auto thisVolume = const_cast(this); thisVolume->assignGeometryId(volumeID); @@ -413,7 +413,7 @@ void TrackingVolume::closeGeometry( auto& bSurface = bSurfIter->surfaceRepresentation(); // create the boundary surface id iboundary++; - auto boundaryID = GeometryIdentifier(volumeID).setBoundary(iboundary); + auto boundaryID = GeometryIdentifier(volumeID).withBoundary(iboundary); // now assign to the boundary surface auto& mutableBSurface = *(const_cast(&bSurface)); mutableBSurface.assignGeometryId(boundaryID); @@ -432,7 +432,7 @@ void TrackingVolume::closeGeometry( for (auto& layerPtr : m_confinedLayers->arrayObjects()) { // create the layer identification ilayer++; - auto layerID = GeometryIdentifier(volumeID).setLayer(ilayer); + auto layerID = GeometryIdentifier(volumeID).withLayer(ilayer); // now close the geometry auto mutableLayerPtr = std::const_pointer_cast(layerPtr); mutableLayerPtr->closeGeometry(materialDecorator, layerID, hook, @@ -464,7 +464,7 @@ void TrackingVolume::closeGeometry( GeometryIdentifier::Value iportal = 0; for (auto& portal : portals()) { iportal++; - auto portalId = GeometryIdentifier(volumeID).setBoundary(iportal); + auto portalId = GeometryIdentifier(volumeID).withBoundary(iportal); assert(portal.isValid() && "Invalid portal encountered during closing"); portal.surface().assignGeometryId(portalId); @@ -477,7 +477,7 @@ void TrackingVolume::closeGeometry( continue; } isensitive++; - auto sensitiveId = GeometryIdentifier(volumeID).setSensitive(isensitive); + auto sensitiveId = GeometryIdentifier(volumeID).withSensitive(isensitive); surface.assignGeometryId(sensitiveId); } diff --git a/Examples/Algorithms/Digitization/src/DigitizationConfigurator.cpp b/Examples/Algorithms/Digitization/src/DigitizationConfigurator.cpp index fdac7783db2..c512bfe3f2b 100644 --- a/Examples/Algorithms/Digitization/src/DigitizationConfigurator.cpp +++ b/Examples/Algorithms/Digitization/src/DigitizationConfigurator.cpp @@ -249,7 +249,7 @@ void ActsExamples::DigitizationConfigurator::operator()( // Check for a representing volume configuration, insert if not // present Acts::GeometryIdentifier volGeoId = - Acts::GeometryIdentifier().setVolume(geoId.volume()); + Acts::GeometryIdentifier().withVolume(geoId.volume()); auto volRep = volumeLayerComponents.find(volGeoId); if (volRep != volumeLayerComponents.end() && @@ -263,7 +263,7 @@ void ActsExamples::DigitizationConfigurator::operator()( // Check for a representing layer configuration, insert if not present Acts::GeometryIdentifier volLayGeoId = - Acts::GeometryIdentifier(volGeoId).setLayer(geoId.layer()); + Acts::GeometryIdentifier(volGeoId).withLayer(geoId.layer()); auto volLayRep = volumeLayerComponents.find(volLayGeoId); if (volLayRep != volumeLayerComponents.end() && diff --git a/Examples/Framework/include/ActsExamples/EventData/GeometryContainers.hpp b/Examples/Framework/include/ActsExamples/EventData/GeometryContainers.hpp index 3ffb0c0ac3b..3bee6cad13f 100644 --- a/Examples/Framework/include/ActsExamples/EventData/GeometryContainers.hpp +++ b/Examples/Framework/include/ActsExamples/EventData/GeometryContainers.hpp @@ -103,11 +103,11 @@ template inline Range::const_iterator> selectVolume( const GeometryIdMultiset& container, Acts::GeometryIdentifier::Value volume) { - auto cmp = Acts::GeometryIdentifier().setVolume(volume); + auto cmp = Acts::GeometryIdentifier().withVolume(volume); auto beg = std::lower_bound(container.begin(), container.end(), cmp, detail::CompareGeometryId{}); // WARNING overflows to volume==0 if the input volume is the last one - cmp = Acts::GeometryIdentifier().setVolume(volume + 1u); + cmp = Acts::GeometryIdentifier().withVolume(volume + 1u); // optimize search by using the lower bound as start point. also handles // volume overflows since the geo id would be located before the start of // the upper edge search window. @@ -129,11 +129,11 @@ inline Range::const_iterator> selectLayer( const GeometryIdMultiset& container, Acts::GeometryIdentifier::Value volume, Acts::GeometryIdentifier::Value layer) { - auto cmp = Acts::GeometryIdentifier().setVolume(volume).setLayer(layer); + auto cmp = Acts::GeometryIdentifier().withVolume(volume).withLayer(layer); auto beg = std::lower_bound(container.begin(), container.end(), cmp, detail::CompareGeometryId{}); // WARNING resets to layer==0 if the input layer is the last one - cmp = Acts::GeometryIdentifier().setVolume(volume).setLayer(layer + 1u); + cmp = Acts::GeometryIdentifier().withVolume(volume).withLayer(layer + 1u); // optimize search by using the lower bound as start point. also handles // volume overflows since the geo id would be located before the start of // the upper edge search window. @@ -163,10 +163,10 @@ inline auto selectModule(const GeometryIdMultiset& container, Acts::GeometryIdentifier::Value volume, Acts::GeometryIdentifier::Value layer, Acts::GeometryIdentifier::Value sensitive) { - return selectModule( - container, - Acts::GeometryIdentifier().setVolume(volume).setLayer(layer).setSensitive( - sensitive)); + return selectModule(container, Acts::GeometryIdentifier() + .withVolume(volume) + .withLayer(layer) + .withSensitive(sensitive)); } /// Select all elements for the lowest non-zero identifier component. diff --git a/Plugins/Json/include/Acts/Plugins/Json/GeometryHierarchyMapJsonConverter.hpp b/Plugins/Json/include/Acts/Plugins/Json/GeometryHierarchyMapJsonConverter.hpp index 74ce7109975..c1203cd02a8 100644 --- a/Plugins/Json/include/Acts/Plugins/Json/GeometryHierarchyMapJsonConverter.hpp +++ b/Plugins/Json/include/Acts/Plugins/Json/GeometryHierarchyMapJsonConverter.hpp @@ -96,12 +96,13 @@ class GeometryHierarchyMapJsonConverter { /// @return a valid geometry Identifier static GeometryIdentifier decodeIdentifier(const nlohmann::json& encoded) { return GeometryIdentifier() - .setVolume(encoded.value("volume", GeometryIdentifier::Value{0u})) - .setBoundary(encoded.value("boundary", GeometryIdentifier::Value{0u})) - .setLayer(encoded.value("layer", GeometryIdentifier::Value{0u})) - .setApproach(encoded.value("approach", GeometryIdentifier::Value{0u})) - .setSensitive(encoded.value("sensitive", GeometryIdentifier::Value{0u})) - .setExtra(encoded.value("extra", GeometryIdentifier::Value{0u})); + .withVolume(encoded.value("volume", GeometryIdentifier::Value{0u})) + .withBoundary(encoded.value("boundary", GeometryIdentifier::Value{0u})) + .withLayer(encoded.value("layer", GeometryIdentifier::Value{0u})) + .withApproach(encoded.value("approach", GeometryIdentifier::Value{0u})) + .withSensitive( + encoded.value("sensitive", GeometryIdentifier::Value{0u})) + .withExtra(encoded.value("extra", GeometryIdentifier::Value{0u})); } private: diff --git a/Tests/UnitTests/Core/Detector/DetectorTests.cpp b/Tests/UnitTests/Core/Detector/DetectorTests.cpp index b35a008aec7..9b2737e2586 100644 --- a/Tests/UnitTests/Core/Detector/DetectorTests.cpp +++ b/Tests/UnitTests/Core/Detector/DetectorTests.cpp @@ -241,7 +241,7 @@ BOOST_AUTO_TEST_CASE(DetectorConstructionWithHierarchyMap) { Acts::Transform3::Identity(), std::make_shared(r, 190.), 0.1); auto surface = detElement->surface().getSharedPtr(); - surface->assignGeometryId(Acts::GeometryIdentifier{}.setSensitive(ir + 1)); + surface->assignGeometryId(Acts::GeometryIdentifier{}.withSensitive(ir + 1)); surfaces.push_back(std::move(surface)); detStore.push_back(std::move(detElement)); } @@ -266,7 +266,7 @@ BOOST_AUTO_TEST_CASE(DetectorConstructionWithHierarchyMap) { const auto& sensitiveHierarchyMap = det->sensitiveHierarchyMap(); const Acts::Surface* surface0 = - det->findSurface(Acts::GeometryIdentifier{}.setSensitive(1)); + det->findSurface(Acts::GeometryIdentifier{}.withSensitive(1)); BOOST_CHECK_EQUAL(sensitiveHierarchyMap.size(), 6u); BOOST_CHECK_NE(surface0, nullptr); diff --git a/Tests/UnitTests/Core/EventData/MultiTrajectoryHelpersTests.cpp b/Tests/UnitTests/Core/EventData/MultiTrajectoryHelpersTests.cpp index 2979e1ce658..5c0245d83c6 100644 --- a/Tests/UnitTests/Core/EventData/MultiTrajectoryHelpersTests.cpp +++ b/Tests/UnitTests/Core/EventData/MultiTrajectoryHelpersTests.cpp @@ -70,9 +70,10 @@ BOOST_AUTO_TEST_CASE(trajectoryStateVolume) { std::vector volumes = {searchVolume}; auto searchSurface = Surface::makeShared(Vector3(0, 0, 0)); - searchSurface->assignGeometryId(GeometryIdentifier().setVolume(searchVolume)); + searchSurface->assignGeometryId( + GeometryIdentifier().withVolume(searchVolume)); auto otherSurface = Surface::makeShared(Vector3(0, 0, 0)); - otherSurface->assignGeometryId(GeometryIdentifier().setVolume(otherVolume)); + otherSurface->assignGeometryId(GeometryIdentifier().withVolume(otherVolume)); VectorMultiTrajectory traj; diff --git a/Tests/UnitTests/Core/Geometry/GeometryHierarchyMapTests.cpp b/Tests/UnitTests/Core/Geometry/GeometryHierarchyMapTests.cpp index a151e932d94..7bc7c5d8059 100644 --- a/Tests/UnitTests/Core/Geometry/GeometryHierarchyMapTests.cpp +++ b/Tests/UnitTests/Core/Geometry/GeometryHierarchyMapTests.cpp @@ -22,7 +22,7 @@ using Acts::GeometryIdentifier; // helper function to create geometry ids GeometryIdentifier makeId(int volume = 0, int layer = 0, int sensitive = 0) { - return GeometryIdentifier().setVolume(volume).setLayer(layer).setSensitive( + return GeometryIdentifier().withVolume(volume).withLayer(layer).withSensitive( sensitive); } diff --git a/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp b/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp index b7e7a163d50..ffea49752f6 100644 --- a/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp +++ b/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp @@ -60,35 +60,35 @@ BOOST_AUTO_TEST_CASE(GeometryIdentifier_max_values) { } BOOST_AUTO_TEST_CASE(GeometryIdentifier_order) { - auto vol1 = GeometryIdentifier() - .setVolume(1u) - .setLayer(14u) - .setSensitive(5u) - .setExtra(42u); - auto vol2 = GeometryIdentifier() - .setVolume(2u) - .setLayer(13u) - .setSensitive(3u) - .setExtra(43u); + GeometryIdentifier vol1; + vol1.setVolume(1u); + vol1.setLayer(14u); + vol1.setSensitive(5u); + vol1.setExtra(42u); + GeometryIdentifier vol2; + vol2.setVolume(2u); + vol2.setLayer(13u); + vol2.setSensitive(3u); + vol2.setExtra(43u); // order uses volume first even if other components are larger BOOST_CHECK_LT(vol1, vol2); - BOOST_CHECK_LT(GeometryIdentifier(vol1).setBoundary(64u), vol2); - BOOST_CHECK_LT(GeometryIdentifier(vol1).setLayer(64u), vol2); - BOOST_CHECK_LT(GeometryIdentifier(vol1).setApproach(64u), vol2); - BOOST_CHECK_LT(GeometryIdentifier(vol1).setSensitive(64u), vol2); - BOOST_CHECK_LT(GeometryIdentifier(vol1).setSensitive(64u), vol2); - BOOST_CHECK_LT(vol2, GeometryIdentifier(vol1).setVolume(3u)); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withBoundary(64u), vol2); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withLayer(64u), vol2); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withApproach(64u), vol2); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withSensitive(64u), vol2); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withSensitive(64u), vol2); + BOOST_CHECK_LT(vol2, GeometryIdentifier(vol1).withVolume(3u)); // other components are hierarchical - BOOST_CHECK_LT(GeometryIdentifier(vol1).setVolume(1u).setBoundary(2u), - GeometryIdentifier(vol1).setVolume(2u).setBoundary(1u)); - BOOST_CHECK_LT(GeometryIdentifier(vol1).setBoundary(1u).setLayer(2u), - GeometryIdentifier(vol1).setBoundary(2u).setLayer(1u)); - BOOST_CHECK_LT(GeometryIdentifier(vol1).setLayer(1u).setApproach(2u), - GeometryIdentifier(vol1).setLayer(2u).setApproach(1u)); - BOOST_CHECK_LT(GeometryIdentifier(vol1).setApproach(1u).setSensitive(2u), - GeometryIdentifier(vol1).setApproach(2u).setSensitive(1u)); - BOOST_CHECK_LT(GeometryIdentifier(vol1).setSensitive(1u).setExtra(2u), - GeometryIdentifier(vol1).setSensitive(2u).setExtra(1u)); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withVolume(1u).withBoundary(2u), + GeometryIdentifier(vol1).withVolume(2u).withBoundary(1u)); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withBoundary(1u).withLayer(2u), + GeometryIdentifier(vol1).withBoundary(2u).withLayer(1u)); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withLayer(1u).withApproach(2u), + GeometryIdentifier(vol1).withLayer(2u).withApproach(1u)); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withApproach(1u).withSensitive(2u), + GeometryIdentifier(vol1).withApproach(2u).withSensitive(1u)); + BOOST_CHECK_LT(GeometryIdentifier(vol1).withSensitive(1u).withExtra(2u), + GeometryIdentifier(vol1).withSensitive(2u).withExtra(1u)); } } // namespace Acts::Test diff --git a/Tests/UnitTests/Core/Material/BinnedSurfaceMaterialAccumulaterTests.cpp b/Tests/UnitTests/Core/Material/BinnedSurfaceMaterialAccumulaterTests.cpp index d3759c16400..b02373e5268 100644 --- a/Tests/UnitTests/Core/Material/BinnedSurfaceMaterialAccumulaterTests.cpp +++ b/Tests/UnitTests/Core/Material/BinnedSurfaceMaterialAccumulaterTests.cpp @@ -40,7 +40,7 @@ BOOST_AUTO_TEST_CASE(InvalidSetupTest) { }; for (auto [is, surface] : enumerate(surfaces)) { - surface->assignGeometryId(GeometryIdentifier().setSensitive(is + 1)); + surface->assignGeometryId(GeometryIdentifier().withSensitive(is + 1)); } // First is homogeneous @@ -72,7 +72,7 @@ BOOST_AUTO_TEST_CASE(AccumulationTest) { 100.0)}; for (auto [is, surface] : enumerate(surfaces)) { - surface->assignGeometryId(GeometryIdentifier().setSensitive(is + 1)); + surface->assignGeometryId(GeometryIdentifier().withSensitive(is + 1)); } // Accepted materials are: @@ -169,15 +169,15 @@ BOOST_AUTO_TEST_CASE(AccumulationTest) { BOOST_CHECK_EQUAL(maps.size(), 3u); - auto m0Itr = maps.find(GeometryIdentifier().setSensitive(1)); + auto m0Itr = maps.find(GeometryIdentifier().withSensitive(1)); BOOST_CHECK(m0Itr != maps.end()); BOOST_CHECK(m0Itr->second != nullptr); - auto m1Itr = maps.find(GeometryIdentifier().setSensitive(2)); + auto m1Itr = maps.find(GeometryIdentifier().withSensitive(2)); BOOST_CHECK(m1Itr != maps.end()); BOOST_CHECK(m1Itr->second != nullptr); - auto m2Itr = maps.find(GeometryIdentifier().setSensitive(3)); + auto m2Itr = maps.find(GeometryIdentifier().withSensitive(3)); BOOST_CHECK(m2Itr != maps.end()); BOOST_CHECK(m2Itr->second != nullptr); @@ -203,7 +203,7 @@ BOOST_AUTO_TEST_CASE(AccumulationTest) { // Check failures auto invalidSurface = Surface::makeShared(Transform3::Identity(), 40.0, 100.0); - invalidSurface->assignGeometryId(GeometryIdentifier().setSensitive(4)); + invalidSurface->assignGeometryId(GeometryIdentifier().withSensitive(4)); // Invalid surface amongst material MaterialInteraction mXX; diff --git a/Tests/UnitTests/Core/Material/MaterialInteractionAssignmentTests.cpp b/Tests/UnitTests/Core/Material/MaterialInteractionAssignmentTests.cpp index 98c052ee97c..366f318d0d1 100644 --- a/Tests/UnitTests/Core/Material/MaterialInteractionAssignmentTests.cpp +++ b/Tests/UnitTests/Core/Material/MaterialInteractionAssignmentTests.cpp @@ -38,7 +38,7 @@ BOOST_AUTO_TEST_CASE(AssignToClosest) { 100.0)}; for (auto [is, surface] : enumerate(surfaces)) { - surface->assignGeometryId(GeometryIdentifier().setSensitive(is + 1)); + surface->assignGeometryId(GeometryIdentifier().withSensitive(is + 1)); } std::vector intersectedSurfaces = { @@ -96,7 +96,7 @@ BOOST_AUTO_TEST_CASE(AssignToClosest_withGlobalVeto) { 100.0)}; for (auto [is, surface] : enumerate(surfaces)) { - surface->assignGeometryId(GeometryIdentifier().setSensitive(is + 1)); + surface->assignGeometryId(GeometryIdentifier().withSensitive(is + 1)); } std::vector intersectedSurfaces = { @@ -148,7 +148,7 @@ BOOST_AUTO_TEST_CASE(AssignToClosest_withLocalVeto) { 100.0)}; for (auto [is, surface] : enumerate(surfaces)) { - surface->assignGeometryId(GeometryIdentifier().setSensitive(is + 1)); + surface->assignGeometryId(GeometryIdentifier().withSensitive(is + 1)); } std::vector intersectedSurfaces = { @@ -183,7 +183,8 @@ BOOST_AUTO_TEST_CASE(AssignToClosest_withLocalVeto) { // We assign this to std::vector< std::pair> - localVetoVector = {{GeometryIdentifier().setSensitive(2), VetoThisOne{}}}; + localVetoVector = { + {GeometryIdentifier().withSensitive(2), VetoThisOne{}}}; GeometryHierarchyMap localVetos( localVetoVector); MaterialInteractionAssignment::Options options; @@ -209,7 +210,7 @@ BOOST_AUTO_TEST_CASE(AssignToClosest_withReassignment) { 100.0)}; for (auto [is, surface] : enumerate(surfaces)) { - surface->assignGeometryId(GeometryIdentifier().setSensitive(is + 1)); + surface->assignGeometryId(GeometryIdentifier().withSensitive(is + 1)); } std::vector intersectedSurfaces = { @@ -250,7 +251,7 @@ BOOST_AUTO_TEST_CASE(AssignToClosest_withReassignment) { std::vector> reassignmentVector = { - {GeometryIdentifier().setSensitive(2), ReAssignToNeighbor{}}}; + {GeometryIdentifier().withSensitive(2), ReAssignToNeighbor{}}}; GeometryHierarchyMap reassignments(reassignmentVector); MaterialInteractionAssignment::Options options; @@ -268,14 +269,14 @@ BOOST_AUTO_TEST_CASE(AssignToClosest_withReassignment) { // Check that the geoid with number 2 never shows up for (const auto& mi : assigned) { - BOOST_CHECK_NE(mi.intersectionID, GeometryIdentifier().setSensitive(2)); + BOOST_CHECK_NE(mi.intersectionID, GeometryIdentifier().withSensitive(2)); } } BOOST_AUTO_TEST_CASE(AssignWithPathLength) { auto surface = Surface::makeShared(Transform3::Identity(), 20.0, 100.0); - surface->assignGeometryId(GeometryIdentifier().setSensitive(1)); + surface->assignGeometryId(GeometryIdentifier().withSensitive(1)); Vector3 position = {20., 10., 0.}; Vector3 direction = position.normalized(); diff --git a/Tests/UnitTests/Core/Material/MaterialMapperTests.cpp b/Tests/UnitTests/Core/Material/MaterialMapperTests.cpp index 9fca8bb64b7..c3debc42bce 100644 --- a/Tests/UnitTests/Core/Material/MaterialMapperTests.cpp +++ b/Tests/UnitTests/Core/Material/MaterialMapperTests.cpp @@ -173,7 +173,7 @@ BOOST_AUTO_TEST_CASE(MaterialMapperFlowTest) { 100.0)}; for (auto [is, surface] : enumerate(surfaces)) { - surface->assignGeometryId(GeometryIdentifier().setSensitive(is + 1)); + surface->assignGeometryId(GeometryIdentifier().withSensitive(is + 1)); } // The assigner diff --git a/Tests/UnitTests/Core/Material/PropagatorMaterialAssignerTests.cpp b/Tests/UnitTests/Core/Material/PropagatorMaterialAssignerTests.cpp index 18f5880c98e..9988336634d 100644 --- a/Tests/UnitTests/Core/Material/PropagatorMaterialAssignerTests.cpp +++ b/Tests/UnitTests/Core/Material/PropagatorMaterialAssignerTests.cpp @@ -119,11 +119,11 @@ BOOST_AUTO_TEST_CASE(FindSurfaceIntersectionsTrackingGeometry) { BOOST_AUTO_TEST_CASE(FindSurfaceIntersectionsTrackingVolume) { unsigned int volID = 1; auto assignGeoIds = [&volID](Experimental::DetectorVolume& dVol) -> void { - dVol.assignGeometryId(GeometryIdentifier().setVolume(volID)); + dVol.assignGeometryId(GeometryIdentifier().withVolume(volID)); unsigned int pID = 1; for (auto& p : dVol.portalPtrs()) { p->surface().assignGeometryId( - GeometryIdentifier().setVolume(volID).setBoundary(pID)); + GeometryIdentifier().withVolume(volID).withBoundary(pID)); } volID++; }; @@ -152,7 +152,7 @@ BOOST_AUTO_TEST_CASE(FindSurfaceIntersectionsTrackingVolume) { auto pCylinderSurface = Surface::makeShared(Transform3::Identity(), pCylinder); pCylinderSurface->assignSurfaceMaterial(surfaceMaterial); - pCylinderSurface->assignGeometryId(GeometryIdentifier().setSensitive(1)); + pCylinderSurface->assignGeometryId(GeometryIdentifier().withSensitive(1)); auto layer0 = Experimental::DetectorVolumeFactory::construct( portalGenerator, tContext, "Layer0", nominal, std::move(vCylinderL0), diff --git a/Tests/UnitTests/Core/Propagator/DirectNavigatorTests.cpp b/Tests/UnitTests/Core/Propagator/DirectNavigatorTests.cpp index 55db36b8854..70b6392f713 100644 --- a/Tests/UnitTests/Core/Propagator/DirectNavigatorTests.cpp +++ b/Tests/UnitTests/Core/Propagator/DirectNavigatorTests.cpp @@ -280,8 +280,8 @@ BOOST_AUTO_TEST_CASE(test_direct_navigator_fwd_bwd) { transform.translate(Vector3{0.0_mm, 0.0_mm, i * 100.0_mm}); auto surface = Surface::makeShared(transform, nullptr); surface->assignGeometryId( - Acts::GeometryIdentifier().setVolume(1).setLayer(1).setSensitive(i + - 1)); + Acts::GeometryIdentifier().withVolume(1).withLayer(1).withSensitive(i + + 1)); surfaces.push_back(surface); } diff --git a/Tests/UnitTests/Core/SpacePointFormation/SpacePointBuilderTests.cpp b/Tests/UnitTests/Core/SpacePointFormation/SpacePointBuilderTests.cpp index d54645d6c87..59736786370 100644 --- a/Tests/UnitTests/Core/SpacePointFormation/SpacePointBuilderTests.cpp +++ b/Tests/UnitTests/Core/SpacePointFormation/SpacePointBuilderTests.cpp @@ -107,11 +107,11 @@ const MeasurementResolution resPixel = {MeasurementType::eLoc01, const MeasurementResolution resStrip = {MeasurementType::eLoc01, {100_um, 100_um}}; const MeasurementResolutionMap resolutions = { - {GeometryIdentifier().setVolume(2), resPixel}, - {GeometryIdentifier().setVolume(3).setLayer(2), resStrip}, - {GeometryIdentifier().setVolume(3).setLayer(4), resStrip}, - {GeometryIdentifier().setVolume(3).setLayer(6), resStrip}, - {GeometryIdentifier().setVolume(3).setLayer(8), resStrip}, + {GeometryIdentifier().withVolume(2), resPixel}, + {GeometryIdentifier().withVolume(3).withLayer(2), resStrip}, + {GeometryIdentifier().withVolume(3).withLayer(4), resStrip}, + {GeometryIdentifier().withVolume(3).withLayer(6), resStrip}, + {GeometryIdentifier().withVolume(3).withLayer(8), resStrip}, }; std::default_random_engine rng(42); diff --git a/Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp b/Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp index 22ce0a52c55..4b48c462487 100644 --- a/Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp +++ b/Tests/UnitTests/Core/TrackFinding/CombinatorialKalmanFilterTests.cpp @@ -97,11 +97,11 @@ struct Detector { MeasurementResolution resStrip0 = {MeasurementType::eLoc0, {100_um}}; MeasurementResolution resStrip1 = {MeasurementType::eLoc1, {150_um}}; MeasurementResolutionMap resolutions = { - {Acts::GeometryIdentifier().setVolume(2), resPixel}, - {Acts::GeometryIdentifier().setVolume(3).setLayer(2), resStrip0}, - {Acts::GeometryIdentifier().setVolume(3).setLayer(4), resStrip1}, - {Acts::GeometryIdentifier().setVolume(3).setLayer(6), resStrip0}, - {Acts::GeometryIdentifier().setVolume(3).setLayer(8), resStrip1}, + {Acts::GeometryIdentifier().withVolume(2), resPixel}, + {Acts::GeometryIdentifier().withVolume(3).withLayer(2), resStrip0}, + {Acts::GeometryIdentifier().withVolume(3).withLayer(4), resStrip1}, + {Acts::GeometryIdentifier().withVolume(3).withLayer(6), resStrip0}, + {Acts::GeometryIdentifier().withVolume(3).withLayer(8), resStrip1}, }; Detector(const Acts::GeometryContext& geoCtx) diff --git a/Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp b/Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp index d97369fbfef..9ec88ff9fd2 100644 --- a/Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp +++ b/Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp @@ -648,26 +648,27 @@ BOOST_AUTO_TEST_CASE(SubsetHitCountCut) { }; auto vol7_lay3_sen2 = makeSurface( - GeometryIdentifier{}.setVolume(7).setLayer(3).setSensitive(2)); - auto vol7_lay4 = makeSurface(GeometryIdentifier{}.setVolume(7).setLayer(4)); + GeometryIdentifier{}.withVolume(7).withLayer(3).withSensitive(2)); + auto vol7_lay4 = makeSurface(GeometryIdentifier{}.withVolume(7).withLayer(4)); auto vol7_lay3_sen8 = makeSurface( - GeometryIdentifier{}.setVolume(7).setLayer(3).setSensitive(8)); + GeometryIdentifier{}.withVolume(7).withLayer(3).withSensitive(8)); auto vol7_lay5_sen11 = makeSurface( - GeometryIdentifier{}.setVolume(7).setLayer(5).setSensitive(11)); + GeometryIdentifier{}.withVolume(7).withLayer(5).withSensitive(11)); auto vol7_lay5_sen12 = makeSurface( - GeometryIdentifier{}.setVolume(7).setLayer(5).setSensitive(12)); + GeometryIdentifier{}.withVolume(7).withLayer(5).withSensitive(12)); auto vol7_lay6_sen3 = makeSurface( - GeometryIdentifier{}.setVolume(7).setLayer(6).setSensitive(3)); + GeometryIdentifier{}.withVolume(7).withLayer(6).withSensitive(3)); auto vol8_lay8_sen1 = makeSurface( - GeometryIdentifier{}.setVolume(8).setLayer(8).setSensitive(1)); + GeometryIdentifier{}.withVolume(8).withLayer(8).withSensitive(1)); auto vol8_lay8_sen2 = makeSurface( - GeometryIdentifier{}.setVolume(8).setLayer(8).setSensitive(2)); + GeometryIdentifier{}.withVolume(8).withLayer(8).withSensitive(2)); auto vol8_lay9_sen1 = makeSurface( - GeometryIdentifier{}.setVolume(8).setLayer(9).setSensitive(1)); + GeometryIdentifier{}.withVolume(8).withLayer(9).withSensitive(1)); TrackSelector::Config cfgVol7; - cfgVol7.measurementCounter.addCounter({GeometryIdentifier{}.setVolume(7)}, 3); + cfgVol7.measurementCounter.addCounter({GeometryIdentifier{}.withVolume(7)}, + 3); TrackSelector selectorVol7{cfgVol7}; auto trackVol7 = makeTrack(); @@ -687,7 +688,8 @@ BOOST_AUTO_TEST_CASE(SubsetHitCountCut) { BOOST_CHECK(selectorVol7.isValidTrack(trackVol7)); TrackSelector::Config cfgVol8; - cfgVol8.measurementCounter.addCounter({GeometryIdentifier{}.setVolume(8)}, 2); + cfgVol8.measurementCounter.addCounter({GeometryIdentifier{}.withVolume(8)}, + 2); TrackSelector selectorVol8{cfgVol8}; // Previous trackVol7 has no measurements in volume 8 @@ -705,7 +707,7 @@ BOOST_AUTO_TEST_CASE(SubsetHitCountCut) { TrackSelector::Config cfgVol7Lay5; cfgVol7Lay5.measurementCounter.addCounter( - {GeometryIdentifier{}.setVolume(7).setLayer(5)}, 2); + {GeometryIdentifier{}.withVolume(7).withLayer(5)}, 2); TrackSelector selectorVol7Lay5{cfgVol7Lay5}; // Only one hit on volume 7 layer 5 @@ -716,7 +718,7 @@ BOOST_AUTO_TEST_CASE(SubsetHitCountCut) { // Check requirement on volume 7 OR 8 TrackSelector::Config cfgVol7Or8; cfgVol7Or8.measurementCounter.addCounter( - {GeometryIdentifier{}.setVolume(7), GeometryIdentifier{}.setVolume(8)}, + {GeometryIdentifier{}.withVolume(7), GeometryIdentifier{}.withVolume(8)}, 4); TrackSelector selectorVol7Or8{cfgVol7Or8}; @@ -732,10 +734,10 @@ BOOST_AUTO_TEST_CASE(SubsetHitCountCut) { BOOST_CHECK(selectorVol7Or8.isValidTrack(trackVol8)); TrackSelector::Config cfgVol7And8; - cfgVol7And8.measurementCounter.addCounter({GeometryIdentifier{}.setVolume(7)}, - 4); - cfgVol7And8.measurementCounter.addCounter({GeometryIdentifier{}.setVolume(8)}, - 2); + cfgVol7And8.measurementCounter.addCounter( + {GeometryIdentifier{}.withVolume(7)}, 4); + cfgVol7And8.measurementCounter.addCounter( + {GeometryIdentifier{}.withVolume(8)}, 2); TrackSelector selectorVol7And8{cfgVol7And8}; // this track has enough hits in vol 7 but not enough in vol 8 diff --git a/Tests/UnitTests/Core/TrackFitting/FitterTestsCommon.hpp b/Tests/UnitTests/Core/TrackFitting/FitterTestsCommon.hpp index b2eed053510..7769a1089f8 100644 --- a/Tests/UnitTests/Core/TrackFitting/FitterTestsCommon.hpp +++ b/Tests/UnitTests/Core/TrackFitting/FitterTestsCommon.hpp @@ -144,11 +144,11 @@ struct FitterTester { MeasurementResolution resStrip0 = {MeasurementType::eLoc0, {100_um}}; MeasurementResolution resStrip1 = {MeasurementType::eLoc1, {150_um}}; MeasurementResolutionMap resolutions = { - {Acts::GeometryIdentifier().setVolume(2), resPixel}, - {Acts::GeometryIdentifier().setVolume(3).setLayer(2), resStrip0}, - {Acts::GeometryIdentifier().setVolume(3).setLayer(4), resStrip1}, - {Acts::GeometryIdentifier().setVolume(3).setLayer(6), resStrip0}, - {Acts::GeometryIdentifier().setVolume(3).setLayer(8), resStrip1}, + {Acts::GeometryIdentifier().withVolume(2), resPixel}, + {Acts::GeometryIdentifier().withVolume(3).withLayer(2), resStrip0}, + {Acts::GeometryIdentifier().withVolume(3).withLayer(4), resStrip1}, + {Acts::GeometryIdentifier().withVolume(3).withLayer(6), resStrip0}, + {Acts::GeometryIdentifier().withVolume(3).withLayer(8), resStrip1}, }; // simulation propagator diff --git a/Tests/UnitTests/Core/TrackFitting/Gx2fTests.cpp b/Tests/UnitTests/Core/TrackFitting/Gx2fTests.cpp index 343190311a3..985ab16b618 100644 --- a/Tests/UnitTests/Core/TrackFitting/Gx2fTests.cpp +++ b/Tests/UnitTests/Core/TrackFitting/Gx2fTests.cpp @@ -228,7 +228,7 @@ const MeasurementResolution resPixel = {MeasurementType::eLoc01, const MeasurementResolution resStrip0 = {MeasurementType::eLoc0, {25_um}}; const MeasurementResolution resStrip1 = {MeasurementType::eLoc1, {50_um}}; const MeasurementResolutionMap resMapAllPixel = { - {Acts::GeometryIdentifier().setVolume(0), resPixel}}; + {Acts::GeometryIdentifier().withVolume(0), resPixel}}; // This test checks if the call to the fitter works and no errors occur in the // framework, without fitting and updating any parameters @@ -429,13 +429,13 @@ BOOST_AUTO_TEST_CASE(MixedDetector) { ACTS_DEBUG("Create the measurements"); const MeasurementResolutionMap resMap = { - {Acts::GeometryIdentifier().setVolume(2).setLayer(2), resPixel}, - {Acts::GeometryIdentifier().setVolume(2).setLayer(4), resStrip0}, - {Acts::GeometryIdentifier().setVolume(2).setLayer(6), resStrip1}, - {Acts::GeometryIdentifier().setVolume(2).setLayer(8), resPixel}, - {Acts::GeometryIdentifier().setVolume(2).setLayer(10), resStrip0}, - {Acts::GeometryIdentifier().setVolume(2).setLayer(12), resStrip1}, - {Acts::GeometryIdentifier().setVolume(2).setLayer(14), resPixel}, + {Acts::GeometryIdentifier().withVolume(2).withLayer(2), resPixel}, + {Acts::GeometryIdentifier().withVolume(2).withLayer(4), resStrip0}, + {Acts::GeometryIdentifier().withVolume(2).withLayer(6), resStrip1}, + {Acts::GeometryIdentifier().withVolume(2).withLayer(8), resPixel}, + {Acts::GeometryIdentifier().withVolume(2).withLayer(10), resStrip0}, + {Acts::GeometryIdentifier().withVolume(2).withLayer(12), resStrip1}, + {Acts::GeometryIdentifier().withVolume(2).withLayer(14), resPixel}, }; using SimPropagator = diff --git a/Tests/UnitTests/Examples/Io/Json/JsonDigitizationConfigTests.cpp b/Tests/UnitTests/Examples/Io/Json/JsonDigitizationConfigTests.cpp index dfa8978f17c..66f13d5a540 100644 --- a/Tests/UnitTests/Examples/Io/Json/JsonDigitizationConfigTests.cpp +++ b/Tests/UnitTests/Examples/Io/Json/JsonDigitizationConfigTests.cpp @@ -46,7 +46,7 @@ struct Fixture { Fixture(std::uint64_t rngSeed, std::shared_ptr surf) : rng(rngSeed), - gid(Acts::GeometryIdentifier().setVolume(1).setLayer(2).setSensitive( + gid(Acts::GeometryIdentifier().withVolume(1).withLayer(2).withSensitive( 3)), pid(ActsFatras::Barcode().setVertexPrimary(12).setParticle(23)), surface(std::move(surf)) { diff --git a/Tests/UnitTests/Fatras/Digitization/UncorrelatedHitSmearerTests.cpp b/Tests/UnitTests/Fatras/Digitization/UncorrelatedHitSmearerTests.cpp index 2a4ad0b2a6e..2726b8b7506 100644 --- a/Tests/UnitTests/Fatras/Digitization/UncorrelatedHitSmearerTests.cpp +++ b/Tests/UnitTests/Fatras/Digitization/UncorrelatedHitSmearerTests.cpp @@ -85,7 +85,7 @@ struct Fixture { Fixture(std::uint64_t rngSeed, std::shared_ptr surf) : rng(rngSeed), - gid(Acts::GeometryIdentifier().setVolume(1).setLayer(2).setSensitive( + gid(Acts::GeometryIdentifier().withVolume(1).withLayer(2).withSensitive( 3)), pid(ActsFatras::Barcode().setVertexPrimary(12).setParticle(23)), surface(std::move(surf)) { diff --git a/Tests/UnitTests/Fatras/EventData/HitTests.cpp b/Tests/UnitTests/Fatras/EventData/HitTests.cpp index 4fc89f8b686..441c943b1cf 100644 --- a/Tests/UnitTests/Fatras/EventData/HitTests.cpp +++ b/Tests/UnitTests/Fatras/EventData/HitTests.cpp @@ -21,7 +21,7 @@ namespace { constexpr auto eps = std::numeric_limits::epsilon(); const auto pid = Barcode().setVertexPrimary(12).setParticle(23); const auto gid = - Acts::GeometryIdentifier().setVolume(1).setLayer(2).setSensitive(3); + Acts::GeometryIdentifier().withVolume(1).withLayer(2).withSensitive(3); } // namespace BOOST_AUTO_TEST_SUITE(FatrasHit) diff --git a/Tests/UnitTests/Plugins/Detray/DetrayGeometryConverterTests.cpp b/Tests/UnitTests/Plugins/Detray/DetrayGeometryConverterTests.cpp index 5a45297faf0..d01f9ed43cd 100644 --- a/Tests/UnitTests/Plugins/Detray/DetrayGeometryConverterTests.cpp +++ b/Tests/UnitTests/Plugins/Detray/DetrayGeometryConverterTests.cpp @@ -83,7 +83,7 @@ BOOST_AUTO_TEST_CASE(DetraySurfaceConversion) { auto cylinderSurface = Acts::Surface::makeShared( Transform3::Identity(), std::make_shared(20., 100.)); - auto sgID = Acts::GeometryIdentifier().setSensitive(1); + auto sgID = Acts::GeometryIdentifier().withSensitive(1); cylinderSurface->assignGeometryId(sgID); detray::io::surface_payload payload = DetrayGeometryConverter::convertSurface( diff --git a/Tests/UnitTests/Plugins/Json/GeometryHierarchyMapJsonConverterTests.cpp b/Tests/UnitTests/Plugins/Json/GeometryHierarchyMapJsonConverterTests.cpp index 238cd3b96cc..b71612d8629 100644 --- a/Tests/UnitTests/Plugins/Json/GeometryHierarchyMapJsonConverterTests.cpp +++ b/Tests/UnitTests/Plugins/Json/GeometryHierarchyMapJsonConverterTests.cpp @@ -29,7 +29,7 @@ using nlohmann::json; // helper function to create geometry ids. GeometryIdentifier makeId(int volume = 0, int layer = 0, int sensitive = 0) { - return GeometryIdentifier().setVolume(volume).setLayer(layer).setSensitive( + return GeometryIdentifier().withVolume(volume).withLayer(layer).withSensitive( sensitive); } From 12508b19f513904dd69d9c7cfe0d625fbb7a41a6 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 18 Feb 2025 15:05:46 +0100 Subject: [PATCH 05/14] ci: Add LCG 107a builds (#4091) --- .gitlab-ci.yml | 18 +++++++++++++++--- CMakePresets.json | 11 ++++++++++- Plugins/Json/src/DetrayJsonHelper.cpp | 19 ++++++++++--------- .../MagneticField/MultiRangeBFieldTests.cpp | 2 +- 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 3d19e27c5de..4acc3968919 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -459,10 +459,8 @@ linux_ubuntu_2204_clang: script: - > cmake -B build -S src - --preset=gitlab-ci + --preset=gitlab-ci-lcg -DCMAKE_INSTALL_PREFIX="${INSTALL_DIR}" - -DPython_EXECUTABLE=$(which python3) - -DACTS_BUILD_PLUGIN_GEOMODEL=OFF # GeoModel is not in LCG at this point - ccache -z - cmake --build build -- -j6 @@ -497,3 +495,17 @@ lcg_106a: - gcc13 - gcc14 - clang16 + +lcg_107: + extends: .lcg_base_job + + variables: + LCG_VERSION: "107" + + parallel: + matrix: + - OS: [alma9] + COMPILER: + - gcc13 + - gcc14 + - clang19 diff --git a/CMakePresets.json b/CMakePresets.json index b8226b30911..436531121ca 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -93,7 +93,7 @@ }, { "name": "gitlab-ci-clangtidy", - "displayName": "GitLab-CI", + "displayName": "GitLab-CI clang-tidy", "inherits": "ci-common", "cacheVariables": { "ACTS_BUILD_ODD": "OFF", @@ -101,6 +101,15 @@ "ACTS_RUN_CLANG_TIDY": "ON" } }, + { + "name": "gitlab-ci-lcg", + "displayName": "GitLab-CI LCG", + "inherits": "ci-common", + "cacheVariables": { + "ACTS_USE_SYSTEM_NLOHMANN_JSON": "ON", + "ACTS_BUILD_PLUGIN_GEOMODEL": "OFF" + } + }, { "name": "gitlab-ci-exatrkx", "displayName": "GitLab-CI", diff --git a/Plugins/Json/src/DetrayJsonHelper.cpp b/Plugins/Json/src/DetrayJsonHelper.cpp index 9accb3d9f2b..fb3ede2ade1 100644 --- a/Plugins/Json/src/DetrayJsonHelper.cpp +++ b/Plugins/Json/src/DetrayJsonHelper.cpp @@ -19,14 +19,14 @@ namespace Acts::DetrayJsonHelper { std::tuple> maskFromBounds( const Acts::SurfaceBounds& sBounds, bool portal) { - auto bType = sBounds.type(); - auto bValues = sBounds.values(); + SurfaceBounds::BoundsType bType = sBounds.type(); + std::vector bValues = sBounds.values(); // Return value unsigned int type = 13u; std::vector boundaries = bValues; // Special treatment for some portals if (portal && bType == SurfaceBounds::BoundsType::eCylinder) { - boundaries = {bValues[0u], -bValues[1u], bValues[1u]}; + boundaries = {bValues.at(0u), -bValues.at(1u), bValues.at(1u)}; type = 4u; } else { switch (bType) { @@ -37,20 +37,21 @@ std::tuple> maskFromBounds( type = 5u; // ACTS: eMinX = 0, eMinY = 1, eMaxX = 2, eMaxY = 3, // detray: e_half_x, e_half_y - boundaries = {0.5 * (bValues[2] - bValues[0]), - 0.5 * (bValues[3] - bValues[1])}; + boundaries = std::vector{0.5 * (bValues.at(2) - bValues.at(0)), + 0.5 * (bValues.at(3) - bValues.at(1))}; } break; case SurfaceBounds::BoundsType::eCylinder: { - boundaries = {bValues[0u], -bValues[1u], bValues[1u]}; + boundaries = + std::vector{bValues.at(0u), -bValues.at(1u), bValues.at(1u)}; type = 2u; } break; case SurfaceBounds::BoundsType::eTrapezoid: { type = 7u; - boundaries = {bValues[0u], bValues[1u], bValues[2u], - 1 / (2 * bValues[2u])}; + boundaries = std::vector{bValues.at(0u), bValues.at(1u), bValues.at(2u), + 1 / (2 * bValues.at(2u))}; } break; case SurfaceBounds::BoundsType::eDisc: { - boundaries = {bValues[0u], bValues[1u]}; + boundaries = std::vector{bValues[0u], bValues[1u]}; type = 6u; } break; default: diff --git a/Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp b/Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp index 3644db058b4..a59dbcc2eb6 100644 --- a/Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp +++ b/Tests/UnitTests/Core/MagneticField/MultiRangeBFieldTests.cpp @@ -6,7 +6,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -#include +#include #include #include "Acts/MagneticField/MagneticFieldContext.hpp" From cc3b0a41e9fa119bf386654fc0c287acc028332b Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 18 Feb 2025 17:22:58 +0100 Subject: [PATCH 06/14] refactor: GeometryIdentifier checks value range (#4094) I've run into overflowing ID values one too many times now. This PR proposes making the value setters check the value to be assigned to be within the valid range. I'm currently throwing an exception, which we can debate if that's the best way to handle failure. --- .../Acts/Geometry/GeometryIdentifier.hpp | 11 ++++++++- .../Core/Geometry/GeometryIdentifierTests.cpp | 24 +++++++++---------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/Core/include/Acts/Geometry/GeometryIdentifier.hpp b/Core/include/Acts/Geometry/GeometryIdentifier.hpp index e6222422eb3..9029b4c4216 100644 --- a/Core/include/Acts/Geometry/GeometryIdentifier.hpp +++ b/Core/include/Acts/Geometry/GeometryIdentifier.hpp @@ -171,7 +171,6 @@ class GeometryIdentifier { static constexpr Value kLayerMask = 0x0000fff000000000; /// (2^8)-1 = 255 approach surfaces static constexpr Value kApproachMask = 0x0000000ff0000000; - static constexpr Value kPassiveMask = kApproachMask; /// (2^20)-1 = 1048575 sensitive surfaces static constexpr Value kSensitiveMask = 0x000000000fffff00; /// (2^8)-1 = 255 extra values @@ -190,12 +189,22 @@ class GeometryIdentifier { return __builtin_ctzll(mask); } + constexpr static Value getMaxValue(Value mask) { + return mask >> extractShift(mask); + } + /// Extract the masked bits from the encoded value. constexpr Value getBits(Value mask) const { return (m_value & mask) >> extractShift(mask); } /// Set the masked bits to id in the encoded value. constexpr GeometryIdentifier& setBits(Value mask, Value id) { + if (id > getMaxValue(mask)) { + throw std::invalid_argument( + "Value " + std::to_string(id) + " exceeds maximum value " + + std::to_string(getMaxValue(mask)) + " for this field"); + } + m_value = (m_value & ~mask) | ((id << extractShift(mask)) & mask); // return *this here that we need to write fewer lines in the setXXX // methods diff --git a/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp b/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp index ffea49752f6..6d9a04922bc 100644 --- a/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp +++ b/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp @@ -45,18 +45,18 @@ BOOST_AUTO_TEST_CASE(GeometryIdentifier_max_values) { constexpr GeometryIdentifier ref = 0xdeadaffe01234567; // values above the maximum are truncated // max+1 has all available bits zeroed - BOOST_CHECK_EQUAL(GeometryIdentifier(ref).setVolume(volumeMax + 1), - GeometryIdentifier(ref).setVolume(0u)); - BOOST_CHECK_EQUAL(GeometryIdentifier(ref).setBoundary(boundaryMax + 1), - GeometryIdentifier(ref).setBoundary(0u)); - BOOST_CHECK_EQUAL(GeometryIdentifier(ref).setLayer(layerMax + 1), - GeometryIdentifier(ref).setLayer(0u)); - BOOST_CHECK_EQUAL(GeometryIdentifier(ref).setApproach(approachMax + 1), - GeometryIdentifier(ref).setApproach(0u)); - BOOST_CHECK_EQUAL(GeometryIdentifier(ref).setSensitive(sensitiveMax + 1), - GeometryIdentifier(ref).setSensitive(0u)); - BOOST_CHECK_EQUAL(GeometryIdentifier(ref).setExtra(extraMax + 1), - GeometryIdentifier(ref).setExtra(0u)); + BOOST_CHECK_THROW(GeometryIdentifier(ref).setVolume(volumeMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).setBoundary(boundaryMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).setLayer(layerMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).setApproach(approachMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).setSensitive(sensitiveMax + 1), + std::invalid_argument); + BOOST_CHECK_THROW(GeometryIdentifier(ref).setExtra(extraMax + 1), + std::invalid_argument); } BOOST_AUTO_TEST_CASE(GeometryIdentifier_order) { From 06447c300f962b8e7f2e6867b3ff1a258dd6f14e Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 18 Feb 2025 20:50:58 +0100 Subject: [PATCH 07/14] feat: Generic TrackingGeometry visitor + closure (#4089) This PR introduces a new visitor interface for `TrackingGeometry`, with the intention of medium-term replacing the `visitSurfaces` and `visitVolumes` API. It also reimplements the Gen1 geometry closure using such visitor. In the future, my plan is to remove the original Gen1 constructor with explicit `IMaterialDecorator` and `GeometryIdentifierHook`, and instead have the blueprint geometry construction handle this through such a visitor. In the meanwhile, I added such a visitor to the blueprint construction, but that currently only sets basic geometry identification. This will be complemented by tree-based identifiers for layers etc. This is decoupled to ensure there are no output changes from this PR, and indeed it should not be API breaking. Blocked by: - https://github.com/acts-project/acts/pull/4085 --- Core/include/Acts/Geometry/Blueprint.hpp | 5 - Core/include/Acts/Geometry/Layer.hpp | 1 + .../Acts/Geometry/TrackingGeometry.hpp | 15 +- .../Acts/Geometry/TrackingGeometryVisitor.hpp | 95 +++++++ Core/include/Acts/Geometry/TrackingVolume.hpp | 98 ++----- Core/src/Geometry/Blueprint.cpp | 59 ++++- Core/src/Geometry/CMakeLists.txt | 1 + Core/src/Geometry/TrackingGeometry.cpp | 201 +++++++++++--- Core/src/Geometry/TrackingGeometryBuilder.cpp | 1 + Core/src/Geometry/TrackingGeometryVisitor.cpp | 58 ++++ Core/src/Geometry/TrackingVolume.cpp | 248 +++++++++--------- Examples/Python/src/Blueprint.cpp | 1 - Examples/Python/src/Geometry.cpp | 50 +++- Examples/Python/tests/test_examples.py | 30 +++ 14 files changed, 627 insertions(+), 236 deletions(-) create mode 100644 Core/include/Acts/Geometry/TrackingGeometryVisitor.hpp create mode 100644 Core/src/Geometry/TrackingGeometryVisitor.cpp diff --git a/Core/include/Acts/Geometry/Blueprint.hpp b/Core/include/Acts/Geometry/Blueprint.hpp index db96654218e..4f41f1fbcf1 100644 --- a/Core/include/Acts/Geometry/Blueprint.hpp +++ b/Core/include/Acts/Geometry/Blueprint.hpp @@ -56,11 +56,6 @@ class Blueprint : public BlueprintNode { /// Determine how much envelope space to produce from the highest volume /// in the geometry hierarchy and the world volume. ExtentEnvelope envelope = ExtentEnvelope::Zero(); - - /// The geometry identifier hook, passed through the `TrackingGeometry` - /// constructor. This will be superseded by a more integrated approach to - /// the identification scheme - GeometryIdentifierHook geometryIdentifierHook = {}; }; /// Constructor from a config object diff --git a/Core/include/Acts/Geometry/Layer.hpp b/Core/include/Acts/Geometry/Layer.hpp index 15b1b98c621..e975fe3cf4b 100644 --- a/Core/include/Acts/Geometry/Layer.hpp +++ b/Core/include/Acts/Geometry/Layer.hpp @@ -87,6 +87,7 @@ class Layer : public virtual GeometryObject { /// Declare the TrackingVolume as a friend, to be able to register previous, /// next and set the enclosing TrackingVolume friend class TrackingVolume; + friend class Gen1GeometryClosureVisitor; public: /// Default Constructor - deleted diff --git a/Core/include/Acts/Geometry/TrackingGeometry.hpp b/Core/include/Acts/Geometry/TrackingGeometry.hpp index e8d48c5ae5e..46cd3ceb766 100644 --- a/Core/include/Acts/Geometry/TrackingGeometry.hpp +++ b/Core/include/Acts/Geometry/TrackingGeometry.hpp @@ -27,6 +27,11 @@ class Surface; class PerigeeSurface; class IMaterialDecorator; class TrackingVolume; +class TrackingGeometryVisitor; +class TrackingGeometryMutableVisitor; + +// Forward declaration only, the implementation is hidden in the .cpp file. +class Gen1GeometryClosureVisitor; /// @class TrackingGeometry /// @@ -48,10 +53,11 @@ class TrackingGeometry { /// surface or volume based material to the TrackingVolume /// @param hook Identifier hook to be applied to surfaces /// @param logger instance of a logger (defaulting to the "silent" one) + /// @param close If true, run the Gen1 geometry closure TrackingGeometry(const std::shared_ptr& highestVolume, const IMaterialDecorator* materialDecorator = nullptr, const GeometryIdentifierHook& hook = {}, - const Logger& logger = getDummyLogger()); + const Logger& logger = getDummyLogger(), bool close = true); /// Destructor ~TrackingGeometry(); @@ -60,6 +66,10 @@ class TrackingGeometry { /// @return plain pointer to the world volume const TrackingVolume* highestTrackingVolume() const; + /// Access to the world volume + /// @return plain pointer to the world volume + TrackingVolume* highestTrackingVolume(); + /// Access to the world volume /// @return shared pointer to the world volume std::shared_ptr highestTrackingVolumePtr() const; @@ -127,6 +137,9 @@ class TrackingGeometry { std::forward(visitor)); } + void apply(TrackingGeometryVisitor& visitor) const; + void apply(TrackingGeometryMutableVisitor& visitor); + /// Search for a volume with the given identifier. /// /// @param id is the geometry identifier of the volume diff --git a/Core/include/Acts/Geometry/TrackingGeometryVisitor.hpp b/Core/include/Acts/Geometry/TrackingGeometryVisitor.hpp new file mode 100644 index 00000000000..59c70b5b883 --- /dev/null +++ b/Core/include/Acts/Geometry/TrackingGeometryVisitor.hpp @@ -0,0 +1,95 @@ +// This file is part of the ACTS project. +// +// Copyright (C) 2016 CERN for the benefit of the ACTS project +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +#pragma once + +namespace Acts { + +class TrackingVolume; +class Portal; +class Surface; +class Layer; + +template +class BoundarySurfaceT; + +/// @brief Visitor interface for traversing the tracking geometry hierarchy +/// +/// This visitor allows for const access to traverse and inspect the tracking +/// geometry components without modifying them. It's used for operations like +/// visualization, validation, or collecting information about the geometry +/// structure. +class TrackingGeometryVisitor { + public: + virtual ~TrackingGeometryVisitor() = 0; + + /// @brief Visit a tracking volume in the geometry + /// @param volume The tracking volume being visited + /// @note Called for each volume in the geometry hierarchy during traversal + virtual void visitVolume(const TrackingVolume& volume); + + /// @brief Visit a portal (boundary between volumes) + /// @param portal The portal being visited + /// @note Called for each portal encountered during geometry traversal + virtual void visitPortal(const Portal& portal); + + /// @brief Visit a surface in the geometry + /// @param surface The surface being visited + /// @note Called for each surface encountered during geometry traversal + virtual void visitSurface(const Surface& surface); + + // Gen 1 + /// @brief Visit a detector layer + /// @param layer The layer being visited + /// @note Called for each layer encountered during geometry traversal + virtual void visitLayer(const Layer& layer); + + /// @brief Visit a boundary surface between tracking volumes + /// @param boundary The boundary surface being visited + /// @note Called for each boundary surface encountered during geometry traversal + virtual void visitBoundarySurface( + const BoundarySurfaceT& boundary); +}; + +/// @brief Mutable visitor interface for modifying the tracking geometry hierarchy +/// +/// This visitor allows for non-const access to traverse and modify the tracking +/// geometry components. It's used for operations like geometry construction, +/// material decoration, or geometry ID assignment. +class TrackingGeometryMutableVisitor { + public: + virtual ~TrackingGeometryMutableVisitor(); + + /// @brief Visit and potentially modify a tracking volume + /// @param volume The tracking volume being visited + /// @note Called for each volume in the geometry hierarchy during traversal + virtual void visitVolume(TrackingVolume& volume); + + /// @brief Visit and potentially modify a portal + /// @param portal The portal being visited + /// @note Called for each portal encountered during geometry traversal + virtual void visitPortal(Portal& portal); + + /// @brief Visit and potentially modify a surface + /// @param surface The surface being visited + /// @note Called for each surface encountered during geometry traversal + virtual void visitSurface(Surface& surface); + + // Gen 1 + /// @brief Visit and potentially modify a detector layer + /// @param layer The layer being visited + /// @note Called for each layer encountered during geometry traversal + virtual void visitLayer(Layer& layer); + + /// @brief Visit and potentially modify a boundary surface + /// @param boundary The boundary surface being visited + /// @note Called for each boundary surface encountered during geometry traversal + virtual void visitBoundarySurface(BoundarySurfaceT& boundary); +}; + +} // namespace Acts diff --git a/Core/include/Acts/Geometry/TrackingVolume.hpp b/Core/include/Acts/Geometry/TrackingVolume.hpp index 664faa39d46..b8f96fe8773 100644 --- a/Core/include/Acts/Geometry/TrackingVolume.hpp +++ b/Core/include/Acts/Geometry/TrackingVolume.hpp @@ -15,6 +15,7 @@ #include "Acts/Geometry/GeometryIdentifier.hpp" #include "Acts/Geometry/Layer.hpp" #include "Acts/Geometry/Portal.hpp" +#include "Acts/Geometry/TrackingGeometryVisitor.hpp" #include "Acts/Geometry/TrackingVolumeVisitorConcept.hpp" #include "Acts/Geometry/Volume.hpp" #include "Acts/Material/IVolumeMaterial.hpp" @@ -177,56 +178,24 @@ class TrackingVolume : public Volume { /// this, e.g. as a private member template void visitSurfaces(visitor_t&& visitor, bool restrictToSensitives) const { - if (!restrictToSensitives) { - // Visit the boundary surfaces - for (const auto& bs : m_boundarySurfaces) { - visitor(&(bs->surfaceRepresentation())); - } + struct Visitor : public TrackingGeometryVisitor { + std::remove_cv_t>* m_visitor{}; + bool m_restrictToSensitives{}; - for (const auto& portal : portals()) { - visitor(&portal.surface()); - } - } - - // Internal structure - if (m_confinedVolumes == nullptr) { - // no sub volumes => loop over the confined layers - if (m_confinedLayers != nullptr) { - for (const auto& layer : m_confinedLayers->arrayObjects()) { - // Surfaces contained in the surface array - if (layer->surfaceArray() != nullptr) { - for (const auto& srf : layer->surfaceArray()->surfaces()) { - visitor(srf); - continue; - } - } - if (!restrictToSensitives) { - // Surfaces of the layer - visitor(&layer->surfaceRepresentation()); - // Approach surfaces of the layer - if (layer->approachDescriptor() != nullptr) { - for (const auto& srf : - layer->approachDescriptor()->containedSurfaces()) { - visitor(srf); - } - } - } + void visitSurface(const Surface& surface) override { + if (m_restrictToSensitives && surface.geometryId().sensitive() == 0) { + return; } + assert(m_visitor != nullptr); + (*m_visitor)(&surface); } - } else { - // contains sub volumes - for (const auto& volume : m_confinedVolumes->arrayObjects()) { - volume->visitSurfaces(visitor, restrictToSensitives); - } - } + }; - for (const auto& surface : surfaces()) { - visitor(&surface); - } + Visitor internal; + internal.m_visitor = &visitor; + internal.m_restrictToSensitives = restrictToSensitives; - for (const auto& volume : volumes()) { - volume.visitSurfaces(visitor, restrictToSensitives); - } + apply(internal); } /// @brief Visit all sensitive surfaces @@ -254,19 +223,24 @@ class TrackingVolume : public Volume { /// this, e.g. as a private member template void visitVolumes(visitor_t&& visitor) const { - visitor(this); - if (m_confinedVolumes != nullptr) { - // contains sub volumes - for (const auto& volume : m_confinedVolumes->arrayObjects()) { - volume->visitVolumes(visitor); + struct Visitor : public TrackingGeometryVisitor { + std::remove_cv_t>* m_visitor{}; + bool m_restrictToSensitives{}; + + void visitVolume(const TrackingVolume& volume) override { + (*m_visitor)(&volume); } - } + }; + + Visitor internal; + internal.m_visitor = &visitor; - for (const auto& volume : m_volumes) { - volume->visitVolumes(visitor); - } + apply(internal); } + void apply(TrackingGeometryVisitor& visitor) const; + void apply(TrackingGeometryMutableVisitor& visitor); + /// Returns the VolumeName - for debug reason, might be depreciated later const std::string& volumeName() const; @@ -547,22 +521,6 @@ class TrackingVolume : public Volume { /// @} private: - /// close the Geometry, i.e. set the GeometryIdentifier and assign material - /// - /// @param materialDecorator is a dedicated decorator for the - /// material to be assigned (surface, volume based) - /// @param volumeMap is a map to find the a volume by identifier - /// @param vol is the geometry id of the volume - /// as calculated by the TrackingGeometry - /// @param hook Identifier hook to be applied to surfaces - /// @param logger A @c Logger instance - /// - void closeGeometry( - const IMaterialDecorator* materialDecorator, - std::unordered_map& volumeMap, - std::size_t& vol, const GeometryIdentifierHook& hook, - const Logger& logger = getDummyLogger()); - /// The volume based material the TrackingVolume consists of std::shared_ptr m_volumeMaterial{nullptr}; diff --git a/Core/src/Geometry/Blueprint.cpp b/Core/src/Geometry/Blueprint.cpp index a35de4a58f0..b7427feecfe 100644 --- a/Core/src/Geometry/Blueprint.cpp +++ b/Core/src/Geometry/Blueprint.cpp @@ -11,6 +11,7 @@ #include "Acts/Geometry/CuboidVolumeBounds.hpp" #include "Acts/Geometry/CylinderVolumeBounds.hpp" #include "Acts/Geometry/Extent.hpp" +#include "Acts/Geometry/GeometryIdentifier.hpp" #include "Acts/Geometry/PortalShell.hpp" #include "Acts/Geometry/VolumeBounds.hpp" #include "Acts/Navigation/INavigationPolicy.hpp" @@ -150,8 +151,64 @@ std::unique_ptr Blueprint::construct( names.insert(volume->volumeName()); }); + // @TODO: Refactor this to ignore already set IDs from inside the tree! + class Visitor : public TrackingGeometryMutableVisitor { + public: + explicit Visitor(const Logger &logger) : m_logger(&logger) { + ACTS_VERBOSE("Creating Gen3 geometry closure visitor"); + } + + const Logger &logger() const { return *m_logger; } + + void visitVolume(TrackingVolume &volume) override { + ACTS_VERBOSE("Volume: " << volume.volumeName()); + + // Increment the volume ID for this volume + m_volumeID = GeometryIdentifier().setVolume(m_volumeID.volume() + 1); + // Reset portal id for this volume + m_iportal = 0; + // Reset sensitive id for this volume + m_isensitive = 0; + + // assign the Volume ID to the volume itself + volume.assignGeometryId(m_volumeID); + ACTS_VERBOSE("~> Volume ID: " << m_volumeID); + } + + void visitPortal(Portal &portal) override { + // Increment the portal ID for this portal + m_iportal += 1; + // create the portal ID + auto portalID = GeometryIdentifier(m_volumeID).setBoundary(m_iportal); + ACTS_VERBOSE("~> Portal ID: " << portalID); + + portal.surface().assignGeometryId(portalID); + } + + void visitSurface(Surface &surface) override { + if (surface.geometryId() == GeometryIdentifier{}) { + // This surface has not been processed yet, assign volume ID + + m_isensitive += 1; + auto surfaceID = + GeometryIdentifier(m_volumeID).setSensitive(m_isensitive); + ACTS_VERBOSE("~> Surface ID: " << surfaceID); + + surface.assignGeometryId(surfaceID); + } + } + + const Logger *m_logger{nullptr}; + GeometryIdentifier m_volumeID; + GeometryIdentifier::Value m_iportal = 0; + GeometryIdentifier::Value m_isensitive = 0; + }; + + Visitor closureVisitor{logger}; + world->apply(closureVisitor); + return std::make_unique( - std::move(world), nullptr, m_cfg.geometryIdentifierHook, logger); + std::move(world), nullptr, GeometryIdentifierHook{}, logger, false); } } // namespace Acts diff --git a/Core/src/Geometry/CMakeLists.txt b/Core/src/Geometry/CMakeLists.txt index 2ed76752198..d138817f547 100644 --- a/Core/src/Geometry/CMakeLists.txt +++ b/Core/src/Geometry/CMakeLists.txt @@ -53,4 +53,5 @@ target_sources( MaterialDesignatorBlueprintNode.cpp VolumeAttachmentStrategy.cpp VolumeResizeStrategy.cpp + TrackingGeometryVisitor.cpp ) diff --git a/Core/src/Geometry/TrackingGeometry.cpp b/Core/src/Geometry/TrackingGeometry.cpp index d54a31dce04..a5dc71ef0a5 100644 --- a/Core/src/Geometry/TrackingGeometry.cpp +++ b/Core/src/Geometry/TrackingGeometry.cpp @@ -10,49 +10,180 @@ #include "Acts/Definitions/Tolerance.hpp" #include "Acts/Geometry/GeometryIdentifier.hpp" +#include "Acts/Geometry/TrackingGeometryVisitor.hpp" #include "Acts/Geometry/TrackingVolume.hpp" +#include "Acts/Material/ProtoVolumeMaterial.hpp" #include "Acts/Surfaces/Surface.hpp" #include -Acts::TrackingGeometry::TrackingGeometry( +namespace Acts { + +class Gen1GeometryClosureVisitor : public TrackingGeometryMutableVisitor { + public: + Gen1GeometryClosureVisitor(const Logger& logger, + const IMaterialDecorator* materialDecorator, + const GeometryIdentifierHook& hook) + : m_logger(&logger), + m_materialDecorator(materialDecorator), + m_hook(&hook) { + ACTS_VERBOSE("Creating Gen1GeometryClosureVisitor"); + } + + const Logger& logger() const { return *m_logger; } + + void visitVolume(TrackingVolume& volume) override { + ACTS_DEBUG("Volume: " << volume.volumeName()); + + // Increment the volume ID for this volume + m_volumeID = GeometryIdentifier().setVolume(m_volumeID.volume() + 1); + // Reset boundary id for this volume + m_iboundary = 0; + // Reset layer id for this volume + m_ilayer = 0; + + // assign the Volume ID to the volume itself + ACTS_VERBOSE("~> volumeID: " << m_volumeID); + volume.assignGeometryId(m_volumeID); + + // assign the material if you have a decorator + if (m_materialDecorator != nullptr) { + ACTS_VERBOSE("Decorating volume " << volume.volumeName() + << " with material"); + m_materialDecorator->decorate(volume); + } + if (volume.volumeMaterial() == nullptr && + volume.motherVolume() != nullptr && + volume.motherVolume()->volumeMaterial() != nullptr) { + auto protoMaterial = dynamic_cast( + volume.motherVolume()->volumeMaterial()); + if (protoMaterial == nullptr) { + volume.assignVolumeMaterial(volume.motherVolume()->volumeMaterialPtr()); + } + } + } + + void visitBoundarySurface( + BoundarySurfaceT& boundary) override { + ACTS_DEBUG("BoundarySurface: " << boundary.surfaceRepresentation().name()); + // get the intersection solution + auto& bSurface = boundary.surfaceRepresentation(); + // create the boundary surface id + m_iboundary += 1; + auto boundaryID = GeometryIdentifier(m_volumeID).setBoundary(m_iboundary); + ACTS_VERBOSE("~> boundaryID: " << boundaryID); + // now assign to the boundary surface + auto& mutableBSurface = *(const_cast(&bSurface)); + + // assign the boundary ID to the surface + ACTS_VERBOSE("~> assigning boundaryID: " << boundaryID); + mutableBSurface.assignGeometryId(boundaryID); + + // Assign material if you have a decorator + if (m_materialDecorator != nullptr) { + ACTS_VERBOSE("Decorating boundary surface " << bSurface.name() + << " with material"); + m_materialDecorator->decorate(mutableBSurface); + } + } + + void visitLayer(Layer& layer) override { + ACTS_DEBUG("Close Layer"); + // create the layer identification + m_ilayer += 1; + auto layerID = GeometryIdentifier(m_volumeID).setLayer(m_ilayer); + ACTS_VERBOSE("~> layerID: " << layerID); + + // now close the geometry + layer.closeGeometry(m_materialDecorator, layerID, *m_hook, *m_logger); + } + + const Logger* m_logger; + GeometryIdentifier m_volumeID; + GeometryIdentifier::Value m_iboundary = 0; + GeometryIdentifier::Value m_ilayer = 0; + const IMaterialDecorator* m_materialDecorator = nullptr; + const GeometryIdentifierHook* m_hook = nullptr; + + std::unordered_map m_volumesById{}; + std::unordered_map m_surfacesById{}; +}; + +TrackingGeometry::TrackingGeometry( const MutableTrackingVolumePtr& highestVolume, const IMaterialDecorator* materialDecorator, - const GeometryIdentifierHook& hook, const Logger& logger) + const GeometryIdentifierHook& hook, const Logger& logger, bool close) : m_world(highestVolume) { - // Close the geometry: assign geometryID and successively the material - std::size_t volumeID = 0; - highestVolume->closeGeometry(materialDecorator, m_volumesById, volumeID, hook, - logger); - m_volumesById.rehash(0); - // fill surface lookup container - m_world->visitSurfaces([this](const Acts::Surface* srf) { - if (srf != nullptr) { - m_surfacesById[srf->geometryId()] = srf; + if (close) { + Gen1GeometryClosureVisitor visitor{logger, materialDecorator, hook}; + apply(visitor); + } + + class Visitor : public TrackingGeometryVisitor { + public: + void visitVolume(const TrackingVolume& volume) override { + auto [it, inserted] = m_volumesById.emplace(volume.geometryId(), &volume); + if (!inserted) { + std::stringstream ss; + ss << "Duplicate volume ID: " << volume.geometryId(); + throw std::invalid_argument(ss.str()); + } } - }); + void visitSurface(const Surface& surface) override { + if (surface.geometryId() == GeometryIdentifier{}) { + throw std::invalid_argument("Surface has no geometry ID"); + } + //@TODO: Why not use all of them? + if (surface.geometryId().sensitive() != 0) { + auto [it, inserted] = + m_surfacesById.emplace(surface.geometryId(), &surface); + if (!inserted) { + std::stringstream ss; + ss << "Duplicate surface ID: " << surface.geometryId(); + throw std::invalid_argument(ss.str()); + } + } + } + + std::unordered_map + m_volumesById{}; + std::unordered_map m_surfacesById{}; + }; + Visitor mapVisitor; + apply(mapVisitor); + m_volumesById = std::move(mapVisitor.m_volumesById); + m_surfacesById = std::move(mapVisitor.m_surfacesById); + + ACTS_DEBUG("TrackingGeometry created with " + << m_volumesById.size() << " volumes and " << m_surfacesById.size() + << " sensitive surfaces"); + + m_volumesById.rehash(0); m_surfacesById.rehash(0); } -Acts::TrackingGeometry::~TrackingGeometry() = default; +TrackingGeometry::~TrackingGeometry() = default; -const Acts::TrackingVolume* Acts::TrackingGeometry::lowestTrackingVolume( - const GeometryContext& gctx, const Acts::Vector3& gp) const { +const TrackingVolume* TrackingGeometry::lowestTrackingVolume( + const GeometryContext& gctx, const Vector3& gp) const { return m_world->lowestTrackingVolume(gctx, gp, s_onSurfaceTolerance); } -const Acts::TrackingVolume* Acts::TrackingGeometry::highestTrackingVolume() - const { +const TrackingVolume* TrackingGeometry::highestTrackingVolume() const { return m_world.get(); } -std::shared_ptr -Acts::TrackingGeometry::highestTrackingVolumePtr() const { +TrackingVolume* TrackingGeometry::highestTrackingVolume() { + return m_world.get(); +} + +std::shared_ptr +TrackingGeometry::highestTrackingVolumePtr() const { return m_world; } -const Acts::Layer* Acts::TrackingGeometry::associatedLayer( - const GeometryContext& gctx, const Acts::Vector3& gp) const { +const Layer* TrackingGeometry::associatedLayer(const GeometryContext& gctx, + const Vector3& gp) const { const TrackingVolume* lowestVol = lowestTrackingVolume(gctx, gp); if (lowestVol == nullptr) { return nullptr; @@ -60,7 +191,7 @@ const Acts::Layer* Acts::TrackingGeometry::associatedLayer( return lowestVol->associatedLayer(gctx, gp); } -const Acts::TrackingVolume* Acts::TrackingGeometry::findVolume( +const TrackingVolume* TrackingGeometry::findVolume( GeometryIdentifier id) const { auto vol = m_volumesById.find(id); if (vol == m_volumesById.end()) { @@ -69,8 +200,7 @@ const Acts::TrackingVolume* Acts::TrackingGeometry::findVolume( return vol->second; } -const Acts::Surface* Acts::TrackingGeometry::findSurface( - GeometryIdentifier id) const { +const Surface* TrackingGeometry::findSurface(GeometryIdentifier id) const { auto srf = m_surfacesById.find(id); if (srf == m_surfacesById.end()) { return nullptr; @@ -78,15 +208,26 @@ const Acts::Surface* Acts::TrackingGeometry::findSurface( return srf->second; } -const std::unordered_map& -Acts::TrackingGeometry::geoIdSurfaceMap() const { +const std::unordered_map& +TrackingGeometry::geoIdSurfaceMap() const { return m_surfacesById; } -void Acts::TrackingGeometry::visualize( - IVisualization3D& helper, const GeometryContext& gctx, - const ViewConfig& viewConfig, const ViewConfig& portalViewConfig, - const ViewConfig& sensitiveViewConfig) const { +void TrackingGeometry::visualize(IVisualization3D& helper, + const GeometryContext& gctx, + const ViewConfig& viewConfig, + const ViewConfig& portalViewConfig, + const ViewConfig& sensitiveViewConfig) const { highestTrackingVolume()->visualize(helper, gctx, viewConfig, portalViewConfig, sensitiveViewConfig); } + +void TrackingGeometry::apply(TrackingGeometryVisitor& visitor) const { + highestTrackingVolume()->apply(visitor); +} + +void TrackingGeometry::apply(TrackingGeometryMutableVisitor& visitor) { + highestTrackingVolume()->apply(visitor); +} + +} // namespace Acts diff --git a/Core/src/Geometry/TrackingGeometryBuilder.cpp b/Core/src/Geometry/TrackingGeometryBuilder.cpp index b8a8b0bf070..0bff6539c2f 100644 --- a/Core/src/Geometry/TrackingGeometryBuilder.cpp +++ b/Core/src/Geometry/TrackingGeometryBuilder.cpp @@ -42,6 +42,7 @@ void Acts::TrackingGeometryBuilder::setLogger( std::unique_ptr Acts::TrackingGeometryBuilder::trackingGeometry( const GeometryContext& gctx) const { + ACTS_DEBUG("Building tracking geometry"); MutableTrackingVolumePtr highestVolume = nullptr; // loop over the builders and wrap one around the other for (auto& volumeBuilder : m_cfg.trackingVolumeBuilders) { diff --git a/Core/src/Geometry/TrackingGeometryVisitor.cpp b/Core/src/Geometry/TrackingGeometryVisitor.cpp new file mode 100644 index 00000000000..a52f7e076de --- /dev/null +++ b/Core/src/Geometry/TrackingGeometryVisitor.cpp @@ -0,0 +1,58 @@ +// This file is part of the ACTS project. +// +// Copyright (C) 2016 CERN for the benefit of the ACTS project +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +#include "Acts/Geometry/TrackingGeometryVisitor.hpp" + +namespace Acts { + +TrackingGeometryVisitor::~TrackingGeometryVisitor() = default; +TrackingGeometryMutableVisitor::~TrackingGeometryMutableVisitor() = default; + +void TrackingGeometryVisitor::visitVolume(const TrackingVolume& /*volume*/) { + // Default implementation is a no-op +} + +void TrackingGeometryVisitor::visitPortal(const Portal& /*portal*/) { + // Default implementation is a no-op +} + +void TrackingGeometryVisitor::visitSurface(const Surface& /*surface*/) { + // Default implementation is a no-op +} + +void TrackingGeometryVisitor::visitLayer(const Layer& /*layer*/) { + // Default implementation is a no-op +} + +void TrackingGeometryVisitor::visitBoundarySurface( + const BoundarySurfaceT& /*boundary*/) { + // Default implementation is a no-op +} + +void TrackingGeometryMutableVisitor::visitVolume(TrackingVolume& /*volume*/) { + // Default implementation is a no-op +} + +void TrackingGeometryMutableVisitor::visitPortal(Portal& /*portal*/) { + // Default implementation is a no-op +} + +void TrackingGeometryMutableVisitor::visitSurface(Surface& /*surface*/) { + // Default implementation is a no-op +} + +void TrackingGeometryMutableVisitor::visitBoundarySurface( + BoundarySurfaceT& /*boundary*/) { + // Default implementation is a no-op +} + +void TrackingGeometryMutableVisitor::visitLayer(Layer& /*layer*/) { + // Default implementation is a no-op +} + +} // namespace Acts diff --git a/Core/src/Geometry/TrackingVolume.cpp b/Core/src/Geometry/TrackingVolume.cpp index 2981c3495fa..9bd9e8cd384 100644 --- a/Core/src/Geometry/TrackingVolume.cpp +++ b/Core/src/Geometry/TrackingVolume.cpp @@ -12,6 +12,7 @@ #include "Acts/Geometry/GeometryIdentifier.hpp" #include "Acts/Geometry/GlueVolumesDescriptor.hpp" #include "Acts/Geometry/Portal.hpp" +#include "Acts/Geometry/TrackingGeometryVisitor.hpp" #include "Acts/Geometry/VolumeBounds.hpp" #include "Acts/Material/IMaterialDecorator.hpp" #include "Acts/Material/IVolumeMaterial.hpp" @@ -359,133 +360,6 @@ void TrackingVolume::interlinkLayers() { } } -void TrackingVolume::closeGeometry( - const IMaterialDecorator* materialDecorator, - std::unordered_map& volumeMap, - std::size_t& vol, const GeometryIdentifierHook& hook, - const Logger& logger) { - if (m_confinedVolumes && !volumes().empty()) { - ACTS_ERROR( - "TrackingVolume::closeGeometry: Volume " - << volumeName() - << " has both confined volumes and volumes. This is not supported."); - throw std::invalid_argument("Volume has both confined volumes and volumes"); - } - - if (m_confinedLayers && !surfaces().empty()) { - ACTS_ERROR( - "TrackingVolume::closeGeometry: Volume " - << volumeName() - << " has both confined layers and surfaces. This is not supported."); - throw std::invalid_argument("Volume has both confined layers and surfaces"); - } - - // we can construct the volume ID from this - auto volumeID = GeometryIdentifier().withVolume(++vol); - // assign the Volume ID to the volume itself - auto thisVolume = const_cast(this); - thisVolume->assignGeometryId(volumeID); - ACTS_DEBUG("volumeID: " << volumeID << ", name: " << volumeName()); - // insert the volume into the map - volumeMap[volumeID] = thisVolume; - - // assign the material if you have a decorator - if (materialDecorator != nullptr) { - materialDecorator->decorate(*thisVolume); - } - if (thisVolume->volumeMaterial() == nullptr && - thisVolume->motherVolume() != nullptr && - thisVolume->motherVolume()->volumeMaterial() != nullptr) { - auto protoMaterial = dynamic_cast( - thisVolume->motherVolume()->volumeMaterial()); - if (protoMaterial == nullptr) { - thisVolume->assignVolumeMaterial( - thisVolume->motherVolume()->volumeMaterialPtr()); - } - } - - this->assignGeometryId(volumeID); - // loop over the boundary surfaces - GeometryIdentifier::Value iboundary = 0; - // loop over the boundary surfaces - for (auto& bSurfIter : boundarySurfaces()) { - // get the intersection solution - auto& bSurface = bSurfIter->surfaceRepresentation(); - // create the boundary surface id - iboundary++; - auto boundaryID = GeometryIdentifier(volumeID).withBoundary(iboundary); - // now assign to the boundary surface - auto& mutableBSurface = *(const_cast(&bSurface)); - mutableBSurface.assignGeometryId(boundaryID); - // Assign material if you have a decorator - if (materialDecorator != nullptr) { - materialDecorator->decorate(mutableBSurface); - } - } - - // A) this is NOT a container volume, volumeID is already incremented - if (!m_confinedVolumes) { - // loop over the confined layers - if (m_confinedLayers) { - GeometryIdentifier::Value ilayer = 0; - // loop over the layers - for (auto& layerPtr : m_confinedLayers->arrayObjects()) { - // create the layer identification - ilayer++; - auto layerID = GeometryIdentifier(volumeID).withLayer(ilayer); - // now close the geometry - auto mutableLayerPtr = std::const_pointer_cast(layerPtr); - mutableLayerPtr->closeGeometry(materialDecorator, layerID, hook, - logger); - } - } - } else { - // B) this is a container volume, go through sub volume - // do the loop - for (auto& volumesIter : m_confinedVolumes->arrayObjects()) { - auto mutableVolumesIter = - std::const_pointer_cast(volumesIter); - mutableVolumesIter->setMotherVolume(this); - mutableVolumesIter->closeGeometry(materialDecorator, volumeMap, vol, hook, - logger); - } - } - - if (!m_confinedDenseVolumes.empty()) { - for (auto& volumesIter : m_confinedDenseVolumes) { - auto mutableVolumesIter = - std::const_pointer_cast(volumesIter); - mutableVolumesIter->setMotherVolume(this); - mutableVolumesIter->closeGeometry(materialDecorator, volumeMap, vol, hook, - logger); - } - } - - GeometryIdentifier::Value iportal = 0; - for (auto& portal : portals()) { - iportal++; - auto portalId = GeometryIdentifier(volumeID).withBoundary(iportal); - assert(portal.isValid() && "Invalid portal encountered during closing"); - - portal.surface().assignGeometryId(portalId); - } - - GeometryIdentifier::Value isensitive = 0; - - for (auto& surface : surfaces()) { - if (surface.associatedDetectorElement() == nullptr) { - continue; - } - isensitive++; - auto sensitiveId = GeometryIdentifier(volumeID).withSensitive(isensitive); - surface.assignGeometryId(sensitiveId); - } - - for (auto& volume : volumes()) { - volume.closeGeometry(materialDecorator, volumeMap, vol, hook, logger); - } -} - // Returns the boundary surfaces ordered in probability to hit them based on boost::container::small_vector TrackingVolume::compatibleBoundaries(const GeometryContext& gctx, @@ -772,4 +646,124 @@ void TrackingVolume::initializeNavigationCandidates( m_navigationDelegate(args, stream, logger); } +namespace { + +void visitLayer(const Acts::Layer& layer, TrackingGeometryVisitor& visitor) { + visitor.visitLayer(layer); + // Surfaces contained in the surface array + if (layer.surfaceArray() != nullptr) { + for (const auto& srf : layer.surfaceArray()->surfaces()) { + visitor.visitSurface(*srf); + } + } + visitor.visitSurface(layer.surfaceRepresentation()); + if (layer.approachDescriptor() != nullptr) { + for (const auto& srf : layer.approachDescriptor()->containedSurfaces()) { + visitor.visitSurface(*srf); + } + } +} + +} // namespace + +// @TODO: Unify once Gen1 is removed: should share most code between mutable and const +void TrackingVolume::apply(TrackingGeometryVisitor& visitor) const { + visitor.visitVolume(*this); + + // Visit the boundary surfaces + for (const auto& bs : m_boundarySurfaces) { + visitor.visitBoundarySurface(*bs); + visitor.visitSurface(bs->surfaceRepresentation()); + } + + for (const auto& portal : portals()) { + visitor.visitPortal(portal); + visitor.visitSurface(portal.surface()); + } + + // Internal structure + if (m_confinedLayers != nullptr) { + std::ranges::for_each( + m_confinedLayers->arrayObjects(), + [&](const auto& layer) { visitLayer(*layer, visitor); }); + } + + if (m_confinedVolumes != nullptr) { + // contains sub volumes + for (const auto& volume : m_confinedVolumes->arrayObjects()) { + volume->apply(visitor); + } + } + + for (const auto& surface : surfaces()) { + visitor.visitSurface(surface); + } + + for (const auto& volume : volumes()) { + volume.apply(visitor); + } +} + +void Acts::TrackingVolume::apply(TrackingGeometryMutableVisitor& visitor) { + visitor.visitVolume(*this); + + // Visit the boundary surfaces + // This does const casts because Gen1 substructure does not have transitive + // const-ness + // @TODO: Remove this when Gen1 is remoeved + for (const auto& bs : m_boundarySurfaces) { + visitor.visitBoundarySurface( + const_cast&>(*bs)); + visitor.visitSurface( + const_cast(bs->surfaceRepresentation())); + } + + for (auto& portal : portals()) { + visitor.visitPortal(portal); + visitor.visitSurface(portal.surface()); + } + + // Internal structure + // This does const casts because Gen1 substructure does not have transitive + // const-ness + // @TODO: Remove this when Gen1 is remoeved + if (m_confinedVolumes == nullptr) { + // no sub volumes => loop over the confined layers + if (m_confinedLayers != nullptr) { + for (const auto& layer : m_confinedLayers->arrayObjects()) { + visitor.visitLayer(const_cast(*layer)); + // Surfaces contained in the surface array + if (layer->surfaceArray() != nullptr) { + for (const auto& srf : layer->surfaceArray()->surfaces()) { + visitor.visitSurface(const_cast(*srf)); + } + } + // Surfaces of the layer + visitor.visitSurface( + const_cast(layer->surfaceRepresentation())); + // Approach surfaces of the layer + if (layer->approachDescriptor() != nullptr) { + for (const auto& srf : + layer->approachDescriptor()->containedSurfaces()) { + visitor.visitSurface(const_cast(*srf)); + } + } + } + } + } else { + // contains sub volumes + for (const auto& volume : m_confinedVolumes->arrayObjects()) { + const_cast(*volume).apply(visitor); + } + } + + for (auto& surface : surfaces()) { + visitor.visitSurface(surface); + } + + for (auto& volume : volumes()) { + volume.apply(visitor); + } +} + } // namespace Acts diff --git a/Examples/Python/src/Blueprint.cpp b/Examples/Python/src/Blueprint.cpp index 8f14f1c89e9..e4ada895a95 100644 --- a/Examples/Python/src/Blueprint.cpp +++ b/Examples/Python/src/Blueprint.cpp @@ -240,7 +240,6 @@ void addBlueprint(Context& ctx) { auto c = py::class_(rootNode, "Config").def(py::init()); ACTS_PYTHON_STRUCT_BEGIN(c, Blueprint::Config); ACTS_PYTHON_MEMBER(envelope); - ACTS_PYTHON_MEMBER(geometryIdentifierHook); ACTS_PYTHON_STRUCT_END(); } diff --git a/Examples/Python/src/Geometry.cpp b/Examples/Python/src/Geometry.cpp index d86fa05cb6c..b93077efcfc 100644 --- a/Examples/Python/src/Geometry.cpp +++ b/Examples/Python/src/Geometry.cpp @@ -30,8 +30,11 @@ #include "Acts/Geometry/GeometryContext.hpp" #include "Acts/Geometry/GeometryHierarchyMap.hpp" #include "Acts/Geometry/GeometryIdentifier.hpp" +#include "Acts/Geometry/Portal.hpp" +#include "Acts/Geometry/PortalLinkBase.hpp" #include "Acts/Geometry/ProtoLayer.hpp" #include "Acts/Geometry/TrackingGeometry.hpp" +#include "Acts/Geometry/TrackingGeometryVisitor.hpp" #include "Acts/Geometry/Volume.hpp" #include "Acts/Geometry/VolumeAttachmentStrategy.hpp" #include "Acts/Geometry/VolumeBounds.hpp" @@ -91,6 +94,42 @@ struct IdentifierSurfacesCollector { } }; +#define _INVOKE(method, name, arg) \ + pybind11::gil_scoped_acquire gil; \ + pybind11::function override = pybind11::get_override(this, name); \ + if (!override) { \ + method(arg); \ + return; \ + } \ + override(&arg); + +// We only implement the mutable visitor here, because pybind11 always casts +// away const ness in any case +class PyTrackingGeometryVisitor : public Acts::TrackingGeometryMutableVisitor { + public: + void visitVolume(Acts::TrackingVolume& volume) override { + _INVOKE(Acts::TrackingGeometryMutableVisitor::visitVolume, "visitVolume", + volume); + } + + void visitPortal(Acts::Portal& portal) override { + _INVOKE(Acts::TrackingGeometryMutableVisitor::visitPortal, "visitPortal", + portal); + } + + void visitLayer(Acts::Layer& layer) override { + _INVOKE(Acts::TrackingGeometryMutableVisitor::visitLayer, "visitLayer", + layer); + } + + void visitSurface(Acts::Surface& surface) override { + _INVOKE(Acts::TrackingGeometryMutableVisitor::visitSurface, "visitSurface", + surface); + } +}; + +#undef _INVOKE + } // namespace namespace Acts::Python { @@ -189,7 +228,9 @@ void addGeometry(Context& ctx) { .def("visualize", &Acts::TrackingGeometry::visualize, py::arg("helper"), py::arg("gctx"), py::arg("viewConfig") = s_viewVolume, py::arg("portalViewConfig") = s_viewPortal, - py::arg("sensitiveViewConfig") = s_viewSensitive); + py::arg("sensitiveViewConfig") = s_viewSensitive) + .def("apply", py::overload_cast( + &TrackingGeometry::apply)); } { @@ -241,6 +282,13 @@ void addGeometry(Context& ctx) { })); } + { + py::class_>( + m, "TrackingGeometryMutableVisitor") + .def(py::init<>()); + } + py::class_(m, "ExtentEnvelope") .def(py::init<>()) .def(py::init()) diff --git a/Examples/Python/tests/test_examples.py b/Examples/Python/tests/test_examples.py index 67b36bd185e..79ca611d1ad 100644 --- a/Examples/Python/tests/test_examples.py +++ b/Examples/Python/tests/test_examples.py @@ -1397,3 +1397,33 @@ def test_exatrkx(tmp_path, trk_geo, field, assert_root_hash, backend, hardware): assert rfp.exists() assert_root_hash(root_file, rfp) + + +def test_geometry_visitor(trk_geo): + class Visitor(acts.TrackingGeometryMutableVisitor): + def __init__(self): + super().__init__() + self.num_surfaces = 0 + self.num_layers = 0 + self.num_volumes = 0 + self.num_portals = 0 + + def visitSurface(self, surface: acts.Surface): + self.num_surfaces += 1 + + def visitLayer(self, layer: acts.Layer): + self.num_layers += 1 + + def visitVolume(self, volume: acts.Volume): + self.num_volumes += 1 + + def visitPortal(self, portal: acts.Portal): + self.num_portals += 1 + + visitor = Visitor() + trk_geo.apply(visitor) + + assert visitor.num_surfaces == 19078 + assert visitor.num_layers == 111 + assert visitor.num_volumes == 18 + assert visitor.num_portals == 0 From 1fe31ff5d56ad2529795ab3ab8cd512228fc05e2 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Wed, 19 Feb 2025 13:51:00 +0100 Subject: [PATCH 08/14] feat: TrackingGeometry visitor gets additional lambda support. (#4099) This allows writing ```cpp trackingGeometry.visit(overloaded{ [](const TrackingVolume& volume) {}, [](const Surface& surface) {}, // ... }); ``` Blocked by: - https://github.com/acts-project/acts/pull/4089 --- .../Acts/Geometry/TrackingGeometry.hpp | 35 +++ .../Acts/Geometry/TrackingGeometryVisitor.hpp | 124 +++++++- Core/include/Acts/Geometry/TrackingVolume.hpp | 78 +++-- Examples/Python/src/Geometry.cpp | 68 +++-- .../Core/Geometry/TrackingVolumeTests.cpp | 271 ++++++++++++++++++ 5 files changed, 512 insertions(+), 64 deletions(-) diff --git a/Core/include/Acts/Geometry/TrackingGeometry.hpp b/Core/include/Acts/Geometry/TrackingGeometry.hpp index 46cd3ceb766..33af38b2a56 100644 --- a/Core/include/Acts/Geometry/TrackingGeometry.hpp +++ b/Core/include/Acts/Geometry/TrackingGeometry.hpp @@ -11,6 +11,7 @@ #include "Acts/Definitions/Algebra.hpp" #include "Acts/Geometry/GeometryContext.hpp" #include "Acts/Geometry/GeometryIdentifier.hpp" +#include "Acts/Geometry/TrackingGeometryVisitor.hpp" #include "Acts/Geometry/TrackingVolume.hpp" #include "Acts/Geometry/TrackingVolumeVisitorConcept.hpp" #include "Acts/Surfaces/SurfaceVisitorConcept.hpp" @@ -137,9 +138,43 @@ class TrackingGeometry { std::forward(visitor)); } + /// @copydoc TrackingVolume::apply void apply(TrackingGeometryVisitor& visitor) const; + + /// @copydoc TrackingVolume::apply void apply(TrackingGeometryMutableVisitor& visitor); + /// @brief Apply an arbitrary callable as a visitor to the tracking volume + /// + /// @param callable The callable to apply + /// + /// @note The visitor can be overloaded on any of the arguments that + /// the methods in @c TrackingGeometryVisitor receive. + template + void apply(Callable&& callable) + requires(detail::callableWithAnyMutable() && + !detail::callableWithAnyConst()) + { + detail::TrackingGeometryLambdaVisitor visitor{ + std::forward(callable)}; + apply(visitor); + } + + /// @brief Apply an arbitrary callable as a visitor to the tracking volume + /// + /// @param callable The callable to apply + /// + /// @note The visitor can be overloaded on any of the arguments that + /// the methods in @c TrackingGeometryMutableVisitor receive. + template + void apply(Callable&& callable) const + requires(detail::callableWithAnyConst()) + { + detail::TrackingGeometryLambdaMutableVisitor visitor{ + std::forward(callable)}; + apply(visitor); + } + /// Search for a volume with the given identifier. /// /// @param id is the geometry identifier of the volume diff --git a/Core/include/Acts/Geometry/TrackingGeometryVisitor.hpp b/Core/include/Acts/Geometry/TrackingGeometryVisitor.hpp index 59c70b5b883..073fdf45b0c 100644 --- a/Core/include/Acts/Geometry/TrackingGeometryVisitor.hpp +++ b/Core/include/Acts/Geometry/TrackingGeometryVisitor.hpp @@ -8,6 +8,9 @@ #pragma once +#include +#include + namespace Acts { class TrackingVolume; @@ -58,9 +61,9 @@ class TrackingGeometryVisitor { /// @brief Mutable visitor interface for modifying the tracking geometry hierarchy /// -/// This visitor allows for non-const access to traverse and modify the tracking -/// geometry components. It's used for operations like geometry construction, -/// material decoration, or geometry ID assignment. +/// This visitor allows for non-const access to traverse and modify the +/// tracking geometry components. It's used for operations like geometry +/// construction, material decoration, or geometry ID assignment. class TrackingGeometryMutableVisitor { public: virtual ~TrackingGeometryMutableVisitor(); @@ -92,4 +95,119 @@ class TrackingGeometryMutableVisitor { virtual void visitBoundarySurface(BoundarySurfaceT& boundary); }; +namespace detail { + +template +consteval bool callableWithAnyMutable() { + return std::is_invocable_v || + std::is_invocable_v || + std::is_invocable_v&> || + std::is_invocable_v || + std::is_invocable_v; +} + +template +consteval bool callableWithAnyConst() { + return std::is_invocable_v || + std::is_invocable_v || + std::is_invocable_v&> || + std::is_invocable_v || + std::is_invocable_v; +} + +template +consteval bool callableWithAny() { + return callableWithAnyMutable() || callableWithAnyConst(); +} + +template + requires(callableWithAnyConst()) +class TrackingGeometryLambdaVisitor : public TrackingGeometryVisitor { + public: + explicit TrackingGeometryLambdaVisitor(Callable callable) + : m_callable(std::move(callable)) {} + + void visitSurface(const Surface& surface) override { + if constexpr (std::is_invocable_v) { + m_callable(surface); + } + } + + void visitVolume(const TrackingVolume& volume) override { + if constexpr (std::is_invocable_v) { + m_callable(volume); + } + } + + void visitBoundarySurface( + const BoundarySurfaceT& boundary) override { + if constexpr (std::is_invocable_v< + Callable, const BoundarySurfaceT&>) { + m_callable(boundary); + } + } + + void visitLayer(const Layer& layer) override { + if constexpr (std::is_invocable_v) { + m_callable(layer); + } + } + + void visitPortal(const Portal& portal) override { + if constexpr (std::is_invocable_v) { + m_callable(portal); + } + } + + private: + Callable m_callable; +}; + +template + requires(callableWithAnyMutable()) +class TrackingGeometryLambdaMutableVisitor + : public TrackingGeometryMutableVisitor { + public: + explicit TrackingGeometryLambdaMutableVisitor(Callable callable) + : m_callable(std::move(callable)) {} + + void visitSurface(Surface& surface) override { + if constexpr (std::is_invocable_v) { + m_callable(surface); + } + } + + void visitVolume(TrackingVolume& volume) override { + if constexpr (std::is_invocable_v) { + m_callable(volume); + } + } + + void visitBoundarySurface( + BoundarySurfaceT& boundary) override { + if constexpr (std::is_invocable_v&>) { + m_callable(boundary); + } + } + + void visitLayer(Layer& layer) override { + if constexpr (std::is_invocable_v) { + m_callable(layer); + } + } + + void visitPortal(Portal& portal) override { + if constexpr (std::is_invocable_v) { + m_callable(portal); + } + } + + private: + Callable m_callable; +}; + +} // namespace detail + } // namespace Acts diff --git a/Core/include/Acts/Geometry/TrackingVolume.hpp b/Core/include/Acts/Geometry/TrackingVolume.hpp index b8f96fe8773..de50642c8a1 100644 --- a/Core/include/Acts/Geometry/TrackingVolume.hpp +++ b/Core/include/Acts/Geometry/TrackingVolume.hpp @@ -22,7 +22,6 @@ #include "Acts/Navigation/NavigationDelegate.hpp" #include "Acts/Navigation/NavigationStream.hpp" #include "Acts/Surfaces/Surface.hpp" -#include "Acts/Surfaces/SurfaceArray.hpp" #include "Acts/Surfaces/SurfaceVisitorConcept.hpp" #include "Acts/Utilities/BinnedArray.hpp" #include "Acts/Utilities/Logger.hpp" @@ -178,24 +177,12 @@ class TrackingVolume : public Volume { /// this, e.g. as a private member template void visitSurfaces(visitor_t&& visitor, bool restrictToSensitives) const { - struct Visitor : public TrackingGeometryVisitor { - std::remove_cv_t>* m_visitor{}; - bool m_restrictToSensitives{}; - - void visitSurface(const Surface& surface) override { - if (m_restrictToSensitives && surface.geometryId().sensitive() == 0) { - return; - } - assert(m_visitor != nullptr); - (*m_visitor)(&surface); + apply([&visitor, restrictToSensitives](const Surface& surface) { + if (restrictToSensitives && surface.geometryId().sensitive() == 0) { + return; } - }; - - Visitor internal; - internal.m_visitor = &visitor; - internal.m_restrictToSensitives = restrictToSensitives; - - apply(internal); + visitor(&surface); + }); } /// @brief Visit all sensitive surfaces @@ -223,23 +210,51 @@ class TrackingVolume : public Volume { /// this, e.g. as a private member template void visitVolumes(visitor_t&& visitor) const { - struct Visitor : public TrackingGeometryVisitor { - std::remove_cv_t>* m_visitor{}; - bool m_restrictToSensitives{}; + apply([&visitor](const TrackingVolume& volume) { visitor(&volume); }); + } - void visitVolume(const TrackingVolume& volume) override { - (*m_visitor)(&volume); - } - }; + /// @brief Apply a visitor to the tracking volume + /// + /// @param visitor The visitor to apply + /// + void apply(TrackingGeometryVisitor& visitor) const; - Visitor internal; - internal.m_visitor = &visitor; + /// @brief Apply a mutable visitor to the tracking volume + /// + /// @param visitor The visitor to apply + /// + void apply(TrackingGeometryMutableVisitor& visitor); - apply(internal); + /// @brief Apply an arbitrary callable as a visitor to the tracking volume + /// + /// @param callable The callable to apply + /// + /// @note The visitor can be overloaded on any of the arguments that + /// the methods in @c TrackingGeometryVisitor receive. + template + void apply(Callable&& callable) + requires(detail::callableWithAnyMutable() && + !detail::callableWithAnyConst()) + { + detail::TrackingGeometryLambdaMutableVisitor visitor{ + std::forward(callable)}; + apply(visitor); } - void apply(TrackingGeometryVisitor& visitor) const; - void apply(TrackingGeometryMutableVisitor& visitor); + /// @brief Apply an arbitrary callable as a visitor to the tracking volume + /// + /// @param callable The callable to apply + /// + /// @note The visitor can be overloaded on any of the arguments that + /// the methods in @c TrackingGeometryMutableVisitor receive. + template + void apply(Callable&& callable) const + requires(detail::callableWithAnyConst()) + { + detail::TrackingGeometryLambdaVisitor visitor{ + std::forward(callable)}; + apply(visitor); + } /// Returns the VolumeName - for debug reason, might be depreciated later const std::string& volumeName() const; @@ -453,6 +468,9 @@ class TrackingVolume : public Volume { /// @param gvd register a new GlueVolumeDescriptor void registerGlueVolumeDescriptor(std::unique_ptr gvd); + /// Clear boundary surfaces for this tracking volume + void clearBoundarySurfaces(); + /// Register the outside glue volumes - /// ordering is in the TrackingVolume Frame: /// - negativeFaceXY diff --git a/Examples/Python/src/Geometry.cpp b/Examples/Python/src/Geometry.cpp index b93077efcfc..7313e8888b1 100644 --- a/Examples/Python/src/Geometry.cpp +++ b/Examples/Python/src/Geometry.cpp @@ -200,37 +200,43 @@ void addGeometry(Context& ctx) { } { - py::class_>( - m, "TrackingGeometry") - .def(py::init([](const MutableTrackingVolumePtr& volPtr, - std::shared_ptr matDec, - const GeometryIdentifierHook& hook, - Acts::Logging::Level level) { - auto logger = Acts::getDefaultLogger("TrackingGeometry", level); - auto trkGeo = std::make_shared( - volPtr, matDec.get(), hook, *logger); - return trkGeo; - })) - .def("visitSurfaces", - [](Acts::TrackingGeometry& self, py::function& func) { - self.visitSurfaces(func); - }) - .def("geoIdSurfaceMap", &Acts::TrackingGeometry::geoIdSurfaceMap) - .def("extractMaterialSurfaces", - [](Acts::TrackingGeometry& self) { - MaterialSurfaceSelector selector; - self.visitSurfaces(selector, false); - return selector.surfaces; - }) - .def_property_readonly( - "highestTrackingVolume", - &Acts::TrackingGeometry::highestTrackingVolumePtr) - .def("visualize", &Acts::TrackingGeometry::visualize, py::arg("helper"), - py::arg("gctx"), py::arg("viewConfig") = s_viewVolume, - py::arg("portalViewConfig") = s_viewPortal, - py::arg("sensitiveViewConfig") = s_viewSensitive) - .def("apply", py::overload_cast( - &TrackingGeometry::apply)); + auto trkGeo = + py::class_>(m, + "TrackingGeometry") + .def(py::init([](const MutableTrackingVolumePtr& volPtr, + std::shared_ptr matDec, + const GeometryIdentifierHook& hook, + Acts::Logging::Level level) { + auto logger = Acts::getDefaultLogger("TrackingGeometry", level); + auto obj = std::make_shared( + volPtr, matDec.get(), hook, *logger); + return obj; + })) + .def("visitSurfaces", + [](Acts::TrackingGeometry& self, py::function& func) { + self.visitSurfaces(func); + }) + .def("geoIdSurfaceMap", &Acts::TrackingGeometry::geoIdSurfaceMap) + .def("extractMaterialSurfaces", + [](Acts::TrackingGeometry& self) { + MaterialSurfaceSelector selector; + self.visitSurfaces(selector, false); + return selector.surfaces; + }) + .def_property_readonly( + "highestTrackingVolume", + &Acts::TrackingGeometry::highestTrackingVolumePtr) + .def("visualize", &Acts::TrackingGeometry::visualize, + py::arg("helper"), py::arg("gctx"), + py::arg("viewConfig") = s_viewVolume, + py::arg("portalViewConfig") = s_viewPortal, + py::arg("sensitiveViewConfig") = s_viewSensitive); + + using apply_ptr_t = + void (TrackingGeometry::*)(TrackingGeometryMutableVisitor&); + + trkGeo.def("apply", static_cast(&TrackingGeometry::apply)); } { diff --git a/Tests/UnitTests/Core/Geometry/TrackingVolumeTests.cpp b/Tests/UnitTests/Core/Geometry/TrackingVolumeTests.cpp index d6d9bb6dd0f..9d301e4bdad 100644 --- a/Tests/UnitTests/Core/Geometry/TrackingVolumeTests.cpp +++ b/Tests/UnitTests/Core/Geometry/TrackingVolumeTests.cpp @@ -10,7 +10,15 @@ #include "Acts/Definitions/Units.hpp" #include "Acts/Geometry/CylinderVolumeBounds.hpp" +#include "Acts/Geometry/Portal.hpp" +#include "Acts/Geometry/PortalLinkBase.hpp" +#include "Acts/Geometry/TrackingGeometryVisitor.hpp" #include "Acts/Geometry/TrackingVolume.hpp" +#include "Acts/Surfaces/PlaneSurface.hpp" +#include "Acts/Surfaces/RectangleBounds.hpp" +#include "Acts/Utilities/Helpers.hpp" + +#include "LayerStub.hpp" using namespace Acts::UnitLiterals; @@ -56,6 +64,269 @@ BOOST_AUTO_TEST_CASE(TrackigVolumeChildren) { BOOST_CHECK_EQUAL(countVolumes(tv), 3); } +namespace { +void testVisitor(auto visitor) { + BOOST_CHECK(!visitor.m_surfaceCalled); + BOOST_CHECK(!visitor.m_volumeCalled); + BOOST_CHECK(!visitor.m_boundaryCalled); + BOOST_CHECK(!visitor.m_layerCalled); + BOOST_CHECK(!visitor.m_portalCalled); + + auto surface = Surface::makeShared( + Transform3::Identity(), std::make_shared(10_mm, 10_mm)); + + visitor.visitSurface(*surface); + BOOST_CHECK(visitor.m_surfaceCalled); + + auto cylBounds = + std::make_shared(10_mm, 20_mm, 100_mm); + + TrackingVolume tv{Transform3::Identity(), cylBounds}; + visitor.visitVolume(tv); + + BOOST_CHECK(visitor.m_volumeCalled); + + BoundarySurfaceT boundary{surface, &tv, nullptr}; + visitor.visitBoundarySurface(boundary); + BOOST_CHECK(visitor.m_boundaryCalled); + + LayerStub layer{nullptr}; + visitor.visitLayer(layer); + BOOST_CHECK(visitor.m_layerCalled); + + Portal portal{Direction::AlongNormal(), surface, tv}; + visitor.visitPortal(portal); + BOOST_CHECK(visitor.m_portalCalled); + + visitor.m_volumeCalled = false; + tv.apply(visitor); + BOOST_CHECK(visitor.m_volumeCalled); +} +} // namespace + +BOOST_AUTO_TEST_CASE(Visit) { + struct Visitor : public TrackingGeometryVisitor { + void visitSurface(const Surface& /*surface*/) override { + m_surfaceCalled = true; + } + + void visitVolume(const TrackingVolume& /*volume*/) override { + m_volumeCalled = true; + } + + void visitBoundarySurface( + const BoundarySurfaceT& /*boundary*/) override { + m_boundaryCalled = true; + } + + void visitLayer(const Layer& /*layer*/) override { m_layerCalled = true; } + + void visitPortal(const Portal& /*portal*/) override { + m_portalCalled = true; + } + + bool m_surfaceCalled = false; + bool m_volumeCalled = false; + bool m_boundaryCalled = false; + bool m_layerCalled = false; + bool m_portalCalled = false; + }; + + testVisitor(Visitor{}); +} + +BOOST_AUTO_TEST_CASE(VisitMutable) { + struct MutableVisitor : public TrackingGeometryMutableVisitor { + void visitSurface(Surface& /*surface*/) override { m_surfaceCalled = true; } + + void visitVolume(TrackingVolume& /*volume*/) override { + m_volumeCalled = true; + } + + void visitBoundarySurface( + BoundarySurfaceT& /*boundary*/) override { + m_boundaryCalled = true; + } + + void visitLayer(Layer& /*layer*/) override { m_layerCalled = true; } + + void visitPortal(Portal& /*portal*/) override { m_portalCalled = true; } + + bool m_surfaceCalled = false; + bool m_volumeCalled = false; + bool m_boundaryCalled = false; + bool m_layerCalled = false; + bool m_portalCalled = false; + }; + + testVisitor(MutableVisitor{}); +} + +BOOST_AUTO_TEST_CASE(VisitLambda) { + bool surfaceCalled = false; + bool volumeCalled = false; + bool boundaryCalled = false; + bool layerCalled = false; + bool portalCalled = false; + + auto visitor = detail::TrackingGeometryLambdaVisitor{overloaded{ + [&](const Surface& /*surface*/) { surfaceCalled = true; }, + [&](const TrackingVolume& /*volume*/) { volumeCalled = true; }, + [&](const BoundarySurfaceT& /*boundary*/) { + boundaryCalled = true; + }, + [&](const Layer& /*layer*/) { layerCalled = true; }, + [&](const Portal& /*portal*/) { portalCalled = true; }, + }}; + + auto surface = Surface::makeShared( + Transform3::Identity(), std::make_shared(10_mm, 10_mm)); + + visitor.visitSurface(*surface); + BOOST_CHECK(surfaceCalled); + + auto cylBounds = + std::make_shared(10_mm, 20_mm, 100_mm); + + TrackingVolume tv{Transform3::Identity(), cylBounds}; + visitor.visitVolume(tv); + + BOOST_CHECK(volumeCalled); + + BoundarySurfaceT boundary{surface, &tv, nullptr}; + visitor.visitBoundarySurface(boundary); + BOOST_CHECK(boundaryCalled); + + LayerStub layer{nullptr}; + visitor.visitLayer(layer); + BOOST_CHECK(layerCalled); + + Portal portal{Direction::AlongNormal(), surface, tv}; + visitor.visitPortal(portal); + BOOST_CHECK(portalCalled); + + volumeCalled = false; + tv.apply(visitor); + BOOST_CHECK(volumeCalled); +} + +BOOST_AUTO_TEST_CASE(VisitLambdaMutable) { + bool surfaceCalled = false; + bool volumeCalled = false; + bool boundaryCalled = false; + bool layerCalled = false; + bool portalCalled = false; + + auto overload = overloaded{ + [&](Surface& /*surface*/) { surfaceCalled = true; }, + [&](TrackingVolume& /*volume*/) { volumeCalled = true; }, + [&](BoundarySurfaceT& /*boundary*/) { + boundaryCalled = true; + }, + [&](Layer& /*layer*/) { layerCalled = true; }, + [&](Portal& /*portal*/) { portalCalled = true; }, + }; + + auto visitor = detail::TrackingGeometryLambdaMutableVisitor{overload}; + + auto surface = Surface::makeShared( + Transform3::Identity(), std::make_shared(10_mm, 10_mm)); + + visitor.visitSurface(*surface); + BOOST_CHECK(surfaceCalled); + + auto cylBounds = + std::make_shared(10_mm, 20_mm, 100_mm); + + TrackingVolume tv{Transform3::Identity(), cylBounds}; + visitor.visitVolume(tv); + + BOOST_CHECK(volumeCalled); + + BoundarySurfaceT boundary{surface, &tv, nullptr}; + visitor.visitBoundarySurface(boundary); + BOOST_CHECK(boundaryCalled); + + LayerStub layer{nullptr}; + visitor.visitLayer(layer); + BOOST_CHECK(layerCalled); + + Portal portal{Direction::AlongNormal(), surface, tv}; + visitor.visitPortal(portal); + BOOST_CHECK(portalCalled); + + volumeCalled = false; + tv.apply(overload); + BOOST_CHECK(volumeCalled); + + volumeCalled = false; + tv.apply([&](Volume& /*volume*/) { volumeCalled = true; }); + BOOST_CHECK(volumeCalled); +} + +BOOST_AUTO_TEST_CASE(CallableWithAny) { + // Test callableWithAny + static_assert( + detail::callableWithAny()); + static_assert( + detail::callableWithAny()); + static_assert(detail::callableWithAny()); + static_assert( + detail::callableWithAny()); + static_assert( + detail::callableWithAny()); + static_assert(!detail::callableWithAny()); + static_assert(!detail::callableWithAny()); + + // Test callableWithAnyConst specifically + static_assert(detail::callableWithAnyConst< + decltype([](const Surface& /*surface*/) {})>()); + static_assert(detail::callableWithAnyConst< + decltype([](const TrackingVolume& /*volume*/) {})>()); + static_assert( + detail::callableWithAnyConst()); + static_assert(!detail::callableWithAnyConst()); + static_assert( + !detail::callableWithAnyConst()); + + // Test callableWithAnyMutable specifically + static_assert( + detail::callableWithAnyMutable()); + static_assert(detail::callableWithAnyMutable< + decltype([](TrackingVolume& /*volume*/) {})>()); + static_assert( + detail::callableWithAnyMutable()); + static_assert(!detail::callableWithAnyMutable()); + static_assert( + !detail::callableWithAnyMutable()); + + // Test mixed const/non-const overloads + static_assert(detail::callableWithAny()); + + // Test with unrelated overloads + static_assert(!detail::callableWithAny()); + + // Test with mix of geometry and unrelated overloads + static_assert(detail::callableWithAny()); + + static_assert(detail::callableWithAny()); +} + BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE_END() From 299a6e74997db37d1b38d73c65974eb2ceafa7ea Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Wed, 19 Feb 2025 15:26:46 +0100 Subject: [PATCH 09/14] refactor: Expose max component values for geometry ids (#4097) Surface the maximum values for the `GeometryIdentifier` components. Blocked by: - https://github.com/acts-project/acts/pull/4094 --- .../Acts/Geometry/GeometryIdentifier.hpp | 27 ++++++++++++++++++- .../Core/Geometry/GeometryIdentifierTests.cpp | 7 +++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/Core/include/Acts/Geometry/GeometryIdentifier.hpp b/Core/include/Acts/Geometry/GeometryIdentifier.hpp index 9029b4c4216..25fc30a8ca8 100644 --- a/Core/include/Acts/Geometry/GeometryIdentifier.hpp +++ b/Core/include/Acts/Geometry/GeometryIdentifier.hpp @@ -161,6 +161,32 @@ class GeometryIdentifier { return id; } + /// Get the maximum value for the volume identifier. + /// @return the maximum value for the volume identifier + static constexpr Value getMaxVolume() { return getMaxValue(kVolumeMask); } + + /// Get the maximum value for the boundary identifier. + /// @return the maximum value for the boundary identifier + static constexpr Value getMaxBoundary() { return getMaxValue(kBoundaryMask); } + + /// Get the maximum value for the layer identifier. + /// @return the maximum value for the layer identifier + static constexpr Value getMaxLayer() { return getMaxValue(kLayerMask); } + + /// Get the maximum value for the approach identifier. + /// @return the maximum value for the approach identifier + static constexpr Value getMaxApproach() { return getMaxValue(kApproachMask); } + + /// Get the maximum value for the sensitive identifier. + /// @return the maximum value for the sensitive identifier + static constexpr Value getMaxSensitive() { + return getMaxValue(kSensitiveMask); + } + + /// Get the maximum value for the extra identifier. + /// @return the maximum value for the extra identifier + static constexpr Value getMaxExtra() { return getMaxValue(kExtraMask); } + private: // clang-format off /// (2^8)-1 = 255 volumes @@ -233,7 +259,6 @@ struct GeometryIdentifierHook { }; } // namespace Acts - // specialize std::hash so GeometryIdentifier can be used e.g. in an // unordered_map namespace std { diff --git a/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp b/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp index 6d9a04922bc..8271d8c8a74 100644 --- a/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp +++ b/Tests/UnitTests/Core/Geometry/GeometryIdentifierTests.cpp @@ -57,6 +57,13 @@ BOOST_AUTO_TEST_CASE(GeometryIdentifier_max_values) { std::invalid_argument); BOOST_CHECK_THROW(GeometryIdentifier(ref).setExtra(extraMax + 1), std::invalid_argument); + + BOOST_CHECK_EQUAL(GeometryIdentifier::getMaxVolume(), 255); + BOOST_CHECK_EQUAL(GeometryIdentifier::getMaxBoundary(), 255); + BOOST_CHECK_EQUAL(GeometryIdentifier::getMaxLayer(), 4095); + BOOST_CHECK_EQUAL(GeometryIdentifier::getMaxApproach(), 255); + BOOST_CHECK_EQUAL(GeometryIdentifier::getMaxSensitive(), 1048575); + BOOST_CHECK_EQUAL(GeometryIdentifier::getMaxExtra(), 255); } BOOST_AUTO_TEST_CASE(GeometryIdentifier_order) { From 8b5261cfbc25ffdf03cbb87e47ead05de24f1c29 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Thu, 20 Feb 2025 11:00:00 +0100 Subject: [PATCH 10/14] fix: Add missing TrackingVolume symbol (#4100) I forgot to commit this in the correct PR :/ --- Core/src/Geometry/TrackingVolume.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Core/src/Geometry/TrackingVolume.cpp b/Core/src/Geometry/TrackingVolume.cpp index 9bd9e8cd384..f995ae2ee6c 100644 --- a/Core/src/Geometry/TrackingVolume.cpp +++ b/Core/src/Geometry/TrackingVolume.cpp @@ -109,7 +109,7 @@ const TrackingVolume* TrackingVolume::lowestTrackingVolume( } const TrackingVolumeBoundaries& TrackingVolume::boundarySurfaces() const { - return (m_boundarySurfaces); + return m_boundarySurfaces; } void TrackingVolume::connectDenseBoundarySurfaces( @@ -174,6 +174,10 @@ void TrackingVolume::createBoundarySurfaces() { } } +void TrackingVolume::clearBoundarySurfaces() { + m_boundarySurfaces.clear(); +} + void TrackingVolume::glueTrackingVolume(const GeometryContext& gctx, BoundarySurfaceFace bsfMine, TrackingVolume* neighbor, From 31ebcaf18401e900cd2deeb76842584a1e9216c7 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 20 Feb 2025 14:59:42 +0100 Subject: [PATCH 11/14] feat: Adaptive step size for the `AtlasStepper` (#4102) Until now the `AtlasStepper` only reduced the step size by a factor of `1/2` if the accuracy target was not met. This can be slow and does not allow for increasing the step size which is important in inhomogeneous magnetic fields. This PR adds our usual adaptive step size solution to the `AtlasStepper` which is identical in the `EigenStepper` and `SympyStepper`. This also allows for apple to apple comparisons between the steppers as they will use the same number of steps to reach a certain path length. ## Summary by CodeRabbit - **New Features** - Enhanced propagation performance with improved error estimation and dynamic step size adjustments, resulting in more robust and accurate operations. - **Refactor** - Streamlined the underlying process by removing redundant calculations, ensuring more efficient and adaptive propagation behavior. --- Core/include/Acts/Propagator/AtlasStepper.hpp | 51 ++++++++++++++++--- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/Core/include/Acts/Propagator/AtlasStepper.hpp b/Core/include/Acts/Propagator/AtlasStepper.hpp index 16cafc17639..80eda451c13 100644 --- a/Core/include/Acts/Propagator/AtlasStepper.hpp +++ b/Core/include/Acts/Propagator/AtlasStepper.hpp @@ -1189,6 +1189,36 @@ class AtlasStepper { bool Helix = false; // if (std::abs(S) < m_cfg.helixStep) Helix = true; + const auto calcStepSizeScaling = + [&](const double errorEstimate_) -> double { + // For details about these values see ATL-SOFT-PUB-2009-001 + constexpr double lower = 0.25; + constexpr double upper = 4.0; + // This is given by the order of the Runge-Kutta method + constexpr double exponent = 0.25; + + double x = state.options.stepTolerance / errorEstimate_; + + if constexpr (exponent == 0.25) { + // This is 3x faster than std::pow + x = std::sqrt(std::sqrt(x)); + } else { + x = std::pow(x, exponent); + } + + return std::clamp(x, lower, upper); + }; + + const auto isErrorTolerable = [&](const double errorEstimate_) { + // For details about these values see ATL-SOFT-PUB-2009-001 + constexpr double marginFactor = 4.0; + + return errorEstimate_ <= marginFactor * state.options.stepTolerance; + }; + + double EST = 0; + double initialH = h; + std::size_t nStepTrials = 0; while (h != 0.) { nStepTrials++; @@ -1269,12 +1299,13 @@ class AtlasStepper { // // This is (h2 * (sd.k1 - sd.k2 - sd.k3 + sd.k4).template lpNorm<1>()) // in EigenStepper - double EST = - 2. * h * - (std::abs((A1 + A6) - (A3 + A4)) + std::abs((B1 + B6) - (B3 + B4)) + - std::abs((C1 + C6) - (C3 + C4))); - if (std::abs(EST) > std::abs(state.options.stepTolerance)) { - h = h * .5; + EST = 2. * std::abs(h) * + (std::abs((A1 + A6) - (A3 + A4)) + std::abs((B1 + B6) - (B3 + B4)) + + std::abs((C1 + C6) - (C3 + C4))); + EST = std::max(1e-20, EST); + if (!isErrorTolerable(EST)) { + const double stepSizeScaling = calcStepSizeScaling(EST); + h *= stepSizeScaling; // neutralize the sign of h again state.stepSize.setAccuracy(h * propDir); // dltm = 0.; @@ -1414,6 +1445,14 @@ class AtlasStepper { ++state.nSteps; state.nStepTrials += nStepTrials; + const double stepSizeScaling = calcStepSizeScaling(EST); + const double nextAccuracy = std::abs(h * stepSizeScaling); + const double previousAccuracy = std::abs(state.stepSize.accuracy()); + const double initialStepLength = std::abs(initialH); + if (nextAccuracy < initialStepLength || nextAccuracy > previousAccuracy) { + state.stepSize.setAccuracy(nextAccuracy); + } + return h; } From 9cfe1898f8335ff94745cdf56862b0fc44e7d7cb Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 20 Feb 2025 17:26:15 +0100 Subject: [PATCH 12/14] refactor: Drop `SympyStepper::stepImpl` (#4103) After getting rid of the templated `step` function there is no reason to have a separate `stepImpl` function anymore. This PR is merging `stepImpl` into `step` and drops former. ## Summary by CodeRabbit - **Refactor** - Streamlined the stepping process by centralizing configuration parameters within the system's state. - Improved error handling and step size scaling to maintain consistent performance. --- Core/include/Acts/Propagator/SympyStepper.hpp | 5 ----- Core/src/Propagator/SympyStepper.cpp | 16 +++++----------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/Core/include/Acts/Propagator/SympyStepper.hpp b/Core/include/Acts/Propagator/SympyStepper.hpp index f64be7cb8f0..46f5e5d2c79 100644 --- a/Core/include/Acts/Propagator/SympyStepper.hpp +++ b/Core/include/Acts/Propagator/SympyStepper.hpp @@ -379,11 +379,6 @@ class SympyStepper { protected: /// Magnetic field inside of the detector std::shared_ptr m_bField; - - private: - Result stepImpl(State& state, Direction propDir, double stepTolerance, - double stepSizeCutOff, - std::size_t maxRungeKuttaStepTrials) const; }; template <> diff --git a/Core/src/Propagator/SympyStepper.cpp b/Core/src/Propagator/SympyStepper.cpp index bc0cf3c2eb4..2b004ac835b 100644 --- a/Core/src/Propagator/SympyStepper.cpp +++ b/Core/src/Propagator/SympyStepper.cpp @@ -129,14 +129,7 @@ void SympyStepper::transportCovarianceToBound( Result SympyStepper::step(State& state, Direction propDir, const IVolumeMaterial* material) const { (void)material; - return stepImpl(state, propDir, state.options.stepTolerance, - state.options.stepSizeCutOff, - state.options.maxRungeKuttaStepTrials); -} -Result SympyStepper::stepImpl( - State& state, Direction propDir, double stepTolerance, - double stepSizeCutOff, std::size_t maxRungeKuttaStepTrials) const { auto pos = position(state); auto dir = direction(state); double t = time(state); @@ -155,7 +148,7 @@ Result SympyStepper::stepImpl( // This is given by the order of the Runge-Kutta method constexpr double exponent = 0.25; - double x = stepTolerance / errorEstimate_; + double x = state.options.stepTolerance / errorEstimate_; if constexpr (exponent == 0.25) { // This is 3x faster than std::pow @@ -179,7 +172,8 @@ Result SympyStepper::stepImpl( // For details about the factor 4 see ATL-SOFT-PUB-2009-001 Result res = rk4(pos.data(), dir.data(), t, h, qop, m, p_abs, getB, &errorEstimate, - 4 * stepTolerance, state.pars.template segment<3>(eFreePos0).data(), + 4 * state.options.stepTolerance, + state.pars.template segment<3>(eFreePos0).data(), state.pars.template segment<3>(eFreeDir0).data(), state.pars.template segment<1>(eFreeTime).data(), state.derivative.data(), @@ -201,14 +195,14 @@ Result SympyStepper::stepImpl( // If step size becomes too small the particle remains at the initial // place - if (std::abs(h) < std::abs(stepSizeCutOff)) { + if (std::abs(h) < std::abs(state.options.stepSizeCutOff)) { // Not moving due to too low momentum needs an aborter return EigenStepperError::StepSizeStalled; } // If the parameter is off track too much or given stepSize is not // appropriate - if (nStepTrials > maxRungeKuttaStepTrials) { + if (nStepTrials > state.options.maxRungeKuttaStepTrials) { // Too many trials, have to abort return EigenStepperError::StepSizeAdjustmentFailed; } From 3e3eb6711496e9a4db6ebbcd236fcf9b09cdc619 Mon Sep 17 00:00:00 2001 From: Ragansu Chakkappai <66349236+Ragansu@users.noreply.github.com> Date: Mon, 24 Feb 2025 13:53:41 +0100 Subject: [PATCH 13/14] feat!: Updating the cuts in ScoreBased Solver to eta based cuts. (#4054) Introduced eta-based cuts in ACTS Standalone. * All detector level cuts are now eta based * The cuts can be a single number or an array with the size of the eta bins * Updated Tests, Examples, python scripts, Json plugins for the same. --- .../ScoreBasedAmbiguityResolution.hpp | 80 ++++---- .../ScoreBasedAmbiguityResolution.ipp | 178 ++++++------------ .../ScoreBasedAmbiguityResolution.cpp | 16 ++ ...ScoreBasedAmbiguityResolutionAlgorithm.hpp | 12 +- ...ScoreBasedAmbiguityResolutionAlgorithm.cpp | 16 +- .../python/acts/examples/reconstruction.py | 18 +- Examples/Python/src/AmbiguityResolution.cpp | 5 +- Examples/Scripts/Python/full_chain_odd.py | 9 +- Examples/Scripts/Python/full_chain_odd_LRT.py | 9 +- Examples/Scripts/Python/full_chain_test.py | 9 +- .../Json/src/AmbiguityConfigJsonConverter.cpp | 58 +++++- .../ScoreBasedAmbiguityResolutionTest.cpp | 9 +- 12 files changed, 193 insertions(+), 226 deletions(-) diff --git a/Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp b/Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp index 6ed24a71be8..60e81c0a8db 100644 --- a/Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp +++ b/Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp @@ -38,6 +38,7 @@ namespace Acts { /// enough hits /// 5) Remove tracks that are not good enough based on cuts Contains method for /// data preparations + class ScoreBasedAmbiguityResolution { public: /// @brief Detector configuration struct : contains the configuration for each detector @@ -50,11 +51,23 @@ class ScoreBasedAmbiguityResolution { int outliersScoreWeight = 0; int otherScoreWeight = 0; - std::size_t minHits = 0; + // the eta bins for the detector + std::vector etaBins = {-5, 5}; + + // the minimum number of hits for each eta bin + std::vector minHitsPerEta = {0}; + + // the maximum number of holes for each eta bin + std::vector maxHolesPerEta = {0}; + + // the maximum number of outliers for each eta bin + std::vector maxOutliersPerEta = {0}; + + // the maximum number of shared hits for each eta bin + std::vector maxSharedHitsPerEta = {0}; + std::size_t maxHits = 0; std::size_t maxHoles = 0; - std::size_t maxOutliers = 0; - std::size_t maxSharedHits = 0; /// if true, the shared hits are considered as bad hits for this detector bool sharedHitsFlag = false; @@ -63,12 +76,12 @@ class ScoreBasedAmbiguityResolution { /// a list of values from 0 to 1, the higher number of hits, higher value /// in the list is multiplied to ambuiguity score applied only if - /// useAmbiguityFunction is true + /// useAmbiguityScoring is true std::vector factorHits = {1.0}; /// a list of values from 0 to 1, the higher number of holes, lower value /// in the list is multiplied to ambuiguity score applied only if - /// useAmbiguityFunction is true + /// useAmbiguityScoring is true std::vector factorHoles = {1.0}; }; @@ -107,28 +120,21 @@ class ScoreBasedAmbiguityResolution { std::size_t maxSharedTracksPerMeasurement = 10; /// maximum number of shared hit per track std::size_t maxShared = 5; - - double pTMin = 0 * UnitConstants::GeV; - double pTMax = 1e5 * UnitConstants::GeV; - - double phiMin = -std::numbers::pi * UnitConstants::rad; - double phiMax = std::numbers::pi * UnitConstants::rad; - - double etaMin = -5; - double etaMax = 5; + /// minimum number of unshared hits per track + std::size_t minUnshared = 5; // if true, the ambiguity score is computed based on a different function. - bool useAmbiguityFunction = false; + bool useAmbiguityScoring = false; }; - /// @brief OptionalCuts struct : contains the optional cuts to be applied. + /// @brief Optionals struct: contains the optional cuts, weights and score to be applied. /// - /// The optional cuts,weights and score are used to remove tracks that are not - /// good enough, based on some criteria. Users are free to add their own cuts - /// with the help of this struct. + /// The default cuts and scoring has only a basic set of cuts and + /// score-modifiers. For more flexibility users can define custom cuts and + /// scores using this structure. template - struct OptionalCuts { - using OptionalFilter = std::function; + struct Optionals { + using OptionalCuts = std::function; using OptionalScoreModifier = std::function; @@ -137,10 +143,10 @@ class ScoreBasedAmbiguityResolution { const track_proxy_t&, const typename track_proxy_t::ConstTrackStateProxy&, TrackStateTypes&)>; - std::vector cuts = {}; + std::vector cuts = {}; std::vector weights = {}; - /// applied only if useAmbiguityFunction is true + /// applied only if useAmbiguityScoring is true std::vector scores = {}; std::vector hitSelections = {}; }; @@ -163,27 +169,37 @@ class ScoreBasedAmbiguityResolution { /// /// @param tracks is the input track container /// @param trackFeaturesVectors is the trackFeatures map from detector ID to trackFeatures - /// @param optionalCuts is the user defined optional cuts to be applied. + /// @param optionals is the user defined optional cuts to be applied. /// @return a vector of scores for each track template std::vector simpleScore( const track_container_t& tracks, const std::vector>& trackFeaturesVectors, - const OptionalCuts& - optionalCuts = {}) const; + const Optionals& optionals = + {}) const; /// Compute the score of each track based on the ambiguity function. /// /// @param tracks is the input track container /// @param trackFeaturesVectors is the trackFeatures map from detector ID to trackFeatures - /// @param optionalCuts is the user defined optional cuts to be applied. + /// @param optionals is the user defined optional cuts to be applied. /// @return a vector of scores for each track template std::vector ambiguityScore( const track_container_t& tracks, const std::vector>& trackFeaturesVectors, - const OptionalCuts& - optionalCuts = {}) const; + const Optionals& optionals = + {}) const; + + /// Rejects Tracks based on eta dependent cuts. + /// + /// @param detector is the detector configuration object + /// @param trackFeatures is the trackFeatures object for a specific detector + /// @param eta is the eta of the track + /// @return true if the track is rejected, false otherwise + bool etaBasedCuts(const DetectorConfig& detector, + const TrackFeatures& trackFeatures, + const double& eta) const; /// Remove hits that are not good enough for each track and removes tracks /// that have a score below a certain threshold or not enough hits. @@ -211,15 +227,15 @@ class ScoreBasedAmbiguityResolution { /// @param tracks is the input track container /// @param sourceLinkHash is the source links /// @param sourceLinkEquality is the equality function for the source links - /// @param optionalCuts is the optional cuts to be applied + /// @param optionals are the optional cuts and score modifiers to be applied /// @return a vector of IDs of the tracks we want to keep template std::vector solveAmbiguity( const track_container_t& tracks, source_link_hash_t sourceLinkHash, source_link_equality_t sourceLinkEquality, - const OptionalCuts& - optionalCuts = {}) const; + const Optionals& optionals = + {}) const; private: Config m_cfg; diff --git a/Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp b/Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp index 0ce0941c0fe..efd58819ab5 100644 --- a/Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp +++ b/Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp @@ -73,8 +73,8 @@ template std::vector Acts::ScoreBasedAmbiguityResolution::simpleScore( const track_container_t& tracks, const std::vector>& trackFeaturesVectors, - const OptionalCuts& - optionalCuts) const { + const Optionals& optionals) + const { std::vector trackScore; trackScore.reserve(tracks.size()); @@ -89,43 +89,10 @@ std::vector Acts::ScoreBasedAmbiguityResolution::simpleScore( // get the trackFeatures map for the track const auto& trackFeaturesVector = trackFeaturesVectors[iTrack]; double score = 1; - auto pT = Acts::VectorHelpers::perp(track.momentum()); auto eta = Acts::VectorHelpers::eta(track.momentum()); - auto phi = Acts::VectorHelpers::phi(track.momentum()); - // cuts on pT - if (pT < m_cfg.pTMin || pT > m_cfg.pTMax) { - score = 0; - iTrack++; - trackScore.push_back(score); - ACTS_DEBUG("Track: " << iTrack - << " has score = 0, due to pT cuts --- pT = " << pT); - continue; - } - - // cuts on phi - if (phi > m_cfg.phiMax || phi < m_cfg.phiMin) { - score = 0; - iTrack++; - trackScore.push_back(score); - ACTS_DEBUG("Track: " << iTrack - << " has score = 0, due to phi cuts --- phi = " - << phi); - continue; - } - - // cuts on eta - if (eta > m_cfg.etaMax || eta < m_cfg.etaMin) { - score = 0; - iTrack++; - trackScore.push_back(score); - ACTS_DEBUG("Track: " << iTrack - << " has score = 0, due to eta cuts --- eta = " - << eta); - continue; - } // cuts on optional cuts - for (const auto& cutFunction : optionalCuts.cuts) { + for (const auto& cutFunction : optionals.cuts) { if (cutFunction(track)) { score = 0; ACTS_DEBUG("Track: " << iTrack @@ -140,6 +107,7 @@ std::vector Acts::ScoreBasedAmbiguityResolution::simpleScore( ACTS_DEBUG("Track: " << iTrack << " score : " << score); continue; } + // Reject tracks which didn't pass the detector cuts. for (std::size_t detectorId = 0; detectorId < m_cfg.detectorConfigs.size(); detectorId++) { @@ -147,15 +115,14 @@ std::vector Acts::ScoreBasedAmbiguityResolution::simpleScore( const auto& trackFeatures = trackFeaturesVector[detectorId]; - ACTS_DEBUG("---> Found summary information"); - ACTS_DEBUG("---> Detector ID: " << detectorId); - ACTS_DEBUG("---> Number of hits: " << trackFeatures.nHits); - ACTS_DEBUG("---> Number of holes: " << trackFeatures.nHoles); - ACTS_DEBUG("---> Number of outliers: " << trackFeatures.nOutliers); + ACTS_VERBOSE("---> Found summary information"); + ACTS_VERBOSE("---> Detector ID: " << detectorId); + ACTS_VERBOSE("---> Number of hits: " << trackFeatures.nHits); + ACTS_VERBOSE("---> Number of holes: " << trackFeatures.nHoles); + ACTS_VERBOSE("---> Number of outliers: " << trackFeatures.nOutliers); - if ((trackFeatures.nHits < detector.minHits) || - (trackFeatures.nHoles > detector.maxHoles) || - (trackFeatures.nOutliers > detector.maxOutliers)) { + // eta based cuts + if (etaBasedCuts(detector, trackFeatures, eta)) { score = 0; ACTS_DEBUG("Track: " << iTrack << " has score = 0, due to detector cuts"); @@ -170,9 +137,7 @@ std::vector Acts::ScoreBasedAmbiguityResolution::simpleScore( continue; } - // real scoring starts here - // if the ambiguity scoring function is used, the score is processed with a - // different algorithm than the simple score. + // All cuts passed, now start scoring the track ACTS_VERBOSE("Using Simple Scoring function"); @@ -192,7 +157,7 @@ std::vector Acts::ScoreBasedAmbiguityResolution::simpleScore( } // Adding scores based on optional weights - for (const auto& weightFunction : optionalCuts.weights) { + for (const auto& weightFunction : optionals.weights) { weightFunction(track, score); } @@ -221,8 +186,8 @@ template std::vector Acts::ScoreBasedAmbiguityResolution::ambiguityScore( const track_container_t& tracks, const std::vector>& trackFeaturesVectors, - const OptionalCuts& - optionalCuts) const { + const Optionals& optionals) + const { std::vector trackScore; trackScore.reserve(tracks.size()); @@ -241,41 +206,9 @@ std::vector Acts::ScoreBasedAmbiguityResolution::ambiguityScore( double score = 1; auto pT = Acts::VectorHelpers::perp(track.momentum()); auto eta = Acts::VectorHelpers::eta(track.momentum()); - auto phi = Acts::VectorHelpers::phi(track.momentum()); - // cuts on pT - if (pT < m_cfg.pTMin || pT > m_cfg.pTMax) { - score = 0; - iTrack++; - trackScore.push_back(score); - ACTS_DEBUG("Track: " << iTrack - << " has score = 0, due to pT cuts --- pT = " << pT); - continue; - } - - // cuts on phi - if (phi > m_cfg.phiMax || phi < m_cfg.phiMin) { - score = 0; - iTrack++; - trackScore.push_back(score); - ACTS_DEBUG("Track: " << iTrack - << " has score = 0, due to phi cuts --- phi = " - << phi); - continue; - } - - // cuts on eta - if (eta > m_cfg.etaMax || eta < m_cfg.etaMin) { - score = 0; - iTrack++; - trackScore.push_back(score); - ACTS_DEBUG("Track: " << iTrack - << " has score = 0, due to eta cuts --- eta = " - << eta); - continue; - } // cuts on optional cuts - for (const auto& cutFunction : optionalCuts.cuts) { + for (const auto& cutFunction : optionals.cuts) { if (cutFunction(track)) { score = 0; ACTS_DEBUG("Track: " << iTrack @@ -290,6 +223,7 @@ std::vector Acts::ScoreBasedAmbiguityResolution::ambiguityScore( ACTS_DEBUG("Track: " << iTrack << " score : " << score); continue; } + // Reject tracks which didn't pass the detector cuts. for (std::size_t detectorId = 0; detectorId < m_cfg.detectorConfigs.size(); detectorId++) { @@ -297,15 +231,14 @@ std::vector Acts::ScoreBasedAmbiguityResolution::ambiguityScore( const auto& trackFeatures = trackFeaturesVector[detectorId]; - ACTS_DEBUG("---> Found summary information"); - ACTS_DEBUG("---> Detector ID: " << detectorId); - ACTS_DEBUG("---> Number of hits: " << trackFeatures.nHits); - ACTS_DEBUG("---> Number of holes: " << trackFeatures.nHoles); - ACTS_DEBUG("---> Number of outliers: " << trackFeatures.nOutliers); + ACTS_VERBOSE("---> Found summary information"); + ACTS_VERBOSE("---> Detector ID: " << detectorId); + ACTS_VERBOSE("---> Number of hits: " << trackFeatures.nHits); + ACTS_VERBOSE("---> Number of holes: " << trackFeatures.nHoles); + ACTS_VERBOSE("---> Number of outliers: " << trackFeatures.nOutliers); - if ((trackFeatures.nHits < detector.minHits) || - (trackFeatures.nHoles > detector.maxHoles) || - (trackFeatures.nOutliers > detector.maxOutliers)) { + // eta based cuts + if (etaBasedCuts(detector, trackFeatures, eta)) { score = 0; ACTS_DEBUG("Track: " << iTrack << " has score = 0, due to detector cuts"); @@ -320,6 +253,8 @@ std::vector Acts::ScoreBasedAmbiguityResolution::ambiguityScore( continue; } + // All cuts passed, now start scoring the track + // start with larger score for tracks with higher pT. score = std::log10(pT / UnitConstants::MeV) - 1.; // pT in GeV, hence 100 MeV is minimum and gets score = 1 @@ -357,7 +292,7 @@ std::vector Acts::ScoreBasedAmbiguityResolution::ambiguityScore( << " New score now: " << score); } - for (const auto& scoreFunction : optionalCuts.scores) { + for (const auto& scoreFunction : optionals.scores) { scoreFunction(track, score); } @@ -370,6 +305,7 @@ std::vector Acts::ScoreBasedAmbiguityResolution::ambiguityScore( << " is : " << fac << " New score now: " << score); } + iTrack++; // Add the score to the vector @@ -386,8 +322,8 @@ template Acts::ScoreBasedAmbiguityResolution::solveAmbiguity( const track_container_t& tracks, source_link_hash_t sourceLinkHash, source_link_equality_t sourceLinkEquality, - const OptionalCuts& - optionalCuts) const { + const Optionals& optionals) + const { ACTS_INFO("Number of tracks before Ambiguty Resolution: " << tracks.size()); // vector of trackFeaturesVectors. where each trackFeaturesVector contains the // number of hits/hole/outliers for each detector in a track. @@ -397,10 +333,10 @@ std::vector Acts::ScoreBasedAmbiguityResolution::solveAmbiguity( std::vector trackScore; trackScore.reserve(tracks.size()); - if (m_cfg.useAmbiguityFunction) { - trackScore = ambiguityScore(tracks, trackFeaturesVectors, optionalCuts); + if (m_cfg.useAmbiguityScoring) { + trackScore = ambiguityScore(tracks, trackFeaturesVectors, optionals); } else { - trackScore = simpleScore(tracks, trackFeaturesVectors, optionalCuts); + trackScore = simpleScore(tracks, trackFeaturesVectors, optionals); } auto MeasurementIndexMap = @@ -440,37 +376,18 @@ std::vector Acts::ScoreBasedAmbiguityResolution::solveAmbiguity( std::vector goodTracks; int cleanTrackIndex = 0; - auto optionalHitSelections = optionalCuts.hitSelections; + auto optionalHitSelections = optionals.hitSelections; // Loop over all the tracks in the container // For each track, check if the track has too many shared hits to be accepted. - // If the track is accepted, remove bad hits and check if the track has a - // score higher than the minimum score for shared tracks. - // If the track is good, add it to the goodTracks vector. + // If the track is good, add it to the goodTracks for (std::size_t iTrack = 0; const auto& track : tracks) { // Check if the track has too many shared hits to be accepted. - auto trackFeaturesVector = trackFeaturesVectors.at(iTrack); - bool trkCouldBeAccepted = true; - for (std::size_t detectorId = 0; detectorId < m_cfg.detectorConfigs.size(); - detectorId++) { - auto detector = m_cfg.detectorConfigs.at(detectorId); - if (trackFeaturesVector[detectorId].nSharedHits > - detector.maxSharedHits) { - trkCouldBeAccepted = false; - break; - } - } - if (!trkCouldBeAccepted) { - iTrack++; - continue; - } - - trkCouldBeAccepted = getCleanedOutTracks( - track, trackScore[iTrack], measurementsPerTrackVector[iTrack], - nTracksPerMeasurement, optionalHitSelections); - if (trkCouldBeAccepted) { + if (getCleanedOutTracks(track, trackScore[iTrack], + measurementsPerTrackVector[iTrack], + nTracksPerMeasurement, optionalHitSelections)) { cleanTrackIndex++; - if (trackScore[iTrack] >= m_cfg.minScore) { + if (trackScore[iTrack] > m_cfg.minScore) { goodTracks.push_back(track.index()); } } @@ -541,6 +458,18 @@ bool Acts::ScoreBasedAmbiguityResolution::getCleanedOutTracks( ACTS_DEBUG("Track state has no reference surface"); continue; } + + std::size_t ivolume = ts.referenceSurface().geometryId().volume(); + auto volume_it = m_cfg.volumeMap.find(ivolume); + if (volume_it == m_cfg.volumeMap.end()) { + ACTS_ERROR("Volume " << ivolume << " not found in the volume map"); + continue; + } + + std::size_t detectorID = volume_it->second; + + const auto& detector = m_cfg.detectorConfigs.at(detectorID); + measurement = measurementsPerTrack[index]; auto it = nTracksPerMeasurement.find(measurement); @@ -573,7 +502,8 @@ bool Acts::ScoreBasedAmbiguityResolution::getCleanedOutTracks( else { ACTS_DEBUG("Try to recover shared hit "); if (nTracksShared <= m_cfg.maxSharedTracksPerMeasurement && - trackScore > m_cfg.minScoreSharedTracks) { + trackScore > m_cfg.minScoreSharedTracks && + !detector.sharedHitsFlag) { ACTS_DEBUG("Accepted hit shared with " << nTracksShared << " tracks"); newMeasurementsPerTrack.push_back(measurement); nshared++; @@ -585,7 +515,7 @@ bool Acts::ScoreBasedAmbiguityResolution::getCleanedOutTracks( } } // Check if the track has enough hits to be accepted. - if (newMeasurementsPerTrack.size() < 3) { + if (newMeasurementsPerTrack.size() < m_cfg.minUnshared) { return false; } else { return true; diff --git a/Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp b/Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp index c7e47f41046..f1ffc74ce15 100644 --- a/Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp +++ b/Core/src/AmbiguityResolution/ScoreBasedAmbiguityResolution.cpp @@ -7,3 +7,19 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. #include "Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp" + +bool Acts::ScoreBasedAmbiguityResolution::etaBasedCuts( + const DetectorConfig& detector, const TrackFeatures& trackFeatures, + const double& eta) const { + const auto& etaBins = detector.etaBins; + + auto it = std::ranges::upper_bound(etaBins, eta); + if (it == etaBins.begin() || it == etaBins.end()) { + return false; // eta out of range + } + std::size_t etaBin = std::distance(etaBins.begin(), it) - 1; + return (trackFeatures.nHits < detector.minHitsPerEta[etaBin] || + trackFeatures.nHoles > detector.maxHolesPerEta[etaBin] || + trackFeatures.nOutliers > detector.maxOutliersPerEta[etaBin] || + trackFeatures.nSharedHits > detector.maxSharedHitsPerEta[etaBin]); +} diff --git a/Examples/Algorithms/AmbiguityResolution/include/ActsExamples/AmbiguityResolution/ScoreBasedAmbiguityResolutionAlgorithm.hpp b/Examples/Algorithms/AmbiguityResolution/include/ActsExamples/AmbiguityResolution/ScoreBasedAmbiguityResolutionAlgorithm.hpp index b63100f8751..bc92b2d656f 100644 --- a/Examples/Algorithms/AmbiguityResolution/include/ActsExamples/AmbiguityResolution/ScoreBasedAmbiguityResolutionAlgorithm.hpp +++ b/Examples/Algorithms/AmbiguityResolution/include/ActsExamples/AmbiguityResolution/ScoreBasedAmbiguityResolutionAlgorithm.hpp @@ -54,16 +54,10 @@ class ScoreBasedAmbiguityResolutionAlgorithm final : public IAlgorithm { // maximum number of shared hit per track std::size_t maxShared = 5; - double pTMin = 0 * Acts::UnitConstants::GeV; - double pTMax = 1e5 * Acts::UnitConstants::GeV; + // minimum number of unshared hits per track + std::size_t minUnshared = 5; - double phiMin = -std::numbers::pi * Acts::UnitConstants::rad; - double phiMax = std::numbers::pi * Acts::UnitConstants::rad; - - double etaMin = -5; - double etaMax = 5; - - bool useAmbiguityFunction = false; + bool useAmbiguityScoring = false; }; /// Construct the ambiguity resolution algorithm. diff --git a/Examples/Algorithms/AmbiguityResolution/src/ScoreBasedAmbiguityResolutionAlgorithm.cpp b/Examples/Algorithms/AmbiguityResolution/src/ScoreBasedAmbiguityResolutionAlgorithm.cpp index b1a6e251dba..2da5cd4dd78 100644 --- a/Examples/Algorithms/AmbiguityResolution/src/ScoreBasedAmbiguityResolutionAlgorithm.cpp +++ b/Examples/Algorithms/AmbiguityResolution/src/ScoreBasedAmbiguityResolutionAlgorithm.cpp @@ -44,13 +44,8 @@ Acts::ScoreBasedAmbiguityResolution::Config transformConfig( result.minScoreSharedTracks = cfg.minScoreSharedTracks; result.maxSharedTracksPerMeasurement = cfg.maxSharedTracksPerMeasurement; result.maxShared = cfg.maxShared; - result.pTMin = cfg.pTMin; - result.pTMax = cfg.pTMax; - result.phiMin = cfg.phiMin; - result.phiMax = cfg.phiMax; - result.etaMin = cfg.etaMin; - result.etaMax = cfg.etaMax; - result.useAmbiguityFunction = cfg.useAmbiguityFunction; + result.minUnshared = cfg.minUnshared; + result.useAmbiguityScoring = cfg.useAmbiguityScoring; return result; } @@ -116,11 +111,10 @@ ActsExamples::ScoreBasedAmbiguityResolutionAlgorithm::execute( const auto& tracks = m_inputTracks(ctx); // Read input data ACTS_VERBOSE("Number of input tracks: " << tracks.size()); - Acts::ScoreBasedAmbiguityResolution::OptionalCuts - optionalCuts; - optionalCuts.cuts.push_back(doubleHolesFilter); + Acts::ScoreBasedAmbiguityResolution::Optionals optionals; + optionals.cuts.push_back(doubleHolesFilter); std::vector goodTracks = m_ambi.solveAmbiguity( - tracks, &sourceLinkHash, &sourceLinkEquality, optionalCuts); + tracks, &sourceLinkHash, &sourceLinkEquality, optionals); // Prepare the output track collection from the IDs TrackContainer solvedTracks{std::make_shared(), std::make_shared()}; diff --git a/Examples/Python/python/acts/examples/reconstruction.py b/Examples/Python/python/acts/examples/reconstruction.py index 915d19baf4f..cee5ac47288 100644 --- a/Examples/Python/python/acts/examples/reconstruction.py +++ b/Examples/Python/python/acts/examples/reconstruction.py @@ -216,16 +216,11 @@ def trackSelectorDefaultKWArgs(c): "minScore", "minScoreSharedTracks", "maxShared", + "minUnshared", "maxSharedTracksPerMeasurement", - "pTMax", - "pTMin", - "phiMax", - "phiMin", - "etaMax", - "etaMin", - "useAmbiguityFunction", + "useAmbiguityScoring", ], - defaults=[None] * 11, + defaults=[None] * 6, ) AmbiguityResolutionMLConfig = namedtuple( @@ -1923,12 +1918,9 @@ def addScoreBasedAmbiguityResolution( minScore=config.minScore, minScoreSharedTracks=config.minScoreSharedTracks, maxShared=config.maxShared, + minUnshared=config.minUnshared, maxSharedTracksPerMeasurement=config.maxSharedTracksPerMeasurement, - phiMax=config.phiMax, - phiMin=config.phiMin, - etaMax=config.etaMax, - etaMin=config.etaMin, - useAmbiguityFunction=config.useAmbiguityFunction, + useAmbiguityScoring=config.useAmbiguityScoring, ), ) s.addAlgorithm(algScoreBased) diff --git a/Examples/Python/src/AmbiguityResolution.cpp b/Examples/Python/src/AmbiguityResolution.cpp index 2075a522609..c45a6f11fd2 100644 --- a/Examples/Python/src/AmbiguityResolution.cpp +++ b/Examples/Python/src/AmbiguityResolution.cpp @@ -33,9 +33,8 @@ void addAmbiguityResolution(Context& ctx) { ACTS_PYTHON_DECLARE_ALGORITHM( ActsExamples::ScoreBasedAmbiguityResolutionAlgorithm, mex, "ScoreBasedAmbiguityResolutionAlgorithm", inputTracks, configFile, - outputTracks, minScore, minScoreSharedTracks, maxShared, - maxSharedTracksPerMeasurement, pTMin, pTMax, phiMin, phiMax, etaMin, - etaMax, useAmbiguityFunction); + outputTracks, minScore, minScoreSharedTracks, maxShared, minUnshared, + maxSharedTracksPerMeasurement, useAmbiguityScoring); } } // namespace Acts::Python diff --git a/Examples/Scripts/Python/full_chain_odd.py b/Examples/Scripts/Python/full_chain_odd.py index 50fa2465e38..0b30e5e42be 100755 --- a/Examples/Scripts/Python/full_chain_odd.py +++ b/Examples/Scripts/Python/full_chain_odd.py @@ -414,14 +414,9 @@ minScore=0, minScoreSharedTracks=1, maxShared=2, + minUnshared=3, maxSharedTracksPerMeasurement=2, - pTMax=1400, - pTMin=0.5, - phiMax=math.pi, - phiMin=-math.pi, - etaMax=4, - etaMin=-4, - useAmbiguityFunction=False, + useAmbiguityScoring=False, ), outputDirRoot=outputDir if args.output_root else None, outputDirCsv=outputDir if args.output_csv else None, diff --git a/Examples/Scripts/Python/full_chain_odd_LRT.py b/Examples/Scripts/Python/full_chain_odd_LRT.py index fde8b3b418c..cc51d83b2e4 100644 --- a/Examples/Scripts/Python/full_chain_odd_LRT.py +++ b/Examples/Scripts/Python/full_chain_odd_LRT.py @@ -417,14 +417,9 @@ minScore=0, minScoreSharedTracks=1, maxShared=2, + minUnshared=3, maxSharedTracksPerMeasurement=2, - pTMax=1400, - pTMin=0.5, - phiMax=math.pi, - phiMin=-math.pi, - etaMax=4, - etaMin=-4, - useAmbiguityFunction=False, + useAmbiguityScoring=False, ), outputDirRoot=outputDir if args.output_root else None, outputDirCsv=outputDir if args.output_csv else None, diff --git a/Examples/Scripts/Python/full_chain_test.py b/Examples/Scripts/Python/full_chain_test.py index 492c823cb5f..b34e0e061c8 100755 --- a/Examples/Scripts/Python/full_chain_test.py +++ b/Examples/Scripts/Python/full_chain_test.py @@ -727,14 +727,9 @@ def full_chain(args): minScore=0, minScoreSharedTracks=1, maxShared=2, + minUnshared=3, maxSharedTracksPerMeasurement=2, - pTMax=1400, - pTMin=0.5, - phiMax=math.pi, - phiMin=-math.pi, - etaMax=4, - etaMin=-4, - useAmbiguityFunction=False, + useAmbiguityScoring=False, ), ambiVolumeFile=args.ambi_config, **writeCovMat, diff --git a/Plugins/Json/src/AmbiguityConfigJsonConverter.cpp b/Plugins/Json/src/AmbiguityConfigJsonConverter.cpp index ded95d8c122..489d2f713b5 100644 --- a/Plugins/Json/src/AmbiguityConfigJsonConverter.cpp +++ b/Plugins/Json/src/AmbiguityConfigJsonConverter.cpp @@ -11,6 +11,23 @@ #include "Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp" #include "Acts/Plugins/Json/ActsJson.hpp" +void initializeEtaVector(std::vector& target, + const std::vector& source, + std::size_t etaBinSize) { + if (source.size() == etaBinSize - 1) { + // Directly copy if sizes match + target = source; + } else if (source.size() == 1) { + // Resize target to the required size + target.resize(etaBinSize - 1); + // Fill with the single value from source + std::fill(target.begin(), target.end(), source[0]); + } else { + throw std::invalid_argument("Invalid cuts size. Expected 1 or " + + std::to_string(etaBinSize - 1) + ", got " + + std::to_string(source.size())); + } +} namespace Acts { void from_json(const nlohmann::json& j, ConfigPair& p) { @@ -27,12 +44,6 @@ void from_json(const nlohmann::json& j, ConfigPair& p) { detectorConfig.outliersScoreWeight = value["outliersScoreWeight"]; detectorConfig.otherScoreWeight = value["otherScoreWeight"]; - detectorConfig.minHits = value["minHits"]; - detectorConfig.maxHits = value["maxHits"]; - detectorConfig.maxHoles = value["maxHoles"]; - detectorConfig.maxOutliers = value["maxOutliers"]; - detectorConfig.maxSharedHits = value["maxSharedHits"]; - detectorConfig.sharedHitsFlag = value["sharedHitsFlag"]; const std::vector& goodHits = value["goodHits"]; @@ -41,10 +52,43 @@ void from_json(const nlohmann::json& j, ConfigPair& p) { const std::vector& fakeHits = value["fakeHits"]; const std::vector& fakeHoles = value["fakeHoles"]; + const std::vector& etaBins = value["etaBins"]; + + // Validate eta bins + if (!etaBins.empty()) { + // Verify monotonically increasing eta bins + if (!std::is_sorted(etaBins.begin(), etaBins.end())) { + throw std::invalid_argument( + "Eta bins must be monotonically increasing"); + } + + detectorConfig.etaBins = etaBins; + } + + const std::vector& minHitsPerEta = value["minHitsPerEta"]; + initializeEtaVector(detectorConfig.minHitsPerEta, minHitsPerEta, + detectorConfig.etaBins.size()); + + const std::vector& maxHolesPerEta = value["maxHolesPerEta"]; + initializeEtaVector(detectorConfig.maxHolesPerEta, maxHolesPerEta, + detectorConfig.etaBins.size()); + + const std::vector& maxOutliersPerEta = + value["maxOutliersPerEta"]; + initializeEtaVector(detectorConfig.maxOutliersPerEta, maxOutliersPerEta, + detectorConfig.etaBins.size()); + + const std::vector& maxSharedHitsPerEta = + value["maxSharedHitsPerEta"]; + initializeEtaVector(detectorConfig.maxSharedHitsPerEta, maxSharedHitsPerEta, + detectorConfig.etaBins.size()); + if (goodHits.size() != fakeHits.size()) { throw std::invalid_argument("goodHits and FakeHits size mismatch"); } + detectorConfig.factorHits = {}; + detectorConfig.maxHits = goodHits.size() - 1; for (std::size_t i = 0; i < goodHits.size(); i++) { detectorConfig.factorHits.push_back(goodHits[i] / fakeHits[i]); } @@ -53,6 +97,8 @@ void from_json(const nlohmann::json& j, ConfigPair& p) { throw std::invalid_argument("goodHoles and FakeHoles size mismatch"); } + detectorConfig.factorHoles = {}; + detectorConfig.maxHoles = goodHoles.size() - 1; for (std::size_t i = 0; i < goodHoles.size(); i++) { detectorConfig.factorHoles.push_back(goodHoles[i] / fakeHoles[i]); } diff --git a/Tests/UnitTests/Core/AmbiguityResolution/ScoreBasedAmbiguityResolutionTest.cpp b/Tests/UnitTests/Core/AmbiguityResolution/ScoreBasedAmbiguityResolutionTest.cpp index 55beab4ca2f..81eef868ff3 100644 --- a/Tests/UnitTests/Core/AmbiguityResolution/ScoreBasedAmbiguityResolutionTest.cpp +++ b/Tests/UnitTests/Core/AmbiguityResolution/ScoreBasedAmbiguityResolutionTest.cpp @@ -48,14 +48,9 @@ struct Fixture { config.minScore = 0; config.minScoreSharedTracks = 100; config.maxShared = 5; + config.minUnshared = 3; config.maxSharedTracksPerMeasurement = 10; - config.phiMax = 3.14; - config.phiMin = -3.14; - config.etaMax = 2.7; - config.etaMin = -2.7; - config.pTMax = 1400; - config.pTMin = 0.5; - config.useAmbiguityFunction = false; + config.useAmbiguityScoring = false; } ~Fixture() = default; From d15f9a32dad6a9c84950e9f1dac8a189aa4f885f Mon Sep 17 00:00:00 2001 From: Noemi Calace <58694235+noemina@users.noreply.github.com> Date: Mon, 24 Feb 2025 19:19:35 +0100 Subject: [PATCH 14/14] feat: Adding a few changes to test seeding algorithm (#4075) This PR includes a few changes to facilitate performance studies for the seeding algorithm @pbutti, @benjaminhuth ## Summary by CodeRabbit - **New Features** - Enhanced seeding capability with new configuration options for additional filtering and customizable seed naming. - Introduced a demonstration script that simplifies data reading and seeding, now featuring improved debug logging. - **Chores** - Optimized tracking criteria by refining selection thresholds and updating filtering parameters for improved performance. - Added debug logging to track space point creation for better visibility during processing. - Introduced a new parameter for occupancy configuration in the seeding algorithm. - Updated filtering criteria in the reader to focus on space points. --- .../TrackFinding/SeedingAlgorithm.hpp | 4 +- Examples/Io/Root/src/RootAthenaDumpReader.cpp | 2 + Examples/Python/python/acts/examples/itk.py | 1 + .../python/acts/examples/reconstruction.py | 3 +- Examples/Python/src/Input.cpp | 6 +- Examples/Scripts/Python/reader_and_seeding.py | 59 +++++++++++++++++++ 6 files changed, 69 insertions(+), 6 deletions(-) create mode 100644 Examples/Scripts/Python/reader_and_seeding.py diff --git a/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp b/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp index 73bb81d5d7f..e702b36b8c5 100644 --- a/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp +++ b/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/SeedingAlgorithm.hpp @@ -111,7 +111,7 @@ class SeedingAlgorithm final : public IAlgorithm { WriteDataHandle m_outputSeeds{this, "OutputSeeds"}; static inline bool itkFastTrackingCuts(float bottomRadius, float cotTheta) { - static float rMin = 50.; + static float rMin = 45.; static float cotThetaMax = 1.5; if (bottomRadius < rMin && @@ -125,7 +125,7 @@ class SeedingAlgorithm final : public IAlgorithm { // At small r we remove points beyond |z| > 200. float r = sp.radius(); float zabs = std::abs(sp.z()); - if (zabs > 200. && r < 50.) { + if (zabs > 200. && r < 45.) { return false; } diff --git a/Examples/Io/Root/src/RootAthenaDumpReader.cpp b/Examples/Io/Root/src/RootAthenaDumpReader.cpp index b791dbf2eba..e4a147d12c7 100644 --- a/Examples/Io/Root/src/RootAthenaDumpReader.cpp +++ b/Examples/Io/Root/src/RootAthenaDumpReader.cpp @@ -626,6 +626,8 @@ RootAthenaDumpReader::readSpacepoints( ACTS_DEBUG("Created " << spacePoints.size() << " overall space points"); ACTS_DEBUG("Created " << pixelSpacePoints.size() << " " << " pixel space points"); + ACTS_DEBUG("Created " << stripSpacePoints.size() << " " + << " strip space points"); return {spacePoints, pixelSpacePoints, stripSpacePoints}; } diff --git a/Examples/Python/python/acts/examples/itk.py b/Examples/Python/python/acts/examples/itk.py index 0141e367ee9..31a6116fadf 100644 --- a/Examples/Python/python/acts/examples/itk.py +++ b/Examples/Python/python/acts/examples/itk.py @@ -581,6 +581,7 @@ def itkSeedingAlgConfig( zBinNeighborsTop=zBinNeighborsTop, zBinNeighborsBottom=zBinNeighborsBottom, numPhiNeighbors=numPhiNeighbors, + useExtraCuts=highOccupancyConfig, ) return ( diff --git a/Examples/Python/python/acts/examples/reconstruction.py b/Examples/Python/python/acts/examples/reconstruction.py index cee5ac47288..227b53813ba 100644 --- a/Examples/Python/python/acts/examples/reconstruction.py +++ b/Examples/Python/python/acts/examples/reconstruction.py @@ -636,6 +636,7 @@ def addStandardSeeding( seedFilterConfigArg: SeedFilterConfigArg, spacePointGridConfigArg: SpacePointGridConfigArg, logLevel: acts.logging.Level = None, + outputSeeds: str = "seeds", ): """adds standard seeding For parameters description see addSeeding @@ -762,7 +763,7 @@ def addStandardSeeding( seedingAlg = acts.examples.SeedingAlgorithm( level=logLevel, inputSpacePoints=[spacePoints], - outputSeeds="seeds", + outputSeeds=outputSeeds, **acts.examples.defaultKWArgs( allowSeparateRMax=seedingAlgorithmConfigArg.allowSeparateRMax, zBinNeighborsTop=seedingAlgorithmConfigArg.zBinNeighborsTop, diff --git a/Examples/Python/src/Input.cpp b/Examples/Python/src/Input.cpp index 60e0424b208..3e2bf371608 100644 --- a/Examples/Python/src/Input.cpp +++ b/Examples/Python/src/Input.cpp @@ -100,9 +100,9 @@ void addInput(Context& ctx) { ActsExamples::RootAthenaDumpReader, mex, "RootAthenaDumpReader", treename, inputfile, outputMeasurements, outputPixelSpacePoints, outputStripSpacePoints, outputSpacePoints, outputClusters, - outputMeasurementParticlesMap, outputParticles, onlyPassedParticles, - skipOverlapSPsPhi, skipOverlapSPsEta, geometryIdMap, trackingGeometry, - absBoundaryTolerance); + outputMeasurementParticlesMap, outputParticles, onlySpacepoints, + onlyPassedParticles, skipOverlapSPsPhi, skipOverlapSPsEta, geometryIdMap, + trackingGeometry, absBoundaryTolerance); #ifdef WITH_GEOMODEL_PLUGIN ACTS_PYTHON_DECLARE_READER(ActsExamples::RootAthenaDumpGeoIdCollector, mex, diff --git a/Examples/Scripts/Python/reader_and_seeding.py b/Examples/Scripts/Python/reader_and_seeding.py new file mode 100644 index 00000000000..62fd9c7e8af --- /dev/null +++ b/Examples/Scripts/Python/reader_and_seeding.py @@ -0,0 +1,59 @@ +import sys +from pathlib import Path + +import acts.examples +import acts + +from acts.examples import ( + CsvSpacePointReader, + TrackParamsEstimationAlgorithm, + SeedingPerformanceWriter, +) +from acts.examples.reconstruction import ( + addStandardSeeding, +) + +from acts.examples.itk import itkSeedingAlgConfig, InputSpacePointsType + + +s = acts.examples.Sequencer(events=1, numThreads=1, outputDir="output") + +# loggingLevel = acts.logging.INFO +loggingLevel = acts.logging.DEBUG + +s.addReader( + acts.examples.RootAthenaDumpReader( + level=loggingLevel, + treename="GNN4ITk", + inputfile="Dump_GNN4Itk.root", + onlySpacepoints=True, + outputPixelSpacePoints="pixel_spacepoints", + outputStripSpacePoints="strip_spacepoints", + outputSpacePoints="spacepoints", + ) +) + + +# run pixel seeding +seeding_pixel = addStandardSeeding( + s, + "pixel_spacepoints", + *acts.examples.itk.itkSeedingAlgConfig( + InputSpacePointsType.PixelSpacePoints, highOccupancyConfig=True + ), + logLevel=loggingLevel, + outputSeeds="pixel_seeds" +) + + +# run strip seeding +seeding_strip = addStandardSeeding( + s, + "strip_spacepoints", + *acts.examples.itk.itkSeedingAlgConfig(InputSpacePointsType.StripSpacePoints), + logLevel=acts.logging.DEBUG, + outputSeeds="strip_seeds" +) + + +s.run()