Skip to content

Commit

Permalink
fix: Ensure correct order of uniform distribution limits (#3976)
Browse files Browse the repository at this point in the history
Pulled out of #3759 

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->

## 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.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
benjaminhuth authored Dec 11, 2024
1 parent 6c910a6 commit 08a3953
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 7 deletions.
13 changes: 9 additions & 4 deletions Core/include/Acts/EventData/detail/GenerateParameters.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,24 @@ template <typename generator_t>
inline std::pair<double, double> generateBoundDirection(
generator_t& rng, const GenerateBoundDirectionOptions& options) {
using UniformReal = std::uniform_real_distribution<double>;
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<double>::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);
Expand Down
6 changes: 3 additions & 3 deletions Tests/UnitTests/Core/Geometry/CylinderVolumeBuilderTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<double>(-11., -15.))) ^
std::uniform_real_distribution<double>(-15., -11.))) ^
bdata::random((bdata::engine = std::mt19937(), bdata::seed = 2,
bdata::distribution =
std::uniform_real_distribution<double>(11., 15.))) ^
Expand Down Expand Up @@ -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<double>(-11., -15.))) ^
std::uniform_real_distribution<double>(-15., -11.))) ^
bdata::random((bdata::engine = std::mt19937(), bdata::seed = 2,
bdata::distribution =
std::uniform_real_distribution<double>(11., 15.))) ^
Expand Down Expand Up @@ -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<double>(-11., -15.))) ^
std::uniform_real_distribution<double>(-15., -11.))) ^
bdata::random((bdata::engine = std::mt19937(), bdata::seed = 2,
bdata::distribution =
std::uniform_real_distribution<double>(11., 15.))) ^
Expand Down

0 comments on commit 08a3953

Please sign in to comment.