From 5a54bf4fd79614483f3e3c1b041e089f7198b806 Mon Sep 17 00:00:00 2001 From: Pavel Larionov Date: Mon, 14 Jul 2025 14:04:27 +0200 Subject: [PATCH 1/4] Fix TGeoSurfaceConverter header bug --- .../Acts/Plugins/Root/TGeoSurfaceConverter.hpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp b/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp index f226d41663c..b53d963f493 100644 --- a/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp +++ b/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp @@ -93,10 +93,13 @@ struct TGeoSurfaceConverter { /// @param degree The input in degree /// @return angle in radians static double toRadian(double degree) { - if (degree > 180. && degree < 360.) { - degree -= 360.; - } - return degree / 180. * std::numbers::pi; + // Normalize degree to [-180, 180) + degree = fmod(degree + 180.0, 360.0); + if (degree < 0) + degree += 360.0; + degree -= 180.0; + + return degree / 180.0 * std::numbers::pi; } }; From 6d47103ba6ff132f2d81b26683b2717e931af59e Mon Sep 17 00:00:00 2001 From: Pavel Larionov Date: Tue, 15 Jul 2025 12:24:26 +0200 Subject: [PATCH 2/4] Make use of UnitConstants and Helpers --- .../include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp b/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp index b53d963f493..a11ec19acf4 100644 --- a/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp +++ b/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp @@ -9,6 +9,8 @@ #pragma once #include "Acts/Definitions/Algebra.hpp" +#include "Acts/Definitions/Units.hpp" +#include "Acts/Utilities/detail/periodic.hpp" #include #include @@ -93,13 +95,7 @@ struct TGeoSurfaceConverter { /// @param degree The input in degree /// @return angle in radians static double toRadian(double degree) { - // Normalize degree to [-180, 180) - degree = fmod(degree + 180.0, 360.0); - if (degree < 0) - degree += 360.0; - degree -= 180.0; - - return degree / 180.0 * std::numbers::pi; + return detail::radian_sym(degree * UnitConstants::degree); } }; From a055262c50cd2f9f65891c675db3cfc6eda3d550 Mon Sep 17 00:00:00 2001 From: Pavel Larionov Date: Mon, 28 Jul 2025 19:28:59 +0200 Subject: [PATCH 3/4] Add unit test for toRadian conversion --- Tests/UnitTests/Plugins/Root/CMakeLists.txt | 1 + .../Root/TGeoToRadianConversionTests.cpp | 71 +++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 Tests/UnitTests/Plugins/Root/TGeoToRadianConversionTests.cpp diff --git a/Tests/UnitTests/Plugins/Root/CMakeLists.txt b/Tests/UnitTests/Plugins/Root/CMakeLists.txt index ee3d12d7de8..99001ab9b5f 100644 --- a/Tests/UnitTests/Plugins/Root/CMakeLists.txt +++ b/Tests/UnitTests/Plugins/Root/CMakeLists.txt @@ -11,3 +11,4 @@ if(${ROOT_VERSION} VERSION_GREATER "6.25.01") endif() add_unittest(TGeoParser TGeoParserTests.cpp) add_unittest(TGeoPrimitivesHelper TGeoPrimitivesHelperTests.cpp) +add_unittest(TGeoToRadianConversionTests TGeoToRadianConversionTests.cpp) diff --git a/Tests/UnitTests/Plugins/Root/TGeoToRadianConversionTests.cpp b/Tests/UnitTests/Plugins/Root/TGeoToRadianConversionTests.cpp new file mode 100644 index 00000000000..35cbd4290fb --- /dev/null +++ b/Tests/UnitTests/Plugins/Root/TGeoToRadianConversionTests.cpp @@ -0,0 +1,71 @@ +// 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 + +#include "Acts/Definitions/Tolerance.hpp" +#include "Acts/Definitions/Units.hpp" +#include "Acts/Plugins/Root/TGeoSurfaceConverter.hpp" +#include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" + +#include +#include +#include +#include +#include + +namespace Acts::Test { + +BOOST_AUTO_TEST_CASE(ToRadian_WrappingCoverage) { + using std::numbers::pi; + + // {input in degrees, expected radians (wrapped to [-pi, pi))} + std::vector> testCases = { + // In-range + {0.0, 0.0}, + {90.0, pi / 2.0}, + {180.0, -pi}, + {270.0, -pi / 2.0}, + {359.0, -1.0 * UnitConstants::degree}, + {1.0, 1.0 * UnitConstants::degree}, + {45.0, pi / 4.0}, + {135.0, 3.0 * pi / 4.0}, + {225.0, -3.0 * pi / 4.0}, + {315.0, -pi / 4.0}, + + // Just below 360 + {360.0 - 1e-10, -1e-10 * UnitConstants::degree}, + + // Negative values + {-1.0, -1.0 * UnitConstants::degree}, + {-90.0, -pi / 2.0}, + {-180.0, -pi}, + {-270.0, pi / 2.0}, + {-360.0, 0.0}, + {-450.0, -pi / 2.0}, + + // > 360 values + {361.0, 1.0 * UnitConstants::degree}, + {450.0, pi / 2.0}, + {720.0, 0.0}, + {810.0, pi / 2.0}, + {1080.0 + 45.0, pi / 4.0}, + + // Large positive and negative inputs + {1260.0, -pi}, + {-1260.0, -pi}, + + }; + + for (const auto& [degInput, expectedWrappedRad] : testCases) { + const double actual = TGeoSurfaceConverter::toRadian(degInput); + CHECK_CLOSE_ABS(actual, expectedWrappedRad, s_epsilon); + } +} + +} // namespace Acts::Test From 266697b9777c789d8447bda89cb062246a8c523d Mon Sep 17 00:00:00 2001 From: Pavel Larionov Date: Fri, 15 Aug 2025 13:23:22 +0200 Subject: [PATCH 4/4] Calculate the phi span correctly, add unit test --- .../Plugins/Root/TGeoSurfaceConverter.hpp | 23 ++++++ Plugins/Root/src/TGeoSurfaceConverter.cpp | 25 +++---- Tests/UnitTests/Plugins/Root/CMakeLists.txt | 1 + .../Plugins/Root/TGeoToRadianSpanTests.cpp | 72 +++++++++++++++++++ 4 files changed, 109 insertions(+), 12 deletions(-) create mode 100644 Tests/UnitTests/Plugins/Root/TGeoToRadianSpanTests.cpp diff --git a/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp b/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp index a11ec19acf4..6a6a220b49d 100644 --- a/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp +++ b/Plugins/Root/include/Acts/Plugins/Root/TGeoSurfaceConverter.hpp @@ -97,6 +97,29 @@ struct TGeoSurfaceConverter { static double toRadian(double degree) { return detail::radian_sym(degree * UnitConstants::degree); } + + /// Compute the angular extent (halfPhi) and center angle (avgPhi), + /// handling periodicity and wraparound across 0°. The returned angles are in radians. + /// + /// The average phi is wrapped into the range [-pi, pi). + /// + /// @param deg1 Starting angle in degrees + /// @param deg2 Ending angle in degrees + /// @return pair {halfPhi, avgPhi} in radians + static std::pair toRadianSpan(double deg1, double deg2) { + double phi1 = deg1 * UnitConstants::degree; + double phi2 = deg2 * UnitConstants::degree; + + // Proper arc length with wraparound (e.g. 0° to 360° = 2π) + double span = + Acts::detail::difference_periodic(phi2, phi1, 2 * std::numbers::pi); + + // Compute average phi in [−π, π) (for symmetry in bounds) + double avg = Acts::detail::radian_sym(phi1 + 0.5 * span); + + return {span * 0.5, avg}; // halfPhi, avgPhi + } + }; } // namespace Acts diff --git a/Plugins/Root/src/TGeoSurfaceConverter.cpp b/Plugins/Root/src/TGeoSurfaceConverter.cpp index 55b1441f65d..8c7299b2525 100644 --- a/Plugins/Root/src/TGeoSurfaceConverter.cpp +++ b/Plugins/Root/src/TGeoSurfaceConverter.cpp @@ -93,9 +93,12 @@ Acts::TGeoSurfaceConverter::cylinderComponents(const TGeoShape& tgShape, // Check if it's a segment auto tubeSeg = dynamic_cast(tube); if (tubeSeg != nullptr) { - double phi1 = toRadian(tubeSeg->GetPhi1()); - double phi2 = toRadian(tubeSeg->GetPhi2()); - if (std::abs(phi2 - phi1) < std::numbers::pi * (1. - s_epsilon)) { + double deg1 = tubeSeg->GetPhi1(); + double deg2 = tubeSeg->GetPhi2(); + std::tie(halfPhi, avgPhi) = toRadianSpan(deg1, deg2); + double span = 2.0 * halfPhi; + + if (span < std::numbers::pi * (1. - s_epsilon)) { if (!boost::starts_with(axes, "X")) { throw std::invalid_argument( "TGeoShape -> CylinderSurface (sectorial): can only be " @@ -103,8 +106,6 @@ Acts::TGeoSurfaceConverter::cylinderComponents(const TGeoShape& tgShape, "with " "'(X)(y/Y)(*)' axes."); } - halfPhi = 0.5 * (std::max(phi1, phi2) - std::min(phi1, phi2)); - avgPhi = 0.5 * (phi1 + phi2); } } bounds = std::make_shared(medR, halfZ, halfPhi, avgPhi); @@ -252,18 +253,18 @@ Acts::TGeoSurfaceConverter::discComponents(const TGeoShape& tgShape, // Check if it's a segment auto tubeSeg = dynamic_cast(tube); if (tubeSeg != nullptr) { - double phi1 = toRadian(tubeSeg->GetPhi1()); - double phi2 = toRadian(tubeSeg->GetPhi2()); - if (std::abs(phi2 - phi1) < 2 * std::numbers::pi * (1. - s_epsilon)) { + double deg1 = tubeSeg->GetPhi1(); + double deg2 = tubeSeg->GetPhi2(); + std::tie(halfPhi, avgPhi) = toRadianSpan(deg1, deg2); + double span = 2.0 * halfPhi; + + if (span < std::numbers::pi * (1. - s_epsilon)) { if (!boost::starts_with(axes, "X")) { throw std::invalid_argument( - "TGeoShape -> CylinderSurface (sectorial): can only be " - "converted " + "TGeoShape -> DiscSurface (sectorial): can only be converted " "with " "'(X)(y/Y)(*)' axes."); } - halfPhi = 0.5 * (std::max(phi1, phi2) - std::min(phi1, phi2)); - avgPhi = 0.5 * (phi1 + phi2); } } bounds = std::make_shared(minR, maxR, halfPhi, avgPhi); diff --git a/Tests/UnitTests/Plugins/Root/CMakeLists.txt b/Tests/UnitTests/Plugins/Root/CMakeLists.txt index 99001ab9b5f..21ef965c0a1 100644 --- a/Tests/UnitTests/Plugins/Root/CMakeLists.txt +++ b/Tests/UnitTests/Plugins/Root/CMakeLists.txt @@ -12,3 +12,4 @@ endif() add_unittest(TGeoParser TGeoParserTests.cpp) add_unittest(TGeoPrimitivesHelper TGeoPrimitivesHelperTests.cpp) add_unittest(TGeoToRadianConversionTests TGeoToRadianConversionTests.cpp) +add_unittest(TGeoToRadianSpanTests TGeoToRadianSpanTests.cpp) diff --git a/Tests/UnitTests/Plugins/Root/TGeoToRadianSpanTests.cpp b/Tests/UnitTests/Plugins/Root/TGeoToRadianSpanTests.cpp new file mode 100644 index 00000000000..6ffe0f4802d --- /dev/null +++ b/Tests/UnitTests/Plugins/Root/TGeoToRadianSpanTests.cpp @@ -0,0 +1,72 @@ +// 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 + +#include "Acts/Definitions/Tolerance.hpp" +#include "Acts/Definitions/Units.hpp" +#include "Acts/Plugins/Root/TGeoSurfaceConverter.hpp" +#include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" + +#include +#include +#include +#include + +namespace Acts::Test { + +BOOST_AUTO_TEST_CASE(ToRadianSpan_Correctness) { + using std::numbers::pi; + + struct TestCase { + double deg1; + double deg2; + double expectedHalfSpan; + double expectedAvg; + }; + + std::vector testCases = { + // Simple cases + {0.0, 90.0, pi / 4.0, pi / 4.0}, + {90.0, 180.0, pi / 4.0, 3.0 * pi / 4.0}, + {0.0, 360.0, pi, 0.0}, + {360.0, 720.0, pi, 0.0}, + {-180.0, 180.0, 0.0, pi}, + + // Wraparound case (crosses 0°) + {315.0, 45.0, pi / 4.0, 0.0}, + + // Reverse direction (same as above, but in opposite order) + {45.0, 315.0, 3.0 * pi / 4.0, pi}, + + // Small wraparound + {359.0, 1.0, UnitConstants::degree, 0.0}, + + // Large positive range + {0.0, 1125.0, pi / 8.0, pi / 8.0}, + + // Large negative to large positive + {-720.0, 90.0, pi / 4.0, pi / 4.0}, + + // Custom case + {316.79, 403.21, 43.21 * UnitConstants::degree * 0.5, + Acts::detail::radian_sym((316.79 + 0.5 * 43.21) * UnitConstants::degree)}, + }; + + for (const auto& [deg1, deg2, expectedHalfSpan, expectedAvg] : testCases) { + const auto [actualHalfSpan, actualAvg] = + Acts::TGeoSurfaceConverter::toRadianSpan(deg1, deg2); + + BOOST_TEST_CONTEXT("deg1 = " << deg1 << ", deg2 = " << deg2) { + CHECK_CLOSE_ABS(actualHalfSpan, expectedHalfSpan, s_epsilon); + CHECK_CLOSE_ABS(actualAvg, expectedAvg, s_epsilon); + } + } +} + +} // namespace Acts::Test