From 11f59e35f3ec0a36152cdd5cf893c643621c9b11 Mon Sep 17 00:00:00 2001 From: Luis Falda Coelho <56648068+LuisFelipeCoelho@users.noreply.github.com> Date: Tue, 1 Mar 2022 18:10:48 +0100 Subject: [PATCH 01/16] fix: Remove rMinMiddleSP and rMaxMiddleSP in the SeedFinder config (#1175) `rMinMiddleSP` and `rMaxMiddleSP` do not need to be configured in the SeedFinder config, so this PR removes them --- Core/include/Acts/Seeding/SeedfinderConfig.hpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/Core/include/Acts/Seeding/SeedfinderConfig.hpp b/Core/include/Acts/Seeding/SeedfinderConfig.hpp index d27fbb6145e..23e0ecf9950 100644 --- a/Core/include/Acts/Seeding/SeedfinderConfig.hpp +++ b/Core/include/Acts/Seeding/SeedfinderConfig.hpp @@ -55,8 +55,6 @@ struct SeedfinderConfig { std::vector> rRangeMiddleSP; bool useVariableMiddleSPRange = false; float deltaRMiddleSPRange = 10.; - float rMinMiddleSP; - float rMaxMiddleSP; // seed confirmation bool seedConfirmation = false; From 3ee4e2e90ad22f1f7e4436516b3a19c8e32d4cc4 Mon Sep 17 00:00:00 2001 From: Luis Falda Coelho <56648068+LuisFelipeCoelho@users.noreply.github.com> Date: Wed, 2 Mar 2022 10:19:33 +0100 Subject: [PATCH 02/16] fix: Fixing units in SeedFinderConfig (#1176) This PR fixes the units of `deltaRMiddleSPRange` to external ACTS units and adds `deltaRMiddleSPRange`, `deltaRMinTopSP`, `deltaRMaxTopSP`, `deltaRMinBottomSP` and `deltaRMaxBottomSP` to the `toInternalUnits` method. --- Core/include/Acts/Seeding/SeedfinderConfig.hpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Core/include/Acts/Seeding/SeedfinderConfig.hpp b/Core/include/Acts/Seeding/SeedfinderConfig.hpp index 23e0ecf9950..1cc36bffa2d 100644 --- a/Core/include/Acts/Seeding/SeedfinderConfig.hpp +++ b/Core/include/Acts/Seeding/SeedfinderConfig.hpp @@ -54,7 +54,7 @@ struct SeedfinderConfig { // radial range for middle SP std::vector> rRangeMiddleSP; bool useVariableMiddleSPRange = false; - float deltaRMiddleSPRange = 10.; + float deltaRMiddleSPRange = 10. * Acts::UnitConstants::mm; // seed confirmation bool seedConfirmation = false; @@ -146,6 +146,11 @@ struct SeedfinderConfig { config.minPt /= 1_MeV; config.deltaRMin /= 1_mm; config.deltaRMax /= 1_mm; + config.deltaRMinTopSP /= 1_mm; + config.deltaRMaxTopSP /= 1_mm; + config.deltaRMinBottomSP /= 1_mm; + config.deltaRMaxBottomSP /= 1_mm; + config.deltaRMiddleSPRange /= 1_mm; config.impactMax /= 1_mm; config.maxPtScattering /= 1_MeV; // correct? config.collisionRegionMin /= 1_mm; From 988e2ca8595ac1828ea3a57432d7bff743f77caf Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Wed, 2 Mar 2022 18:10:57 +0100 Subject: [PATCH 03/16] fix: Assertion failure in propagation example (#1180) This was for some reason using matrix access with two arguments to set elements of a vector (the diagonal of a matrix). Switched to use single component element access. --- .../Propagation/PropagationOptions.hpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/Examples/Algorithms/Propagation/include/ActsExamples/Propagation/PropagationOptions.hpp b/Examples/Algorithms/Propagation/include/ActsExamples/Propagation/PropagationOptions.hpp index f58e4b8fa20..036c7f402a4 100644 --- a/Examples/Algorithms/Propagation/include/ActsExamples/Propagation/PropagationOptions.hpp +++ b/Examples/Algorithms/Propagation/include/ActsExamples/Propagation/PropagationOptions.hpp @@ -134,23 +134,17 @@ inline ActsExamples::PropagationAlgorithm::Config readPropagationConfig( /// Set the covariance transport to true pAlgConfig.covarianceTransport = true; /// Set the covariance matrix - pAlgConfig.covariances(Acts::BoundIndices::eBoundLoc0, - Acts::BoundIndices::eBoundLoc0) = + pAlgConfig.covariances(Acts::BoundIndices::eBoundLoc0) = pAlgConfig.d0Sigma * pAlgConfig.d0Sigma; - pAlgConfig.covariances(Acts::BoundIndices::eBoundLoc1, - Acts::BoundIndices::eBoundLoc1) = + pAlgConfig.covariances(Acts::BoundIndices::eBoundLoc1) = pAlgConfig.z0Sigma * pAlgConfig.z0Sigma; - pAlgConfig.covariances(Acts::BoundIndices::eBoundPhi, - Acts::BoundIndices::eBoundPhi) = + pAlgConfig.covariances(Acts::BoundIndices::eBoundPhi) = pAlgConfig.phiSigma * pAlgConfig.phiSigma; - pAlgConfig.covariances(Acts::BoundIndices::eBoundTheta, - Acts::BoundIndices::eBoundTheta) = + pAlgConfig.covariances(Acts::BoundIndices::eBoundTheta) = pAlgConfig.thetaSigma * pAlgConfig.thetaSigma; - pAlgConfig.covariances(Acts::BoundIndices::eBoundQOverP, - Acts::BoundIndices::eBoundQOverP) = + pAlgConfig.covariances(Acts::BoundIndices::eBoundQOverP) = pAlgConfig.qpSigma * pAlgConfig.qpSigma; - pAlgConfig.covariances(Acts::BoundIndices::eBoundTime, - Acts::BoundIndices::eBoundTime) = + pAlgConfig.covariances(Acts::BoundIndices::eBoundTime) = pAlgConfig.tSigma * pAlgConfig.tSigma; // Only if they are properly defined, assign off-diagonals From 40d6e793c5d84c3007acfb3fc88653e5370fcf8f Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Thu, 3 Mar 2022 16:05:24 +0100 Subject: [PATCH 04/16] build: Allow forcing assertions (#1182) In the CI, we currently run the examples in Release only, which adds the `-DNDEBUG` flag and consequently disables `assert`. We can't run these in the CI with Debug, since that implies `-O0` and runs forever. This PR adds a new CMake option `ACTS_FORCE_ASSERTIONS` which defaults to `OFF`. If enabled, this will prepend a system include directory `cmake/assert_include` to all compilation units, which contains a wrapper around `cassert`, `Eigen/{Core,Dense,Geometry}` which enables `assert` (and `eigen_assert`) regardless of `NDEBUG`. I verified that this would have triggered the assertion from #1180 for example. The CI jobs are configured to run with `ACTS_FORCE_ASSERTIONS=ON`. --- .github/workflows/builds.yml | 4 ++++ CMakeLists.txt | 7 +++++++ cmake/assert_include/Eigen/Core | 2 ++ cmake/assert_include/Eigen/Dense | 2 ++ cmake/assert_include/Eigen/Geometry | 2 ++ cmake/assert_include/cassert | 9 +++++++++ docs/getting_started.md | 1 + 7 files changed, 27 insertions(+) create mode 100644 cmake/assert_include/Eigen/Core create mode 100644 cmake/assert_include/Eigen/Dense create mode 100644 cmake/assert_include/Eigen/Geometry create mode 100644 cmake/assert_include/cassert diff --git a/.github/workflows/builds.yml b/.github/workflows/builds.yml index 4ca36fc86d0..4255e992109 100644 --- a/.github/workflows/builds.yml +++ b/.github/workflows/builds.yml @@ -53,6 +53,7 @@ jobs: -DACTS_BUILD_EVERYTHING=ON -DACTS_BUILD_EXAMPLES_PYTHON_BINDINGS=ON -DACTS_LOG_FAILURE_THRESHOLD=WARNING + -DACTS_BUILD_ASSERTIONS=ON - name: Build run: ${SETUP} && cmake --build build -- - name: Unit tests @@ -129,6 +130,7 @@ jobs: -DACTS_BUILD_UNITTESTS=ON -DACTS_BUILD_INTEGRATIONTESTS=ON -DACTS_LOG_FAILURE_THRESHOLD=WARNING + -DACTS_BUILD_ASSERTIONS=ON -DACTS_USE_SYSTEM_BOOST=OFF -DACTS_USE_SYSTEM_EIGEN3=OFF -DACTS_BUILD_PLUGIN_JSON=ON @@ -152,6 +154,7 @@ jobs: -DCMAKE_CXX_FLAGS=-Werror -DCMAKE_INSTALL_PREFIX="${INSTALL_DIR}" -DACTS_LOG_FAILURE_THRESHOLD=WARNING + -DACTS_BUILD_ASSERTIONS=ON -DACTS_USE_SYSTEM_BOOST=OFF -DACTS_USE_SYSTEM_EIGEN3=OFF -DACTS_BUILD_EXAMPLES=ON @@ -204,6 +207,7 @@ jobs: -DCMAKE_PREFIX_PATH=/usr/local/acts -DACTS_BUILD_EVERYTHING=ON -DACTS_LOG_FAILURE_THRESHOLD=WARNING + -DACTS_BUILD_ASSERTIONS=ON - name: Build run: cmake --build build -- - name: Unit tests diff --git a/CMakeLists.txt b/CMakeLists.txt index e2a6ceea806..c8e9c1dc199 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -25,6 +25,7 @@ option(ACTS_BUILD_EVERYTHING "Build with most options enabled (except HepMC3 and # core related options set(ACTS_PARAMETER_DEFINITIONS_HEADER "" CACHE FILEPATH "Use a different (track) parameter definitions header") set(ACTS_LOG_FAILURE_THRESHOLD "" CACHE STRING "Log level above which an exception should be automatically thrown") +option(ACTS_FORCE_ASSERTIONS "Force assertions regardless of build type" OFF) # plugins related options option(ACTS_BUILD_PLUGIN_AUTODIFF "Build the autodiff plugin" OFF) option(ACTS_USE_SYSTEM_AUTODIFF "Use autodiff provided by the system instead of the bundled version" OFF) @@ -126,6 +127,12 @@ include(ActsComponentsHelpers) # handle components via add_..._if commands set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/${CMAKE_INSTALL_BINDIR}") set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR}") +# This needs to happen before we set up any targets +if(ACTS_FORCE_ASSERTIONS) + message(STATUS "Injecting headers to force assertions. This can have side-effects, USE WITH CAUTION!") + include_directories(BEFORE SYSTEM ${CMAKE_CURRENT_SOURCE_DIR}/cmake/assert_include) +endif() + # minimal dependency versions. they are defined here in a single place so # they can be easily upgraded, although they might not be used if the # dependency is included via `add_subdirectory(...)`. diff --git a/cmake/assert_include/Eigen/Core b/cmake/assert_include/Eigen/Core new file mode 100644 index 00000000000..158f2d9087e --- /dev/null +++ b/cmake/assert_include/Eigen/Core @@ -0,0 +1,2 @@ +#define eigen_assert(x) assert(x) +#include_next "Eigen/Core" diff --git a/cmake/assert_include/Eigen/Dense b/cmake/assert_include/Eigen/Dense new file mode 100644 index 00000000000..2e24cc8f4af --- /dev/null +++ b/cmake/assert_include/Eigen/Dense @@ -0,0 +1,2 @@ +#define eigen_assert(x) assert(x) +#include_next "Eigen/Dense" diff --git a/cmake/assert_include/Eigen/Geometry b/cmake/assert_include/Eigen/Geometry new file mode 100644 index 00000000000..009246d3255 --- /dev/null +++ b/cmake/assert_include/Eigen/Geometry @@ -0,0 +1,2 @@ +#define eigen_assert(x) assert(x) +#include_next "Eigen/Geometry" diff --git a/cmake/assert_include/cassert b/cmake/assert_include/cassert new file mode 100644 index 00000000000..806ccd3d22a --- /dev/null +++ b/cmake/assert_include/cassert @@ -0,0 +1,9 @@ +#if defined(NDEBUG) +#define _HAD_NDEBUG +#undef NDEBUG +#endif +#include_next +#if defined(_HAD_NDEBUG) +#undef _HAD_NDEBUG +#define NDEBUG +#endif diff --git a/docs/getting_started.md b/docs/getting_started.md index f88aeea5043..7848b3c46b0 100644 --- a/docs/getting_started.md +++ b/docs/getting_started.md @@ -259,6 +259,7 @@ components. | ACTS_BUILD_DOCS | Build documentation | | ACTS_BUILD_ANALYSIS_APPS | Build root based stand-alone analysis applications (defaults is OFF) | | ACTS_LOG_FAILURE_THRESHOLD | Automatically fail when a log above the specified debug level is emitted (useful for automated tests) | +| ACTS_FORCE_ASSERTIONS | Try to force keeping `assert` even in Release builds. (useful for automated tests) | | ACTS_PARAMETER_DEFINITIONS_HEADER | Use a different (track) parameter definitions header | | ACTS_USE_SYSTEM_AUTODIFF | Use autodiff provided by the system instead of the bundled version | | ACTS_USE_SYSTEM_NLOHMANN_JSON | Use nlohmann::json provided by the system instead of the bundled version | From 6b30469a2cdaa130f230d086ead1ab363a755678 Mon Sep 17 00:00:00 2001 From: pbutti Date: Wed, 9 Mar 2022 16:34:21 +0100 Subject: [PATCH 05/16] feat: Improvements to Cuboid Volume Builder (#1166) Adds a number of changes to the CuboidVolumeBuilder to make it more robust. --- .../Acts/Geometry/CuboidVolumeBuilder.hpp | 15 ++-- Core/src/Geometry/CuboidVolumeBuilder.cpp | 80 +++++++++++++++---- .../Geometry/CuboidVolumeBuilderTests.cpp | 12 +-- .../Core/Propagator/StepperTests.cpp | 4 +- .../Visualization/EventDataView3DBase.hpp | 2 +- 5 files changed, 83 insertions(+), 30 deletions(-) diff --git a/Core/include/Acts/Geometry/CuboidVolumeBuilder.hpp b/Core/include/Acts/Geometry/CuboidVolumeBuilder.hpp index 8724a5d58f5..37a16f011bc 100644 --- a/Core/include/Acts/Geometry/CuboidVolumeBuilder.hpp +++ b/Core/include/Acts/Geometry/CuboidVolumeBuilder.hpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -28,7 +29,7 @@ class RectangleBounds; class ISurfaceMaterial; class IVolumeMaterial; class DetectorElementBase; -class PlaneSurface; +class Surface; class Layer; /// @brief This class builds a box detector with a configurable amount of @@ -59,18 +60,20 @@ class CuboidVolumeBuilder : public ITrackingVolumeBuilder { }; /// @brief This struct stores the data for the construction of a PlaneLayer - /// that has a single PlaneSurface encapsulated struct LayerConfig { // Configuration of the surface - SurfaceConfig surfaceCfg; + std::vector surfaceCfg; // Encapsulated surface - std::shared_ptr surface = nullptr; + std::vector> surfaces; // Boolean flag if layer is active bool active = false; // Bins in Y direction size_t binsY = 1; // Bins in Z direction size_t binsZ = 1; + // Envelope in X (along layer normal) + std::pair envelopeX{0, 0}; + std::optional rotation{std::nullopt}; }; /// @brief This struct stores the data for the construction of a cuboid @@ -124,8 +127,8 @@ class CuboidVolumeBuilder : public ITrackingVolumeBuilder { /// @param [in] cfg Configuration of the surface /// /// @return Pointer to the created surface - std::shared_ptr buildSurface( - const GeometryContext& gctx, const SurfaceConfig& cfg) const; + std::shared_ptr buildSurface(const GeometryContext& gctx, + const SurfaceConfig& cfg) const; /// @brief This function creates a layer with a surface encaspulated with a /// given configuration. The surface gets a detector element attached if the diff --git a/Core/src/Geometry/CuboidVolumeBuilder.cpp b/Core/src/Geometry/CuboidVolumeBuilder.cpp index 4aec353f0a2..85e7b8eaf7d 100644 --- a/Core/src/Geometry/CuboidVolumeBuilder.cpp +++ b/Core/src/Geometry/CuboidVolumeBuilder.cpp @@ -33,10 +33,8 @@ #include "Acts/Utilities/Logger.hpp" #include -#include -std::shared_ptr -Acts::CuboidVolumeBuilder::buildSurface( +std::shared_ptr Acts::CuboidVolumeBuilder::buildSurface( const GeometryContext& /*gctx*/, const CuboidVolumeBuilder::SurfaceConfig& cfg) const { std::shared_ptr surface; @@ -60,33 +58,73 @@ Acts::CuboidVolumeBuilder::buildSurface( std::shared_ptr Acts::CuboidVolumeBuilder::buildLayer( const GeometryContext& gctx, Acts::CuboidVolumeBuilder::LayerConfig& cfg) const { + if (cfg.surfaces.empty() && cfg.surfaceCfg.empty()) { + throw std::runtime_error{ + "Neither surfaces nor config to build surfaces was provided. Cannot " + "proceed"}; + } + // Build the surface - if (cfg.surface == nullptr) { - cfg.surface = buildSurface(gctx, cfg.surfaceCfg); + if (cfg.surfaces.empty()) { + for (const auto& sCfg : cfg.surfaceCfg) { + cfg.surfaces.push_back(buildSurface(gctx, sCfg)); + } } // Build transformation centered at the surface position - Transform3 trafo(Transform3::Identity() * cfg.surfaceCfg.rotation); - trafo.translation() = cfg.surfaceCfg.position; + Vector3 centroid{0., 0., 0.}; + + for (const auto& surface : cfg.surfaces) { + centroid += surface->transform(gctx).translation(); + } + + centroid /= cfg.surfaces.size(); + + // In the case the layer configuration doesn't define the rotation of the + // layer use the orientation of the first surface to define the layer rotation + // in space. + Transform3 trafo = Transform3::Identity(); + trafo.translation() = centroid; + if (cfg.rotation) { + trafo.linear() = *cfg.rotation; + } else { + trafo.linear() = cfg.surfaces.front()->transform(gctx).rotation(); + } LayerCreator::Config lCfg; lCfg.surfaceArrayCreator = std::make_shared(); LayerCreator layerCreator(lCfg); - return layerCreator.planeLayer(gctx, {cfg.surface}, cfg.binsY, cfg.binsZ, - BinningValue::binX, std::nullopt, trafo); + ProtoLayer pl{gctx, cfg.surfaces}; + pl.envelope[binX] = cfg.envelopeX; + return layerCreator.planeLayer(gctx, cfg.surfaces, cfg.binsY, cfg.binsZ, + BinningValue::binX, pl, trafo); } std::pair Acts::CuboidVolumeBuilder::binningRange( - const GeometryContext& /*gctx*/, + const GeometryContext& gctx, const Acts::CuboidVolumeBuilder::VolumeConfig& cfg) const { using namespace UnitLiterals; // Construct return value std::pair minMax = std::make_pair( std::numeric_limits::max(), -std::numeric_limits::max()); + + // Compute the min volume boundaries for computing the binning start + // See + // https://acts.readthedocs.io/en/latest/core/geometry.html#geometry-building + // !! IMPORTANT !! The volume is assumed to be already rotated into the + // telescope geometry + Vector3 minVolumeBoundaries = cfg.position - 0.5 * cfg.length; + Vector3 maxVolumeBoundaries = cfg.position + 0.5 * cfg.length; + + // Compute first the min-max from the layers + for (const auto& layercfg : cfg.layerCfg) { - auto surfacePosMin = layercfg.surfaceCfg.position.x() - - layercfg.surfaceCfg.thickness / 2. - 1._um; - auto surfacePosMax = layercfg.surfaceCfg.position.x() + - layercfg.surfaceCfg.thickness / 2. + 1._um; + // recreating the protolayer for each layer => slow, but only few sensors + ProtoLayer pl{gctx, layercfg.surfaces}; + pl.envelope[binX] = layercfg.envelopeX; + + double surfacePosMin = pl.min(binX); + double surfacePosMax = pl.max(binX); + // Test if new extreme is found and set it if (surfacePosMin < minMax.first) { minMax.first = surfacePosMin; @@ -95,6 +133,11 @@ std::pair Acts::CuboidVolumeBuilder::binningRange( minMax.second = surfacePosMax; } } + + // Use the volume boundaries as limits for the binning + minMax.first = std::min(minMax.first, minVolumeBoundaries(binX)); + minMax.second = std::max(minMax.second, maxVolumeBoundaries(binX)); + return minMax; } @@ -124,7 +167,7 @@ std::shared_ptr Acts::CuboidVolumeBuilder::buildVolume( RectangleBounds(cfg.length.y() * 0.5, cfg.length.z() * 0.5)); LayerConfig lCfg; - lCfg.surfaceCfg = sCfg; + lCfg.surfaceCfg = {sCfg}; cfg.layerCfg.push_back(lCfg); } @@ -183,6 +226,13 @@ Acts::MutableTrackingVolumePtr Acts::CuboidVolumeBuilder::trackingVolume( volumes.push_back(buildVolume(gctx, volCfg)); } + // Sort the volumes vectors according to the center location, otherwise the + // binning boundaries will fail + std::sort(volumes.begin(), volumes.end(), + [](const TrackingVolumePtr& lhs, const TrackingVolumePtr& rhs) { + return lhs->center().x() < rhs->center().x(); + }); + // Glue volumes for (unsigned int i = 0; i < volumes.size() - 1; i++) { volumes[i + 1]->glueTrackingVolume( diff --git a/Tests/UnitTests/Core/Geometry/CuboidVolumeBuilderTests.cpp b/Tests/UnitTests/Core/Geometry/CuboidVolumeBuilderTests.cpp index 77f7ed98531..eb25d0c5df9 100644 --- a/Tests/UnitTests/Core/Geometry/CuboidVolumeBuilderTests.cpp +++ b/Tests/UnitTests/Core/Geometry/CuboidVolumeBuilderTests.cpp @@ -102,7 +102,7 @@ BOOST_AUTO_TEST_CASE(CuboidVolumeBuilderTest) { // Test that 4 surfaces can be built for (const auto& cfg : surfaceConfig) { - std::shared_ptr pSur = cvb.buildSurface(tgContext, cfg); + std::shared_ptr pSur = cvb.buildSurface(tgContext, cfg); BOOST_CHECK_NE(pSur, nullptr); CHECK_CLOSE_ABS(pSur->center(tgContext), cfg.position, 1e-9); BOOST_CHECK_NE(pSur->surfaceMaterial(), nullptr); @@ -114,7 +114,7 @@ BOOST_AUTO_TEST_CASE(CuboidVolumeBuilderTest) { std::vector layerConfig; for (auto& sCfg : surfaceConfig) { CuboidVolumeBuilder::LayerConfig cfg; - cfg.surfaceCfg = sCfg; + cfg.surfaceCfg = {sCfg}; layerConfig.push_back(cfg); } @@ -125,13 +125,13 @@ BOOST_AUTO_TEST_CASE(CuboidVolumeBuilderTest) { for (auto& cfg : layerConfig) { LayerPtr layer = cvb.buildLayer(tgContext, cfg); BOOST_CHECK_NE(layer, nullptr); - BOOST_CHECK_NE(cfg.surface, nullptr); + BOOST_CHECK(!cfg.surfaces.empty()); BOOST_CHECK_EQUAL(layer->surfaceArray()->surfaces().size(), 1u); BOOST_CHECK_EQUAL(layer->layerType(), LayerType::active); } for (auto& cfg : layerConfig) { - cfg.surface = nullptr; + cfg.surfaces = {}; } // Build volume configuration @@ -164,7 +164,7 @@ BOOST_AUTO_TEST_CASE(CuboidVolumeBuilderTest) { volumeConfig.layers.clear(); for (auto& lay : volumeConfig.layerCfg) { - lay.surface = nullptr; + lay.surfaces = {}; lay.active = true; } trVol = cvb.buildVolume(tgContext, volumeConfig); @@ -218,7 +218,7 @@ BOOST_AUTO_TEST_CASE(CuboidVolumeBuilderTest) { std::vector layerConfig2; for (auto& sCfg : surfaceConfig2) { CuboidVolumeBuilder::LayerConfig cfg; - cfg.surfaceCfg = sCfg; + cfg.surfaceCfg = {sCfg}; layerConfig2.push_back(cfg); } CuboidVolumeBuilder::VolumeConfig volumeConfig2; diff --git a/Tests/UnitTests/Core/Propagator/StepperTests.cpp b/Tests/UnitTests/Core/Propagator/StepperTests.cpp index 6a4a4b579d8..c18bf3f4d17 100644 --- a/Tests/UnitTests/Core/Propagator/StepperTests.cpp +++ b/Tests/UnitTests/Core/Propagator/StepperTests.cpp @@ -983,7 +983,7 @@ BOOST_AUTO_TEST_CASE(step_extension_trackercalomdt_test) { new HomogeneousSurfaceMaterial(matProp)); sConf1.thickness = 1._mm; CuboidVolumeBuilder::LayerConfig lConf1; - lConf1.surfaceCfg = sConf1; + lConf1.surfaceCfg = {sConf1}; CuboidVolumeBuilder::SurfaceConfig sConf2; sConf2.position = Vector3(0.6_m, 0., 0.); @@ -996,7 +996,7 @@ BOOST_AUTO_TEST_CASE(step_extension_trackercalomdt_test) { new HomogeneousSurfaceMaterial(matProp)); sConf2.thickness = 1._mm; CuboidVolumeBuilder::LayerConfig lConf2; - lConf2.surfaceCfg = sConf2; + lConf2.surfaceCfg = {sConf2}; CuboidVolumeBuilder::VolumeConfig muConf1; muConf1.position = {2.3_m, 0., 0.}; diff --git a/Tests/UnitTests/Core/Visualization/EventDataView3DBase.hpp b/Tests/UnitTests/Core/Visualization/EventDataView3DBase.hpp index 65c94ebfebb..44445dfa6c9 100644 --- a/Tests/UnitTests/Core/Visualization/EventDataView3DBase.hpp +++ b/Tests/UnitTests/Core/Visualization/EventDataView3DBase.hpp @@ -160,7 +160,7 @@ static inline std::string testMultiTrajectory(IVisualization3D& helper) { return new Test::DetectorElementStub(trans, bounds, thickness); }; CuboidVolumeBuilder::LayerConfig lConf; - lConf.surfaceCfg = sConf; + lConf.surfaceCfg = {sConf}; lConfs.push_back(lConf); } From 3f3038c441a8148adbfc89ef695b556200265c7e Mon Sep 17 00:00:00 2001 From: Carlo Varni <75478407+CarloVarni@users.noreply.github.com> Date: Wed, 9 Mar 2022 09:22:50 -0800 Subject: [PATCH 06/16] fix: Coulombs Unit (#1179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since `e=1` the C unit should be `C = 1/1.602176634e-19 = 6.2415091e18` so that `e = 1.602176634e−19 C` is respected --- Core/include/Acts/Definitions/Units.hpp | 2 +- Tests/UnitTests/Core/Definitions/UnitsTests.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Core/include/Acts/Definitions/Units.hpp b/Core/include/Acts/Definitions/Units.hpp index 4fbc65105c0..b1728554ae9 100644 --- a/Core/include/Acts/Definitions/Units.hpp +++ b/Core/include/Acts/Definitions/Units.hpp @@ -140,7 +140,7 @@ constexpr double g = 1.0 / 1.782662e-24; constexpr double kg = 1.0 / 1.782662e-27; // Charge, native unit e (elementary charge) constexpr double e = 1.0; -constexpr double C = 1.602176634e19; +constexpr double C = 6.2415091e18; // Magnetic field, native unit GeV/(e*mm) constexpr double T = 0.000299792458; // equivalent to c in appropriate SI units constexpr double Gauss = 1e-4 * T; diff --git a/Tests/UnitTests/Core/Definitions/UnitsTests.cpp b/Tests/UnitTests/Core/Definitions/UnitsTests.cpp index 2ef32307d1d..1f8912b2778 100644 --- a/Tests/UnitTests/Core/Definitions/UnitsTests.cpp +++ b/Tests/UnitTests/Core/Definitions/UnitsTests.cpp @@ -109,7 +109,7 @@ BOOST_AUTO_TEST_CASE(DecayWidthTime) { } BOOST_AUTO_TEST_CASE(Charge) { - CHECK_CLOSE_REL(1_C, 1.602176634e19_e, eps); + CHECK_CLOSE_REL(1_C, 6.2415091e18_e, eps); } BOOST_AUTO_TEST_CASE(MagneticField) { From b5f5efb322a3a77311f4a54b0a96806a9fa75348 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Sun, 13 Mar 2022 14:26:12 +0100 Subject: [PATCH 07/16] fix: Clang (13) warning in BoundingBox (#1189) I noticed a warning: ``` [715/1093] Building CXX object Tests/UnitTests/Core/Geo...pezoidVolumeBounds.dir/TrapezoidVolumeBoundsTests.cpp.o In file included from ../Tests/UnitTests/Core/Geometry/TrapezoidVolumeBoundsTests.cpp:13: In file included from ../Core/include/Acts/Geometry/TrapezoidVolumeBounds.hpp:12: In file included from ../Core/include/Acts/Geometry/Volume.hpp:15: ../Core/include/Acts/Utilities/BoundingBox.hpp:101:3: warning: definition of implicit copy assignment operator for 'AxisAlignedBoundingBox' is deprecated because it has a user-declared copy constructor [-Wdeprecated-copy] AxisAlignedBoundingBox(const self_t& other) = default; ^ ../Tests/UnitTests/Core/Geometry/TrapezoidVolumeBoundsTests.cpp:40:6: note: in implicit copy assignment operator for 'Acts::AxisAlignedBoundingBox' first required here bb = tvb.boundingBox(&trf); ^ 1 warning generated. ``` in a build with clang-13. This PR should remove this warning by defaulting the copy assignment operator. --- Core/include/Acts/Utilities/BoundingBox.hpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Core/include/Acts/Utilities/BoundingBox.hpp b/Core/include/Acts/Utilities/BoundingBox.hpp index 4cfd91a1306..af0d6485b2e 100644 --- a/Core/include/Acts/Utilities/BoundingBox.hpp +++ b/Core/include/Acts/Utilities/BoundingBox.hpp @@ -100,6 +100,12 @@ class AxisAlignedBoundingBox { */ AxisAlignedBoundingBox(const self_t& other) = default; + /** + * Copy assignment operator from other bounding box. + * @param other The other AABB + */ + AxisAlignedBoundingBox& operator=(const self_t& other) = default; + /** * Constructor from an entity pointer, and the min and max vertices. * @param entity The entity to store From 1c1b624078e6c947c31bbd97272e684911370af4 Mon Sep 17 00:00:00 2001 From: "Alexander J. Pfleger" <70842573+AJPfleger@users.noreply.github.com> Date: Mon, 14 Mar 2022 12:26:14 +0100 Subject: [PATCH 08/16] docs: remove copy-paste relics in comment in RPStepsWriter (#1192) --- .../include/ActsExamples/Io/Root/RootPropagationStepsWriter.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Examples/Io/Root/include/ActsExamples/Io/Root/RootPropagationStepsWriter.hpp b/Examples/Io/Root/include/ActsExamples/Io/Root/RootPropagationStepsWriter.hpp index 1b35cf3ba62..e594a0c0523 100644 --- a/Examples/Io/Root/include/ActsExamples/Io/Root/RootPropagationStepsWriter.hpp +++ b/Examples/Io/Root/include/ActsExamples/Io/Root/RootPropagationStepsWriter.hpp @@ -23,7 +23,7 @@ using PropagationSteps = std::vector; /// @class RootPropagationStepsWriter /// /// Write out the steps of test propgations for stepping validation, -/// each step sequence is one entry in the in the root file for optimised +/// each step sequence is one entry in the root file for optimised /// data writing speed. /// The event number is part of the written data. /// From b804b327ac60a5e5798d99ffa3b5ae9c915764be Mon Sep 17 00:00:00 2001 From: Nora Pettersson <94444689+noraemi@users.noreply.github.com> Date: Tue, 15 Mar 2022 17:19:27 +0100 Subject: [PATCH 09/16] fix: Fix issue with applying transformation to bevelled cylinders (#1185) Applying transforms to the cylinder did not work as intended. These fixes should place the side disks at the right place as well as fixing the visualization of the cylinder length. --- Core/src/Geometry/CylinderVolumeBounds.cpp | 8 ++------ Core/src/Surfaces/CylinderBounds.cpp | 7 ++++++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Core/src/Geometry/CylinderVolumeBounds.cpp b/Core/src/Geometry/CylinderVolumeBounds.cpp index d188a88ea4f..079858248f9 100644 --- a/Core/src/Geometry/CylinderVolumeBounds.cpp +++ b/Core/src/Geometry/CylinderVolumeBounds.cpp @@ -67,9 +67,7 @@ Acts::OrientedSurfaces Acts::CylinderVolumeBounds::orientedSurfaces( double bevelMaxZ = get(eBevelMaxZ); Transform3 transMinZ, transMaxZ; if (bevelMinZ != 0.) { - double rmax = get(eMaxR); - double rmin = get(eMinR); - double sy = (rmax + rmin) * (1 - std::cos(bevelMinZ)) / std::cos(bevelMinZ); + double sy = 1 - 1 / std::cos(bevelMinZ); transMinZ = transform * vMinZ * Eigen::AngleAxisd(-bevelMinZ, Eigen::Vector3d(1., 0., 0.)) * Eigen::Scaling(1., 1. + sy, 1.); @@ -77,9 +75,7 @@ Acts::OrientedSurfaces Acts::CylinderVolumeBounds::orientedSurfaces( transMinZ = transform * vMinZ; } if (bevelMaxZ != 0.) { - double rmax = get(eMaxR); - double rmin = get(eMinR); - double sy = (rmax + rmin) * (1 - std::cos(bevelMaxZ)) / std::cos(bevelMaxZ); + double sy = 1 - 1 / std::cos(bevelMaxZ); transMaxZ = transform * vMaxZ * Eigen::AngleAxisd(bevelMaxZ, Eigen::Vector3d(1., 0., 0.)) * Eigen::Scaling(1., 1. + sy, 1.); diff --git a/Core/src/Surfaces/CylinderBounds.cpp b/Core/src/Surfaces/CylinderBounds.cpp index 50a5f0e6e42..3f535cab4cc 100644 --- a/Core/src/Surfaces/CylinderBounds.cpp +++ b/Core/src/Surfaces/CylinderBounds.cpp @@ -167,7 +167,12 @@ std::vector Acts::CylinderBounds::createCircles( if ((bevelMinZ != 0. || bevelMaxZ != 0.) && vertices.size() % 2 == 0) { auto halfWay = vertices.end() - vertices.size() / 2; double mult{1}; - auto func = [&mult](Vector3& v) { v(2) += v(1) * mult; }; + auto invCtrans = ctrans.inverse(); + auto func = [&mult, &ctrans, &invCtrans](Vector3& v) { + v = invCtrans * v; + v(2) += v(1) * mult; + v = ctrans * v; + }; if (bevelMinZ != 0.) { mult = std::tan(-bevelMinZ); std::for_each(vertices.begin(), halfWay, func); From 7034c5ac9ab0aa273626f94fa9ab115144b3376e Mon Sep 17 00:00:00 2001 From: Stephen Nicholas Swatman Date: Tue, 15 Mar 2022 19:42:04 +0100 Subject: [PATCH 10/16] feat: Implement blocked matrix multiplication (#1184) Eigen uses a built-in cost model to determine how it should perform certain matrix operations. In general, this works quite well, but there are some edge cases where Eigen can struggle to output efficient code. An especially apparent instance of this is when multiplying 8x8 matrices, which we do very commonly in operations such as stepping. In these cases, we observe that Eigen emits code for generalized matrix-matrix multiplication, rather than specialized code for small matrices. This adds significant overhead to the matrix multiplication, and it slows down some parts of our code a lot. Our proposed solution is to implement a wrapper around Eigen's matrix multiplication code that breaks a multiplication down into smaller operations through a technique called blocking. A matrix of size 2ix2j can be split into four matrices of sizes ixj, which can then be multiplied and added to provide a correct multiplication of the parent matrices. In this way, we can encourage Eigen to emit code for its hand-optimized small-matrix operations, which should give speed-up in some cases. The operation is also supported for odd-sized matrices. --- Core/include/Acts/Utilities/Helpers.hpp | 102 ++++++++++++++++++ .../UnitTests/Core/Utilities/HelpersTests.cpp | 42 ++++++++ 2 files changed, 144 insertions(+) diff --git a/Core/include/Acts/Utilities/Helpers.hpp b/Core/include/Acts/Utilities/Helpers.hpp index 44bb78f1313..1568ed7abc1 100644 --- a/Core/include/Acts/Utilities/Helpers.hpp +++ b/Core/include/Acts/Utilities/Helpers.hpp @@ -434,4 +434,106 @@ auto matrixToBitset(const Eigen::PlainObjectBase& m) { return res; } +/// @brief Perform a blocked matrix multiplication, avoiding Eigen GEMM +/// methods. +/// +/// @tparam A The type of the first matrix, which should be MxN +/// @tparam B The type of the second matrix, which should be NxP +/// +/// @param[in] a An MxN matrix of type A +/// @param[in] b An NxP matrix of type P +/// +/// @returns The product ab +template +inline ActsMatrix blockedMult( + const A& a, const B& b) { + // Extract the sizes of the matrix types that we receive as template + // parameters. + constexpr int M = A::RowsAtCompileTime; + constexpr int N = A::ColsAtCompileTime; + constexpr int P = B::ColsAtCompileTime; + + // Ensure that the second dimension of our first matrix equals the first + // dimension of the second matrix, otherwise we cannot multiply. + static_assert(N == B::RowsAtCompileTime); + + if constexpr (M <= 4 && N <= 4 && P <= 4) { + // In cases where the matrices being multiplied are small, we can rely on + // Eigen do to a good job, and we don't really need to do any blocking. + return a * b; + } else { + // Here, we want to calculate the expression: C = AB, Eigen, natively, + // doesn't do a great job at this if the matrices A and B are large + // (roughly M >= 8, N >= 8, or P >= 8), and applies a slow GEMM operation. + // We apply a blocked matrix multiplication operation to decompose the + // multiplication into smaller operations, relying on the fact that: + // + // ┌ ┐ ┌ ┐ ┌ ┐ + // │ C₁₁ C₁₂ │ = │ A₁₁ A₁₂ │ │ B₁₁ B₁₂ │ + // │ C₂₁ C₂₂ │ = │ A₂₁ A₂₂ │ │ B₂₁ B₂₂ │ + // └ ┘ └ ┘ └ ┘ + // + // where: + // + // C₁₁ = A₁₁ * B₁₁ + A₁₂ * B₂₁ + // C₁₂ = A₁₁ * B₁₂ + A₁₂ * B₂₂ + // C₂₁ = A₂₁ * B₁₁ + A₂₂ * B₂₁ + // C₂₂ = A₂₁ * B₁₂ + A₂₂ * B₂₂ + // + // The sizes of these submatrices are roughly half (in each dimension) that + // of the parent matrix. If the size of the parent matrix is even, we can + // divide it exactly, If the size of the parent matrix is odd, then some + // of the submatrices will be one larger than the others. In general, for + // any matrix Q, the sizes of the submatrices are (where / denotes integer + // division): + // + // Q₁₁ : M / 2 × P / 2 + // Q₁₂ : M / 2 × (P + 1) / 2 + // Q₂₁ : (M + 1) / 2 × P / 2 + // Q₂₂ : (M + 1) / 2 × (P + 1) / 2 + // + // See https://csapp.cs.cmu.edu/public/waside/waside-blocking.pdf for a + // more in-depth explanation of blocked matrix multiplication. + constexpr int M1 = M / 2; + constexpr int M2 = (M + 1) / 2; + constexpr int N1 = N / 2; + constexpr int N2 = (N + 1) / 2; + constexpr int P1 = P / 2; + constexpr int P2 = (P + 1) / 2; + + // Construct the end result in this matrix, which destroys a few of Eigen's + // built-in optimization techniques, but sadly this is necessary. + ActsMatrix r; + + // C₁₁ = A₁₁ * B₁₁ + A₁₂ * B₂₁ + r.template topLeftCorner().noalias() = + a.template topLeftCorner() * + b.template topLeftCorner() + + a.template topRightCorner() * + b.template bottomLeftCorner(); + + // C₁₂ = A₁₁ * B₁₂ + A₁₂ * B₂₂ + r.template topRightCorner().noalias() = + a.template topLeftCorner() * + b.template topRightCorner() + + a.template topRightCorner() * + b.template bottomRightCorner(); + + // C₂₁ = A₂₁ * B₁₁ + A₂₂ * B₂₁ + r.template bottomLeftCorner().noalias() = + a.template bottomLeftCorner() * + b.template topLeftCorner() + + a.template bottomRightCorner() * + b.template bottomLeftCorner(); + + // C₂₂ = A₂₁ * B₁₂ + A₂₂ * B₂₂ + r.template bottomRightCorner().noalias() = + a.template bottomLeftCorner() * + b.template topRightCorner() + + a.template bottomRightCorner() * + b.template bottomRightCorner(); + + return r; + } +} } // namespace Acts diff --git a/Tests/UnitTests/Core/Utilities/HelpersTests.cpp b/Tests/UnitTests/Core/Utilities/HelpersTests.cpp index 4f3e07ee2a6..f0b00acd7d6 100644 --- a/Tests/UnitTests/Core/Utilities/HelpersTests.cpp +++ b/Tests/UnitTests/Core/Utilities/HelpersTests.cpp @@ -155,6 +155,48 @@ BOOST_AUTO_TEST_CASE(test_matrix_dimension_switch) { } } +typedef std::tuple< + // Square matrix tests + std::pair, ActsMatrix<4, 4>>, + std::pair, ActsMatrix<8, 8>>, + + // Square odd matrix tests + std::pair, ActsMatrix<3, 3>>, + std::pair, ActsMatrix<7, 7>>, + + // Non-square matrix tests + std::pair, ActsMatrix<4, 3>>, + std::pair, ActsMatrix<8, 4>>, + std::pair, ActsMatrix<3, 8>>, + std::pair, ActsMatrix<3, 7>>, + std::pair, ActsMatrix<3, 7>>, + std::pair, ActsMatrix<7, 4>>, + + // Very large matrix tests for sanity + std::pair, ActsMatrix<81, 59>>, + std::pair, ActsMatrix<82, 60>>, + std::pair, ActsMatrix<82, 59>>, + std::pair, ActsMatrix<81, 60>>> + MatrixProductTypes; + +BOOST_AUTO_TEST_CASE_TEMPLATE(BlockedMatrixMultiplication, Matrices, + MatrixProductTypes) { + using A = typename Matrices::first_type; + using B = typename Matrices::second_type; + using C = ActsMatrix; + + for (std::size_t i = 0; i < 100; ++i) { + A a = A::Random(); + B b = B::Random(); + + C ref = a * b; + C res = blockedMult(a, b); + + BOOST_CHECK(ref.isApprox(res)); + BOOST_CHECK(res.isApprox(ref)); + } +} + BOOST_AUTO_TEST_SUITE_END() } // namespace Test } // namespace Acts From 3b264d75dc73491705e1afa69f568aad5d5253cd Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Wed, 16 Mar 2022 14:57:55 +0100 Subject: [PATCH 11/16] feat: Allow setting ACTS_LOG_FAILURE_THRESHOLD at runtime (#1195) The physics perf monitoring in #1193 runs on the ODD, which currently throws a number of ERRORs. To circumvent this, this PR allows setting the threshold through an environment variable, in addition to the compile time option. PLEASE FOLLOW THE CHECKLIST BELOW WHEN CREATING A NEW PULL REQUEST. THE CHECKLIST IS FOR YOUR INFORMATION AND MUST BE REMOVED BEFORE SUBMITTING THE PULL REQUEST. --- Core/include/Acts/Utilities/Logger.hpp | 39 ++++++++++++++++++- .../UnitTests/Core/Utilities/LoggerTests.cpp | 4 +- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/Core/include/Acts/Utilities/Logger.hpp b/Core/include/Acts/Utilities/Logger.hpp index 6f1b7f6598c..9a7fafb5763 100644 --- a/Core/include/Acts/Utilities/Logger.hpp +++ b/Core/include/Acts/Utilities/Logger.hpp @@ -171,6 +171,41 @@ constexpr Level FAILURE_THRESHOLD = Level::MAX; #endif +/// Function which returns the failure threshold for log messages. +/// This can either be from a compilation option or from an environment +/// variable. +/// @return The log level failure threshold +inline Level getFailureThreshold() { + static const Level threshold{[]() -> Level { + Level level = FAILURE_THRESHOLD; + const char* envvar = std::getenv("ACTS_LOG_FAILURE_THRESHOLD"); + if (envvar == nullptr) { + return level; + } + std::string slevel = envvar; + if (slevel == "VERBOSE") { + level = std::min(level, Level::VERBOSE); + } else if (slevel == "DEBUG") { + level = std::min(level, Level::DEBUG); + } else if (slevel == "INFO") { + level = std::min(level, Level::INFO); + } else if (slevel == "WARNING") { + level = std::min(level, Level::WARNING); + } else if (slevel == "ERROR") { + level = std::min(level, Level::ERROR); + } else if (slevel == "FATAL") { + level = std::min(level, Level::FATAL); + } else { + std::cerr << "ACTS_LOG_FAILURE_THRESHOLD environment variable is set to " + "unknown value: " + << slevel << std::endl; + } + return level; + }()}; + + return threshold; +} + /// @brief abstract base class for printing debug output /// /// Implementations of this interface need to define how and where to @a print @@ -215,7 +250,7 @@ class DefaultFilterPolicy final : public OutputFilterPolicy { /// /// @param [in] lvl threshold debug level explicit DefaultFilterPolicy(const Level& lvl) : m_level(lvl) { - if (lvl > FAILURE_THRESHOLD) { + if (lvl > getFailureThreshold()) { throw std::runtime_error( "Requested debug level is incompatible with " "the ACTS_LOG_FAILURE_THRESHOLD configuration"); @@ -428,7 +463,7 @@ class DefaultPrintPolicy final : public OutputPrintPolicy { /// @param [in] input text of debug message void flush(const Level& lvl, const std::string& input) final { (*m_out) << input << std::endl; - if (lvl >= FAILURE_THRESHOLD) { + if (lvl >= getFailureThreshold()) { throw std::runtime_error( "Previous debug message exceeds the " "ACTS_LOG_FAILURE_THRESHOLD configuration, bailing out"); diff --git a/Tests/UnitTests/Core/Utilities/LoggerTests.cpp b/Tests/UnitTests/Core/Utilities/LoggerTests.cpp index dcea8708139..172854a6f0d 100644 --- a/Tests/UnitTests/Core/Utilities/LoggerTests.cpp +++ b/Tests/UnitTests/Core/Utilities/LoggerTests.cpp @@ -50,7 +50,7 @@ void debug_level_test(const char* output_file, Logging::Level lvl) { // If fail-on-error is enabled, then the logger will not, and should not, // tolerate being set up with a coarser debug level. - if (lvl > Logging::FAILURE_THRESHOLD) { + if (lvl > Logging::getFailureThreshold()) { BOOST_CHECK_THROW(detail::create_logger("TestLogger", &logfile, lvl), std::runtime_error); return; @@ -62,7 +62,7 @@ void debug_level_test(const char* output_file, Logging::Level lvl) { // Test logging at a certain debug level auto test_logging = [](auto&& test_operation, Logging::Level test_lvl) { - if (test_lvl >= Logging::FAILURE_THRESHOLD) { + if (test_lvl >= Logging::getFailureThreshold()) { BOOST_CHECK_THROW(test_operation(), std::runtime_error); } else { test_operation(); From 6969a87663e9f6e17ee66e37e021b5575f903730 Mon Sep 17 00:00:00 2001 From: Stephen Nicholas Swatman Date: Fri, 18 Mar 2022 10:37:00 +0100 Subject: [PATCH 12/16] perf: Optimize Eigen usage in covariance engine (#1183) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit optimizes some of the Eigen usage in the covariance engine, specifically in the critical path for the propagation examples. The first optimisation we make is to introduce a tiled matrix multiplication method, which takes 2i×2j matrices, and performs four i×j multiplications instead, which Eigen can optimize far more easily. Secondly, we reduce the number of floating point operations performed by working with smaller submatrices wherever possible. On my machine, the following performance is achieved in the propagation example before this patch: 53.555595 ms/event. After this patch, we take 43.750143 ms/event. This performance gain is independent from the performance gain of #1181. --- Core/src/Propagator/detail/CovarianceEngine.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/Core/src/Propagator/detail/CovarianceEngine.cpp b/Core/src/Propagator/detail/CovarianceEngine.cpp index 33325ffb88f..e216e162fd4 100644 --- a/Core/src/Propagator/detail/CovarianceEngine.cpp +++ b/Core/src/Propagator/detail/CovarianceEngine.cpp @@ -10,6 +10,7 @@ #include "Acts/EventData/detail/TransformationBoundToFree.hpp" #include "Acts/EventData/detail/TransformationFreeToBound.hpp" +#include "Acts/Utilities/Helpers.hpp" #include "Acts/Utilities/Logger.hpp" #include "Acts/Utilities/Result.hpp" @@ -143,20 +144,22 @@ void boundToCurvilinearJacobian(const Vector3& direction, const FreeMatrix& freeTransportJacobian, const FreeVector& freeToPathDerivatives, BoundMatrix& fullTransportJacobian) { - // Calculate the derivative of path length at the the curvilinear surface - // w.r.t. free parameters - FreeToPathMatrix freeToPath = FreeToPathMatrix::Zero(); - freeToPath.segment<3>(eFreePos0) = -1.0 * direction; // Calculate the jacobian from global to local at the curvilinear surface FreeToBoundMatrix freeToBoundJacobian = freeToCurvilinearJacobian(direction); + + // Update the jacobian to include the derivative of the path length at the + // curvilinear surface w.r.t. the free parameters + freeToBoundJacobian.topLeftCorner<6, 3>() += + (freeToBoundJacobian * freeToPathDerivatives) * + (-1.0 * direction).transpose(); + // Calculate the full jocobian from the local parameters at the start surface // to curvilinear parameters // @note jac(locA->locB) = jac(gloB->locB)*(1+ // pathCorrectionFactor(gloB))*jacTransport(gloA->gloB) *jac(locA->gloA) fullTransportJacobian = - freeToBoundJacobian * - (FreeMatrix::Identity() + freeToPathDerivatives * freeToPath) * - freeTransportJacobian * boundToFreeJacobian; + blockedMult(freeToBoundJacobian, + blockedMult(freeTransportJacobian, boundToFreeJacobian)); } /// @brief This function reinitialises the state members required for the From 6147e1bfef03a6fdd1c288a5d716f13572b5ed3c Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 18 Mar 2022 13:35:54 +0100 Subject: [PATCH 13/16] build: Allow python bindings to be used from install (#1194) This PR adds some changes to enable the python bindings to be used from an install. Instead of `source $BUILD/python/setup.sh` you can now do `source $INSTALL/python/setup.sh`. The folder `python` won't make sense if ACTS is installed into a directory alongside other projects, but I would argue in that case you would not want to enable the python bindings anyway. --- Examples/Python/CMakeLists.txt | 23 ++++++++++++++--------- Examples/Python/setup.sh.in | 4 +++- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/Examples/Python/CMakeLists.txt b/Examples/Python/CMakeLists.txt index 275931f2a08..67ba5813580 100644 --- a/Examples/Python/CMakeLists.txt +++ b/Examples/Python/CMakeLists.txt @@ -5,6 +5,8 @@ set(_python_dir "${CMAKE_BINARY_DIR}/python") file(MAKE_DIRECTORY ${_python_dir}) file(MAKE_DIRECTORY ${_python_dir}/acts) +set(_python_install_dir "python/acts") + pybind11_add_module(ActsPythonBindings src/ModuleEntry.cpp src/Base.cpp @@ -22,9 +24,9 @@ pybind11_add_module(ActsPythonBindings src/TrackFinding.cpp src/Vertexing.cpp ) -install(TARGETS ActsPythonBindings DESTINATION .) +install(TARGETS ActsPythonBindings DESTINATION ${_python_install_dir}) -set_target_properties(ActsPythonBindings PROPERTIES INSTALL_RPATH "\$ORIGIN/${CMAKE_INSTALL_LIBDIR}") +set_target_properties(ActsPythonBindings PROPERTIES INSTALL_RPATH "\$ORIGIN/../../${CMAKE_INSTALL_LIBDIR}") set_target_properties(ActsPythonBindings PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${_python_dir}/acts) target_link_libraries(ActsPythonBindings PUBLIC @@ -70,8 +72,8 @@ if(ACTS_BUILD_PLUGIN_DD4HEP AND ACTS_BUILD_EXAMPLES_DD4HEP) ActsExamplesDetectorDD4hep) add_dependencies(ActsPythonBindings ActsPythonBindingsDD4hep) - install(TARGETS ActsPythonBindingsDD4hep DESTINATION .) - set_target_properties(ActsPythonBindingsDD4hep PROPERTIES INSTALL_RPATH "\$ORIGIN/${CMAKE_INSTALL_LIBDIR}") + install(TARGETS ActsPythonBindingsDD4hep DESTINATION ${_python_install_dir}) + set_target_properties(ActsPythonBindingsDD4hep PROPERTIES INSTALL_RPATH "\$ORIGIN../../${CMAKE_INSTALL_LIBDIR}") set_target_properties(ActsPythonBindingsDD4hep PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${_python_dir}/acts) list(APPEND py_files examples/dd4hep.py) endif() @@ -97,8 +99,8 @@ if(ACTS_BUILD_EXAMPLES_GEANT4) ActsPythonUtilities) add_dependencies(ActsPythonBindings ActsPythonBindingsGeant4) - install(TARGETS ActsPythonBindingsGeant4 DESTINATION .) - set_target_properties(ActsPythonBindingsGeant4 PROPERTIES INSTALL_RPATH "\$ORIGIN/${CMAKE_INSTALL_LIBDIR}") + install(TARGETS ActsPythonBindingsGeant4 DESTINATION ${_python_install_dir}) + set_target_properties(ActsPythonBindingsGeant4 PROPERTIES INSTALL_RPATH "\$ORIGIN../../${CMAKE_INSTALL_LIBDIR}") set_target_properties(ActsPythonBindingsGeant4 PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${_python_dir}/acts) list(APPEND py_files examples/geant4/__init__.py) @@ -110,8 +112,8 @@ if(ACTS_BUILD_EXAMPLES_GEANT4) ActsExamplesDetectorDD4hep) add_dependencies(ActsPythonBindings ActsPythonBindingsDDG4) - install(TARGETS ActsPythonBindingsDDG4 DESTINATION .) - set_target_properties(ActsPythonBindingsDDG4 PROPERTIES INSTALL_RPATH "\$ORIGIN/${CMAKE_INSTALL_LIBDIR}") + install(TARGETS ActsPythonBindingsDDG4 DESTINATION ${_python_install_dir}) + set_target_properties(ActsPythonBindingsDDG4 PROPERTIES INSTALL_RPATH "\$ORIGIN../../${CMAKE_INSTALL_LIBDIR}") set_target_properties(ActsPythonBindingsDDG4 PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${_python_dir}/acts) list(APPEND py_files examples/geant4/dd4hep.py) endif() @@ -136,7 +138,8 @@ endif() add_custom_target(ActsPythonGlueCode) -configure_file(setup.sh.in ${_python_dir}/setup.sh) +configure_file(setup.sh.in ${_python_dir}/setup.sh COPYONLY) +install(FILES setup.sh.in DESTINATION "python" RENAME setup.sh) foreach(f ${py_files}) @@ -147,4 +150,6 @@ file(MAKE_DIRECTORY ${_dir}) file(CREATE_LINK ${CMAKE_CURRENT_SOURCE_DIR}/python/acts/${f} ${_target} SYMBOLIC) endforeach() +install(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/python/acts/ DESTINATION ${_python_install_dir}) + add_dependencies(ActsPythonBindings ActsPythonGlueCode) diff --git a/Examples/Python/setup.sh.in b/Examples/Python/setup.sh.in index 3a1faf04b00..87af52b1e2c 100644 --- a/Examples/Python/setup.sh.in +++ b/Examples/Python/setup.sh.in @@ -1 +1,3 @@ -export PYTHONPATH=@_python_dir@:$PYTHONPATH \ No newline at end of file +python_dir=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) + +export PYTHONPATH=$python_dir:$PYTHONPATH From 1674bceb62549fdc8c1e07f39187d1e2d7c946d2 Mon Sep 17 00:00:00 2001 From: Luis Falda Coelho <56648068+LuisFelipeCoelho@users.noreply.github.com> Date: Fri, 18 Mar 2022 16:38:53 +0100 Subject: [PATCH 14/16] feat: ITk seeding example (#1186) This PR adds the `ITkSeedingExample.cpp` to run the ITk seeding. It shows how to configure the non-equidistant binning in z (#1005), the seed confirmation cuts (#1084), the radial range for middle SP cut (#1084) and the vector containing the map of z neighbours (#1052 and #1038). It contains all the parameters to run the seeding for ITk **pixel** space points (I intend to extend this to ITk strip SPs soon). @noemina @paulgessinger --- .../include/Acts/Seeding/SeedfinderConfig.hpp | 14 +- .../Io/Csv/CsvSpacePointReader.hpp | 3 + Examples/Python/src/Input.cpp | 22 ++- Examples/Python/src/TrackFinding.cpp | 28 +++ Examples/Scripts/Python/itk_seeding.py | 179 ++++++++++++++++++ 5 files changed, 239 insertions(+), 7 deletions(-) create mode 100755 Examples/Scripts/Python/itk_seeding.py diff --git a/Core/include/Acts/Seeding/SeedfinderConfig.hpp b/Core/include/Acts/Seeding/SeedfinderConfig.hpp index 1cc36bffa2d..e4ba4997ae2 100644 --- a/Core/include/Acts/Seeding/SeedfinderConfig.hpp +++ b/Core/include/Acts/Seeding/SeedfinderConfig.hpp @@ -16,12 +16,14 @@ namespace Acts { struct SeedConfirmationRange { - float zMinSeedConf; - float zMaxSeedConf; - float rMaxSeedConf; - size_t nTopSeedConf; - size_t nTopForLargeR; - size_t nTopForSmallR; + float zMinSeedConf = + std::numeric_limits::min() * Acts::UnitConstants::mm; + float zMaxSeedConf = + std::numeric_limits::max() * Acts::UnitConstants::mm; + float rMaxSeedConf = + std::numeric_limits::max() * Acts::UnitConstants::mm; + size_t nTopForLargeR = 0; + size_t nTopForSmallR = 0; }; // forward declaration to avoid cyclic dependence diff --git a/Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSpacePointReader.hpp b/Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSpacePointReader.hpp index eee15fce22b..0c546fff9e9 100644 --- a/Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSpacePointReader.hpp +++ b/Examples/Io/Csv/include/ActsExamples/Io/Csv/CsvSpacePointReader.hpp @@ -55,6 +55,9 @@ class CsvSpacePointReader final : public IReader { /// Read out data from the input stream. ProcessCode read(const ActsExamples::AlgorithmContext& ctx) final override; + /// Readonly access to the config + const Config& config() const { return m_cfg; } + private: Config m_cfg; std::pair m_eventsRange; diff --git a/Examples/Python/src/Input.cpp b/Examples/Python/src/Input.cpp index 79e746f210b..ab4cdf1cea3 100644 --- a/Examples/Python/src/Input.cpp +++ b/Examples/Python/src/Input.cpp @@ -11,6 +11,7 @@ #include "ActsExamples/Io/Csv/CsvParticleReader.hpp" #include "ActsExamples/Io/Csv/CsvPlanarClusterReader.hpp" #include "ActsExamples/Io/Csv/CsvSimHitReader.hpp" +#include "ActsExamples/Io/Csv/CsvSpacePointReader.hpp" #include "ActsExamples/Io/Root/RootMaterialTrackReader.hpp" #include "ActsExamples/Io/Root/RootParticleReader.hpp" #include "ActsExamples/Io/Root/RootTrajectorySummaryReader.hpp" @@ -169,5 +170,24 @@ void addInput(Context& ctx) { ACTS_PYTHON_MEMBER(outputSimHits); ACTS_PYTHON_STRUCT_END(); } + + { + using Reader = ActsExamples::CsvSpacePointReader; + using Config = Reader::Config; + auto reader = + py::class_>( + mex, "CsvSpacePointReader") + .def(py::init(), + py::arg("config"), py::arg("level")) + .def_property_readonly("config", &Reader::config); + + auto c = py::class_(reader, "Config").def(py::init<>()); + ACTS_PYTHON_STRUCT_BEGIN(c, Config); + ACTS_PYTHON_MEMBER(inputDir); + ACTS_PYTHON_MEMBER(inputStem); + ACTS_PYTHON_MEMBER(inputCollection); + ACTS_PYTHON_MEMBER(outputSpacePoints); + ACTS_PYTHON_STRUCT_END(); + } } -} // namespace Acts::Python \ No newline at end of file +} // namespace Acts::Python diff --git a/Examples/Python/src/TrackFinding.cpp b/Examples/Python/src/TrackFinding.cpp index 9916bc0314d..c0468ffca45 100644 --- a/Examples/Python/src/TrackFinding.cpp +++ b/Examples/Python/src/TrackFinding.cpp @@ -102,6 +102,29 @@ void addTrackFinding(Context& ctx) { ACTS_PYTHON_MEMBER(maxBlockSize); ACTS_PYTHON_MEMBER(nTrplPerSpBLimit); ACTS_PYTHON_MEMBER(nAvgTrplPerSpBLimit); + ACTS_PYTHON_MEMBER(impactMax); + ACTS_PYTHON_MEMBER(zBinEdges); + ACTS_PYTHON_MEMBER(enableCutsForSortedSP); + ACTS_PYTHON_MEMBER(zBinEdges); + ACTS_PYTHON_MEMBER(rRangeMiddleSP); + ACTS_PYTHON_MEMBER(useVariableMiddleSPRange); + ACTS_PYTHON_MEMBER(deltaRMiddleSPRange); + ACTS_PYTHON_MEMBER(seedConfirmation); + ACTS_PYTHON_MEMBER(centralSeedConfirmationRange); + ACTS_PYTHON_MEMBER(forwardSeedConfirmationRange); + ACTS_PYTHON_STRUCT_END(); + patchKwargsConstructor(c); + } + + { + using seedConf = Acts::SeedConfirmationRange; + auto c = py::class_(m, "SeedConfirmationRange").def(py::init<>()); + ACTS_PYTHON_STRUCT_BEGIN(c, seedConf); + ACTS_PYTHON_MEMBER(zMinSeedConf); + ACTS_PYTHON_MEMBER(zMaxSeedConf); + ACTS_PYTHON_MEMBER(rMaxSeedConf); + ACTS_PYTHON_MEMBER(nTopForLargeR); + ACTS_PYTHON_MEMBER(nTopForSmallR); ACTS_PYTHON_STRUCT_END(); patchKwargsConstructor(c); } @@ -118,6 +141,9 @@ void addTrackFinding(Context& ctx) { ACTS_PYTHON_MEMBER(zMin); ACTS_PYTHON_MEMBER(deltaRMax); ACTS_PYTHON_MEMBER(cotThetaMax); + ACTS_PYTHON_MEMBER(numPhiNeighbors); + ACTS_PYTHON_MEMBER(impactMax); + ACTS_PYTHON_MEMBER(zBinEdges); ACTS_PYTHON_STRUCT_END(); patchKwargsConstructor(c); } @@ -142,6 +168,8 @@ void addTrackFinding(Context& ctx) { ACTS_PYTHON_MEMBER(seedFilterConfig); ACTS_PYTHON_MEMBER(seedFinderConfig); ACTS_PYTHON_MEMBER(gridConfig); + ACTS_PYTHON_MEMBER(zBinNeighborsTop); + ACTS_PYTHON_MEMBER(zBinNeighborsBottom); ACTS_PYTHON_STRUCT_END(); } diff --git a/Examples/Scripts/Python/itk_seeding.py b/Examples/Scripts/Python/itk_seeding.py new file mode 100755 index 00000000000..12f829bf9b7 --- /dev/null +++ b/Examples/Scripts/Python/itk_seeding.py @@ -0,0 +1,179 @@ +#!/usr/bin/env python3 +import os +import argparse +import tempfile + +import acts +import acts.examples + +from acts.examples import CsvSpacePointReader + +u = acts.UnitConstants + + +def runITkSeeding(field, csvInputDir, outputDir, s=None): + + # Read input space points from input csv files + evReader = CsvSpacePointReader( + level=acts.logging.INFO, + inputStem="spacepoints", + inputCollection="pixel", + inputDir=csvInputDir, + outputSpacePoints="PixelSpacePoints", + ) + + gridConfig = acts.SpacePointGridConfig( + bFieldInZ=2 * u.T, + minPt=900 * u.MeV, + rMax=320 * u.mm, # pixel: 320 mm, strip: 1000 mm + zMax=3000 * u.mm, + zMin=-3000 * u.mm, + deltaRMax=280 * u.mm, # pixel: 280 mm, strip: 600 mm + cotThetaMax=27.2899, # pixel: 27.2899 (4 eta) + impactMax=2 * u.mm, # pixel: 2 mm, strip: 20 mm + zBinEdges=[ + -3000.0, + -2500.0, + -1400.0, + -925.0, + -450.0, + -250.0, + 250.0, + 450.0, + 925.0, + 1400.0, + 2500.0, + 3000.0, + ], # zBinEdges enables non-equidistant binning in z, in case the binning is not defined the edges are evaluated automatically using equidistant binning + numPhiNeighbors=1, # number of phiBin neighbors (plus the current bin) that covers the full deflection of a minimum pT particle + ) + + seedFinderConfig = acts.SeedfinderConfig( + rMax=gridConfig.rMax, + deltaRMin=20 * u.mm, + deltaRMax=gridConfig.deltaRMax, + deltaRMinTopSP=6 * u.mm, # pixel: 6 mm, strip: 20 mm + deltaRMinBottomSP=6 * u.mm, # pixel: 6 mm, strip: 20 mm + deltaRMaxTopSP=280 * u.mm, # pixel: 280 mm, strip: 3000 mm + deltaRMaxBottomSP=120 * u.mm, # pixel: 120 mm, strip: 3000 mm + collisionRegionMin=-200 * u.mm, + collisionRegionMax=200 * u.mm, + zMin=gridConfig.zMin, + zMax=gridConfig.zMax, + maxSeedsPerSpM=4, + cotThetaMax=gridConfig.cotThetaMax, + sigmaScattering=2, + radLengthPerSeed=0.1, + minPt=gridConfig.minPt, + bFieldInZ=gridConfig.bFieldInZ, + beamPos=acts.Vector2(0 * u.mm, 0 * u.mm), + impactMax=gridConfig.impactMax, + maxPtScattering=float("inf") * u.GeV, + zBinEdges=gridConfig.zBinEdges, + enableCutsForSortedSP=True, # enable cotTheta sorting in SeedFinder + rRangeMiddleSP=[ + [40.0, 90.0], + [40.0, 200.0], + [46.0, 200.0], + [46.0, 200.0], + [46.0, 250.0], + [46.0, 250.0], + [46.0, 250.0], + [46.0, 200.0], + [46.0, 200.0], + [40.0, 200.0], + [40.0, 90.0], + ], # if useVariableMiddleSPRange is set to false, the vector rRangeMiddleSP can be used to define a fixed r range for each z bin: {{rMin, rMax}, ...}. If useVariableMiddleSPRange is set to false and the vector is empty, the cuts won't be applied + useVariableMiddleSPRange=True, # if useVariableMiddleSPRange is true, the values in rRangeMiddleSP will be calculated based on r values of the SPs and deltaRMiddleSPRange + deltaRMiddleSPRange=10, + seedConfirmation=True, + centralSeedConfirmationRange=acts.SeedConfirmationRange( + zMinSeedConf=250 * u.mm, + zMaxSeedConf=250 * u.mm, + rMaxSeedConf=140 * u.mm, + nTopForLargeR=1, + nTopForSmallR=2, + ), # contains parameters for seed confirmation + forwardSeedConfirmationRange=acts.SeedConfirmationRange( + zMinSeedConf=3000 * u.mm, + zMaxSeedConf=-3000 * u.mm, + rMaxSeedConf=140 * u.mm, + nTopForLargeR=1, + nTopForSmallR=2, + ), + ) + + seedFilterConfig = acts.SeedFilterConfig( + maxSeedsPerSpM=seedFinderConfig.maxSeedsPerSpM, + deltaRMin=seedFinderConfig.deltaRMin, + impactWeightFactor=100, + compatSeedWeight=100, + compatSeedLimit=3, + ) + + seedingAlg = acts.examples.SeedingAlgorithm( + level=acts.logging.VERBOSE, + inputSpacePoints=[evReader.config.outputSpacePoints], + outputSeeds="PixelSeeds", + outputProtoTracks="prototracks", + gridConfig=gridConfig, + seedFinderConfig=seedFinderConfig, + seedFilterConfig=seedFilterConfig, + zBinNeighborsTop=[ + [0, 0], + [-1, 0], + [-1, 0], + [-1, 0], + [-1, 0], + [-1, 1], + [0, 1], + [0, 1], + [0, 1], + [0, 1], + [0, 0], + ], # allows to specify the number of neighbors desired for each bin, [-1,1] means one neighbor on the left and one on the right, if the vector is empty the algorithm returns the 8 surrounding bins + zBinNeighborsBottom=[ + [0, 1], + [0, 1], + [0, 1], + [0, 1], + [0, 1], + [0, 0], + [-1, 0], + [-1, 0], + [-1, 0], + [-1, 0], + [-1, 0], + ], + ) + + s = s or acts.examples.Sequencer( + events=1, numThreads=-1, logLevel=acts.logging.INFO + ) + + s.addReader(evReader) + s.addAlgorithm(seedingAlg) + + return s + + +if "__main__" == __name__: + + # create temporary file + with tempfile.TemporaryDirectory() as tmpdirname: + temp = open(tmpdirname + "/event000000000-spacepoints_pixel.csv", "w+t") + print( + "created temporary file: " + + tmpdirname + + "/event000000000-spacepoints_pixel.csv" + ) + temp.write( + "measurement_id,sp_type,module_idhash,sp_x,sp_y,sp_z,sp_radius,sp_covr,sp_covz\n 1,0,3139,32.67557144165039,-5.311902523040771,-47.65000152587891,33.10452270507812,0.05999999865889549,0.02999880164861679\n 2,0,3422,95.14442443847656,-15.46361255645752,-52.125,96.39286804199219,0.05999999865889549,0.01687432639300823\n 3,0,3650,102.8257064819336,-16.71612739562988,-52.67499923706055,104.1755981445312,0.05999999865889549,0.001875000074505806\n 4,0,4223,159.4266204833984,-25.91166687011719,-56.75,161.5186157226562,0.05999999865889549,0.02999880164861679\n 5,0,5015,224.07958984375,-36.37123107910156,-61.40000152587891,227.0121765136719,0.05999999865889549,0.007499700412154198\n 6,0,6023,284.1485595703125,-46.0638542175293,-65.72499847412109,287.8580932617188,0.05999999865889549,0.001875000074505806" + ) + temp.read() + + # set magnetic field + field = acts.ConstantBField(acts.Vector3(0, 0, 2 * u.T)) + + # run seeding + runITkSeeding(field, os.path.dirname(temp.name), outputDir=os.getcwd()).run() From be5fec486edee87e0b3d5db134e02ea663642631 Mon Sep 17 00:00:00 2001 From: Noemi Calace <58694235+noemina@users.noreply.github.com> Date: Fri, 18 Mar 2022 18:34:15 +0100 Subject: [PATCH 15/16] refactor!: Using non const InternalSpacePoint objects (#1196) This PR tries to fix usage of mutable properties in https://github.com/acts-project/acts/pull/1168 and https://github.com/acts-project/acts/pull/1143. Tagging: @paulgessinger @robertlangenberg @LuisFelipeCoelho BREAKING CHANGE: Remove assumption on constness of InternalSpacePoint. The need of this major change is to allow seed finding and filtering to evaluate and set properties of InternalSpacePoints as soon as they are candidates for triplet formation and seed quality evaluation. --- Core/include/Acts/Seeding/BinnedSPGroup.hpp | 4 +- Core/include/Acts/Seeding/BinnedSPGroup.ipp | 12 +++--- Core/include/Acts/Seeding/InternalSeed.hpp | 13 +++--- .../Acts/Seeding/InternalSpacePoint.hpp | 40 +++++++++++-------- Core/include/Acts/Seeding/SeedFilter.hpp | 6 +-- Core/include/Acts/Seeding/SeedFilter.ipp | 12 ++++-- Core/include/Acts/Seeding/SeedFinderUtils.hpp | 10 ++--- Core/include/Acts/Seeding/SeedFinderUtils.ipp | 11 ++--- Core/include/Acts/Seeding/Seedfinder.hpp | 7 ++-- Core/include/Acts/Seeding/SpacePointGrid.hpp | 3 +- .../Acts/Plugins/Cuda/Seeding/Seedfinder.ipp | 10 ++--- .../Acts/Plugins/Cuda/Seeding2/SeedFinder.ipp | 20 +++++----- .../Acts/Plugins/Sycl/Seeding/Seedfinder.ipp | 24 +++++------ 13 files changed, 88 insertions(+), 84 deletions(-) diff --git a/Core/include/Acts/Seeding/BinnedSPGroup.hpp b/Core/include/Acts/Seeding/BinnedSPGroup.hpp index 7fffa881b0e..f426da42b36 100644 --- a/Core/include/Acts/Seeding/BinnedSPGroup.hpp +++ b/Core/include/Acts/Seeding/BinnedSPGroup.hpp @@ -30,7 +30,7 @@ template class NeighborhoodIterator { public: using sp_it_t = typename std::vector>>::const_iterator; + InternalSpacePoint>>::const_iterator; NeighborhoodIterator() = delete; @@ -94,7 +94,7 @@ class NeighborhoodIterator { } } - const InternalSpacePoint* operator*() { + InternalSpacePoint* operator*() { return (*m_curIt).get(); } diff --git a/Core/include/Acts/Seeding/BinnedSPGroup.ipp b/Core/include/Acts/Seeding/BinnedSPGroup.ipp index 6246c99eca1..c7724c9af02 100644 --- a/Core/include/Acts/Seeding/BinnedSPGroup.ipp +++ b/Core/include/Acts/Seeding/BinnedSPGroup.ipp @@ -35,8 +35,8 @@ Acts::BinnedSPGroup::BinnedSPGroup( // create number of bins equal to number of millimeters rMax // (worst case minR: configured minR + 1mm) size_t numRBins = (config.rMax + config.beamPos.norm()); - std::vector>>> + std::vector< + std::vector>>> rBins(numRBins); for (spacepoint_iterator_t it = spBegin; it != spEnd; it++) { if (*it == nullptr) { @@ -58,9 +58,8 @@ Acts::BinnedSPGroup::BinnedSPGroup( continue; } - auto isp = - std::make_unique>( - sp, spPosition, config.beamPos, variance); + auto isp = std::make_unique>( + sp, spPosition, config.beamPos, variance); // calculate r-Bin index and protect against overflow (underflow not // possible) size_t rIndex = isp->radius(); @@ -75,8 +74,7 @@ Acts::BinnedSPGroup::BinnedSPGroup( for (auto& rbin : rBins) { for (auto& isp : rbin) { Acts::Vector2 spLocation(isp->phi(), isp->z()); - std::vector< - std::unique_ptr>>& + std::vector>>& bin = grid->atPosition(spLocation); bin.push_back(std::move(isp)); } diff --git a/Core/include/Acts/Seeding/InternalSeed.hpp b/Core/include/Acts/Seeding/InternalSeed.hpp index c22ee2e0ef3..28dd297400a 100644 --- a/Core/include/Acts/Seeding/InternalSeed.hpp +++ b/Core/include/Acts/Seeding/InternalSeed.hpp @@ -21,12 +21,12 @@ class InternalSeed { ///////////////////////////////////////////////////////////////////////////////// public: - InternalSeed(const InternalSpacePoint& s0, - const InternalSpacePoint& s1, - const InternalSpacePoint& s2, float z); + InternalSeed(InternalSpacePoint& s0, + InternalSpacePoint& s1, + InternalSpacePoint& s2, float z); InternalSeed& operator=(const InternalSeed& seed); - const std::array*, 3> sp; + const std::array*, 3> sp; float z() const { return m_z; } protected: @@ -49,9 +49,8 @@ inline InternalSeed& InternalSeed::operator=( template inline InternalSeed::InternalSeed( - const InternalSpacePoint& s0, - const InternalSpacePoint& s1, - const InternalSpacePoint& s2, float z) + InternalSpacePoint& s0, InternalSpacePoint& s1, + InternalSpacePoint& s2, float z) : sp({&s0, &s1, &s2}) { m_z = z; } diff --git a/Core/include/Acts/Seeding/InternalSpacePoint.hpp b/Core/include/Acts/Seeding/InternalSpacePoint.hpp index cb5ac8d0262..c7d980ee945 100644 --- a/Core/include/Acts/Seeding/InternalSpacePoint.hpp +++ b/Core/include/Acts/Seeding/InternalSpacePoint.hpp @@ -40,16 +40,32 @@ class InternalSpacePoint { float phi() const { return atan2f(m_y, m_x); } const float& varianceR() const { return m_varianceR; } const float& varianceZ() const { return m_varianceZ; } + const float& quality() const { return m_quality; } + const float& cotTheta() const { return m_cotTheta; } + void setCotTheta(float cotTheta) { m_cotTheta = cotTheta; } + void setQuality(float quality) { + if (quality >= m_quality) { + m_quality = quality; + } + } const SpacePoint& sp() const { return m_sp; } protected: - float m_x; // x-coordinate in beam system coordinates - float m_y; // y-coordinate in beam system coordinates - float m_z; // z-coordinate in beam system coordinetes - float m_r; // radius in beam system coordinates - float m_varianceR; // - float m_varianceZ; // - const SpacePoint& m_sp; // external space point + float m_x; // x-coordinate in beam system coordinates + float m_y; // y-coordinate in beam system coordinates + float m_z; // z-coordinate in beam system coordinetes + float m_r; // radius in beam system coordinates + float m_varianceR; // + float m_varianceZ; // + float m_cotTheta = std::numeric_limits< + double>::quiet_NaN(); // 1/tanTheta estimated from central+this space + // point. Its evaluation requires that the space + // point is a candidate for triplet search. + float m_quality = -std::numeric_limits< + double>::infinity(); // Quality score of the seed the space point is used + // for. Quality can be changed if the space point is + // used for a better quality seed. + const SpacePoint& m_sp; // external space point }; ///////////////////////////////////////////////////////////////////////////////// @@ -75,14 +91,6 @@ inline InternalSpacePoint::InternalSpacePoint( template inline InternalSpacePoint::InternalSpacePoint( - const InternalSpacePoint& sp) - : m_sp(sp.sp()) { - m_x = sp.m_x; - m_y = sp.m_y; - m_z = sp.m_z; - m_r = sp.m_r; - m_varianceR = sp.m_varianceR; - m_varianceZ = sp.m_varianceZ; -} + const InternalSpacePoint& sp) = default; } // end of namespace Acts diff --git a/Core/include/Acts/Seeding/SeedFilter.hpp b/Core/include/Acts/Seeding/SeedFilter.hpp index d9d1e927c8f..0b5761be138 100644 --- a/Core/include/Acts/Seeding/SeedFilter.hpp +++ b/Core/include/Acts/Seeding/SeedFilter.hpp @@ -42,9 +42,9 @@ class SeedFilter { /// @param zOrigin on the z axis as defined by bottom and middle space point /// @param outIt Output iterator for the seeds virtual void filterSeeds_2SpFixed( - const InternalSpacePoint& bottomSP, - const InternalSpacePoint& middleSP, - std::vector*>& topSpVec, + InternalSpacePoint& bottomSP, + InternalSpacePoint& middleSP, + std::vector*>& topSpVec, std::vector& invHelixDiameterVec, std::vector& impactParametersVec, float zOrigin, std::back_insert_iterator::SeedFilter( // return vector must contain weight of each seed template void SeedFilter::filterSeeds_2SpFixed( - const InternalSpacePoint& bottomSP, - const InternalSpacePoint& middleSP, - std::vector*>& topSpVec, + InternalSpacePoint& bottomSP, + InternalSpacePoint& middleSP, + std::vector*>& topSpVec, std::vector& invHelixDiameterVec, std::vector& impactParametersVec, float zOrigin, std::back_insert_iterator::filterSeeds_1SpFixed( // ordering by weight by filterSeeds_2SpFixed means these are the lowest // weight seeds for (; it < itBegin + maxSeeds; ++it) { + float bestSeedQuality = (*it).first; + + (*it).second->sp[0]->setQuality(bestSeedQuality); + (*it).second->sp[1]->setQuality(bestSeedQuality); + (*it).second->sp[2]->setQuality(bestSeedQuality); + outIt = Seed{ (*it).second->sp[0]->sp(), (*it).second->sp[1]->sp(), (*it).second->sp[2]->sp(), (*it).second->z()}; diff --git a/Core/include/Acts/Seeding/SeedFinderUtils.hpp b/Core/include/Acts/Seeding/SeedFinderUtils.hpp index 1e61cdaca09..a0fa62897e6 100644 --- a/Core/include/Acts/Seeding/SeedFinderUtils.hpp +++ b/Core/include/Acts/Seeding/SeedFinderUtils.hpp @@ -32,9 +32,9 @@ struct LinCircle { /// @param[in] spM The middle spacepoint to use. /// @param[in] bottom Should be true if sp is a bottom SP. template -LinCircle transformCoordinates( - const InternalSpacePoint& sp, - const InternalSpacePoint& spM, bool bottom); +LinCircle transformCoordinates(InternalSpacePoint& sp, + InternalSpacePoint& spM, + bool bottom); /// @brief Transform a vector of spacepoints to u-v space circles with respect /// to a given middle spacepoint. @@ -48,8 +48,8 @@ LinCircle transformCoordinates( /// @param[out] linCircleVec The output vector to write to. template void transformCoordinates( - const std::vector*>& vec, - const InternalSpacePoint& spM, bool bottom, + const std::vector*>& vec, + InternalSpacePoint& spM, bool bottom, bool enableCutsForSortedSP, std::vector& linCircleVec); } // namespace Acts diff --git a/Core/include/Acts/Seeding/SeedFinderUtils.ipp b/Core/include/Acts/Seeding/SeedFinderUtils.ipp index 18d1b09a125..63a27f02ca2 100644 --- a/Core/include/Acts/Seeding/SeedFinderUtils.ipp +++ b/Core/include/Acts/Seeding/SeedFinderUtils.ipp @@ -8,9 +8,9 @@ namespace Acts { template -LinCircle transformCoordinates( - const InternalSpacePoint& sp, - const InternalSpacePoint& spM, bool bottom) { +LinCircle transformCoordinates(InternalSpacePoint& sp, + InternalSpacePoint& spM, + bool bottom) { // The computation inside this function is exactly identical to that in the // vectorized version of this function, except that it operates on a single // spacepoint. Please see the other version of this function for more @@ -46,8 +46,8 @@ LinCircle transformCoordinates( template void transformCoordinates( - const std::vector*>& vec, - const InternalSpacePoint& spM, bool bottom, + const std::vector*>& vec, + InternalSpacePoint& spM, bool bottom, bool enableCutsForSortedSP, std::vector& linCircleVec) { float xM = spM.x(); float yM = spM.y(); @@ -93,6 +93,7 @@ void transformCoordinates( (cot_theta * cot_theta) * (varianceRM + sp->varianceR())) * iDeltaR2; linCircleVec.push_back(l); + sp->setCotTheta(cot_theta); } // sort the SP in order of cotTheta if (enableCutsForSortedSP) { diff --git a/Core/include/Acts/Seeding/Seedfinder.hpp b/Core/include/Acts/Seeding/Seedfinder.hpp index c300d2aa345..7c76e07399d 100644 --- a/Core/include/Acts/Seeding/Seedfinder.hpp +++ b/Core/include/Acts/Seeding/Seedfinder.hpp @@ -34,9 +34,8 @@ class Seedfinder { public: struct State { // bottom space point - std::vector*> - compatBottomSP; - std::vector*> compatTopSP; + std::vector*> compatBottomSP; + std::vector*> compatTopSP; // contains parameters required to calculate circle with linear equation // ...for bottom-middle std::vector linCircleBottom; @@ -44,7 +43,7 @@ class Seedfinder { std::vector linCircleTop; // create vectors here to avoid reallocation in each loop - std::vector*> topSpVec; + std::vector*> topSpVec; std::vector curvatures; std::vector impactParameters; std::vector etaVec; diff --git a/Core/include/Acts/Seeding/SpacePointGrid.hpp b/Core/include/Acts/Seeding/SpacePointGrid.hpp index e026a7fed49..dec2a665ca4 100644 --- a/Core/include/Acts/Seeding/SpacePointGrid.hpp +++ b/Core/include/Acts/Seeding/SpacePointGrid.hpp @@ -59,8 +59,7 @@ struct SpacePointGridConfig { template using SpacePointGrid = detail::Grid< - std::vector< - std::unique_ptr>>, + std::vector>>, detail::Axis, detail::Axis>; diff --git a/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding/Seedfinder.ipp b/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding/Seedfinder.ipp index 8c22b345a7d..3390fb17f84 100644 --- a/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding/Seedfinder.ipp +++ b/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding/Seedfinder.ipp @@ -66,11 +66,9 @@ Seedfinder::createSeedsForGroup( // Algorithm 0. Matrix Flattening //--------------------------------- - std::vector*> - middleSPvec; - std::vector*> - bottomSPvec; - std::vector*> topSPvec; + std::vector*> middleSPvec; + std::vector*> bottomSPvec; + std::vector*> topSPvec; // Get the size of spacepoints int nSpM(0); @@ -296,4 +294,4 @@ Seedfinder::createSeedsForGroup( ACTS_CUDA_ERROR_CHECK(cudaStreamDestroy(cuStream)); return outputVec; } -} // namespace Acts +} // namespace Acts \ No newline at end of file diff --git a/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding2/SeedFinder.ipp b/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding2/SeedFinder.ipp index 47167699a88..2e17aa11d14 100644 --- a/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding2/SeedFinder.ipp +++ b/Plugins/Cuda/include/Acts/Plugins/Cuda/Seeding2/SeedFinder.ipp @@ -81,18 +81,18 @@ SeedFinder::createSeedsForGroup( // Create more convenient vectors out of the space point containers. auto spVecMaker = [](sp_range_t spRange) { - std::vector*> result; + std::vector*> result; for (auto* sp : spRange) { result.push_back(sp); } return result; }; - std::vector*> - bottomSPVec(spVecMaker(bottomSPs)); - std::vector*> - middleSPVec(spVecMaker(middleSPs)); - std::vector*> topSPVec( + std::vector*> bottomSPVec( + spVecMaker(bottomSPs)); + std::vector*> middleSPVec( + spVecMaker(middleSPs)); + std::vector*> topSPVec( spVecMaker(topSPs)); // If either one of them is empty, we have nothing to find. @@ -193,12 +193,12 @@ SeedFinder::createSeedsForGroup( std::vector>>> seedsPerSPM; - const auto& middleSP = *(middleSPVec[middleIndex]); + auto& middleSP = *(middleSPVec[middleIndex]); for (const Details::Triplet& triplet : *triplet_itr) { assert(triplet.bottomIndex < bottomSPVec.size()); - const auto& bottomSP = *(bottomSPVec[triplet.bottomIndex]); + auto& bottomSP = *(bottomSPVec[triplet.bottomIndex]); assert(triplet.topIndex < topSPVec.size()); - const auto& topSP = *(topSPVec[triplet.topIndex]); + auto& topSP = *(topSPVec[triplet.topIndex]); seedsPerSPM.emplace_back(std::make_pair( triplet.weight, std::make_unique>( @@ -222,4 +222,4 @@ void SeedFinder::setLogger( } } // namespace Cuda -} // namespace Acts +} // namespace Acts \ No newline at end of file diff --git a/Plugins/Sycl/include/Acts/Plugins/Sycl/Seeding/Seedfinder.ipp b/Plugins/Sycl/include/Acts/Plugins/Sycl/Seeding/Seedfinder.ipp index 96dece6be9e..31d345ff3d4 100644 --- a/Plugins/Sycl/include/Acts/Plugins/Sycl/Seeding/Seedfinder.ipp +++ b/Plugins/Sycl/include/Acts/Plugins/Sycl/Seeding/Seedfinder.ipp @@ -84,37 +84,33 @@ Seedfinder::createSeedsForGroup( vecmem::vector deviceMiddleSPs(m_resource); vecmem::vector deviceTopSPs(m_resource); - std::vector*> - bottomSPvec; - std::vector*> - middleSPvec; - std::vector*> topSPvec; + std::vector*> bottomSPvec; + std::vector*> middleSPvec; + std::vector*> topSPvec; - for (const Acts::InternalSpacePoint* SP : bottomSPs) { + for (auto SP : bottomSPs) { bottomSPvec.push_back(SP); } deviceBottomSPs.reserve(bottomSPvec.size()); - for (const Acts::InternalSpacePoint* SP : - bottomSPvec) { + for (auto SP : bottomSPvec) { deviceBottomSPs.push_back({SP->x(), SP->y(), SP->z(), SP->radius(), SP->varianceR(), SP->varianceZ()}); } - for (const Acts::InternalSpacePoint* SP : middleSPs) { + for (auto SP : middleSPs) { middleSPvec.push_back(SP); } deviceMiddleSPs.reserve(middleSPvec.size()); - for (const Acts::InternalSpacePoint* SP : - middleSPvec) { + for (auto SP : middleSPvec) { deviceMiddleSPs.push_back({SP->x(), SP->y(), SP->z(), SP->radius(), SP->varianceR(), SP->varianceZ()}); } - for (const Acts::InternalSpacePoint* SP : topSPs) { + for (auto SP : topSPs) { topSPvec.push_back(SP); } deviceTopSPs.reserve(topSPvec.size()); - for (const Acts::InternalSpacePoint* SP : topSPvec) { + for (auto SP : topSPvec) { deviceTopSPs.push_back({SP->x(), SP->y(), SP->z(), SP->radius(), SP->varianceR(), SP->varianceZ()}); } @@ -149,4 +145,4 @@ Seedfinder::createSeedsForGroup( } return outputVec; } -} // namespace Acts::Sycl +} // namespace Acts::Sycl \ No newline at end of file From 4c9374cba530201f971160ad603cdc658eec32e8 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Mon, 21 Mar 2022 16:57:31 +0100 Subject: [PATCH 16/16] ci: Turn on FORCE_ASSERTIONS (#1191) In #1182 I unfortunately set the wrong CMake option to turn the forces assertions on. This PR should fix this. --- .github/workflows/builds.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/builds.yml b/.github/workflows/builds.yml index 4255e992109..5cae142f6be 100644 --- a/.github/workflows/builds.yml +++ b/.github/workflows/builds.yml @@ -53,7 +53,7 @@ jobs: -DACTS_BUILD_EVERYTHING=ON -DACTS_BUILD_EXAMPLES_PYTHON_BINDINGS=ON -DACTS_LOG_FAILURE_THRESHOLD=WARNING - -DACTS_BUILD_ASSERTIONS=ON + -DACTS_FORCE_ASSERTIONS=ON - name: Build run: ${SETUP} && cmake --build build -- - name: Unit tests @@ -130,7 +130,7 @@ jobs: -DACTS_BUILD_UNITTESTS=ON -DACTS_BUILD_INTEGRATIONTESTS=ON -DACTS_LOG_FAILURE_THRESHOLD=WARNING - -DACTS_BUILD_ASSERTIONS=ON + -DACTS_FORCE_ASSERTIONS=ON -DACTS_USE_SYSTEM_BOOST=OFF -DACTS_USE_SYSTEM_EIGEN3=OFF -DACTS_BUILD_PLUGIN_JSON=ON @@ -154,7 +154,7 @@ jobs: -DCMAKE_CXX_FLAGS=-Werror -DCMAKE_INSTALL_PREFIX="${INSTALL_DIR}" -DACTS_LOG_FAILURE_THRESHOLD=WARNING - -DACTS_BUILD_ASSERTIONS=ON + -DACTS_FORCE_ASSERTIONS=ON -DACTS_USE_SYSTEM_BOOST=OFF -DACTS_USE_SYSTEM_EIGEN3=OFF -DACTS_BUILD_EXAMPLES=ON @@ -181,7 +181,7 @@ jobs: && CI/check_boost_eigen_versions.sh ./build-downstream-nodeps/bin/ShowActsVersion macos: - runs-on: macos-10.15 + runs-on: macos-11 env: INSTALL_DIR: ${{ github.workspace }}/install steps: @@ -191,7 +191,7 @@ jobs: brew install cmake eigen ninja && sudo mkdir /usr/local/acts && sudo chown $USER /usr/local/acts - && wget --verbose --progress=dot:giga --continue --retry-connrefused --tries=5 --timeout=2 -O deps.tar.gz https://acts.web.cern.ch/ci/macOS/deps.43e0201.tar.gz + && wget --verbose --progress=dot:giga --continue --retry-connrefused --tries=5 --timeout=2 -O deps.tar.gz https://acts.web.cern.ch/ci/macOS/deps.494c52d.tar.gz && tar -xf deps.tar.gz -C /usr/local/acts - name: Configure # setting CMAKE_CXX_STANDARD=17 is a workaround for a bug in the @@ -207,7 +207,7 @@ jobs: -DCMAKE_PREFIX_PATH=/usr/local/acts -DACTS_BUILD_EVERYTHING=ON -DACTS_LOG_FAILURE_THRESHOLD=WARNING - -DACTS_BUILD_ASSERTIONS=ON + -DACTS_FORCE_ASSERTIONS=ON - name: Build run: cmake --build build -- - name: Unit tests