From 08a3953b3197282ac8a1e0b98309193b7c166867 Mon Sep 17 00:00:00 2001 From: Benjamin Huth <37871400+benjaminhuth@users.noreply.github.com> Date: Wed, 11 Dec 2024 12:47:19 +0100 Subject: [PATCH] fix: Ensure correct order of uniform distribution limits (#3976) Pulled out of #3759 ## Summary by CodeRabbit - **Bug Fixes** - Enhanced validation for direction generation parameters to ensure they are within acceptable ranges. - Adjusted calculations for cosine and eta values to align with the properties of the cosine function. - **Tests** - Modified random distribution ranges in unit tests for the `CylinderVolumeBuilder` to improve test coverage and accuracy. --- .../Acts/EventData/detail/GenerateParameters.hpp | 13 +++++++++---- .../Core/Geometry/CylinderVolumeBuilderTests.cpp | 6 +++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/Core/include/Acts/EventData/detail/GenerateParameters.hpp b/Core/include/Acts/EventData/detail/GenerateParameters.hpp index b06d853faa6..630878e7d4a 100644 --- a/Core/include/Acts/EventData/detail/GenerateParameters.hpp +++ b/Core/include/Acts/EventData/detail/GenerateParameters.hpp @@ -102,19 +102,24 @@ template inline std::pair generateBoundDirection( generator_t& rng, const GenerateBoundDirectionOptions& options) { using UniformReal = std::uniform_real_distribution; + assert(options.thetaMin >= 0.f); + assert(options.thetaMax <= std::numbers::pi); + assert(options.thetaMin <= options.thetaMax); // since we want to draw the direction uniform on the unit sphere, we must // draw from cos(theta) instead of theta. see e.g. // https://mathworld.wolfram.com/SpherePointPicking.html - double cosThetaMin = std::cos(options.thetaMin); + // Get cosThetaMin from thetaMax and vice versa, because cos is + // monothonical decreasing between [0, pi] + double cosThetaMin = std::cos(options.thetaMax); // ensure upper bound is included. see e.g. // https://en.cppreference.com/w/cpp/numeric/random/uniform_real_distribution - double cosThetaMax = std::nextafter(std::cos(options.thetaMax), + double cosThetaMax = std::nextafter(std::cos(options.thetaMin), std::numeric_limits::max()); // in case we force uniform eta generation - double etaMin = Acts::AngleHelpers::etaFromTheta(options.thetaMin); - double etaMax = Acts::AngleHelpers::etaFromTheta(options.thetaMax); + double etaMin = Acts::AngleHelpers::etaFromTheta(options.thetaMax); + double etaMax = Acts::AngleHelpers::etaFromTheta(options.thetaMin); UniformReal phiDist(options.phiMin, options.phiMax); UniformReal cosThetaDist(cosThetaMin, cosThetaMax); diff --git a/Tests/UnitTests/Core/Geometry/CylinderVolumeBuilderTests.cpp b/Tests/UnitTests/Core/Geometry/CylinderVolumeBuilderTests.cpp index e7a96099bb0..9ab6f7392dc 100644 --- a/Tests/UnitTests/Core/Geometry/CylinderVolumeBuilderTests.cpp +++ b/Tests/UnitTests/Core/Geometry/CylinderVolumeBuilderTests.cpp @@ -22,7 +22,7 @@ BOOST_DATA_TEST_CASE( CylinderVolumeBuilder_wraps, bdata::random((bdata::engine = std::mt19937(), bdata::seed = 1, bdata::distribution = - std::uniform_real_distribution(-11., -15.))) ^ + std::uniform_real_distribution(-15., -11.))) ^ bdata::random((bdata::engine = std::mt19937(), bdata::seed = 2, bdata::distribution = std::uniform_real_distribution(11., 15.))) ^ @@ -114,7 +114,7 @@ BOOST_DATA_TEST_CASE( CylinderVolumeBuilder_containes, bdata::random((bdata::engine = std::mt19937(), bdata::seed = 1, bdata::distribution = - std::uniform_real_distribution(-11., -15.))) ^ + std::uniform_real_distribution(-15., -11.))) ^ bdata::random((bdata::engine = std::mt19937(), bdata::seed = 2, bdata::distribution = std::uniform_real_distribution(11., 15.))) ^ @@ -285,7 +285,7 @@ BOOST_DATA_TEST_CASE( CylinderVolumeBuilder_overlapsInZ, bdata::random((bdata::engine = std::mt19937(), bdata::seed = 1, bdata::distribution = - std::uniform_real_distribution(-11., -15.))) ^ + std::uniform_real_distribution(-15., -11.))) ^ bdata::random((bdata::engine = std::mt19937(), bdata::seed = 2, bdata::distribution = std::uniform_real_distribution(11., 15.))) ^