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 1/5] 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.))) ^ From 93e3b27d35b751a47f239997d44dcf768638ecf2 Mon Sep 17 00:00:00 2001 From: Tim Adye Date: Wed, 11 Dec 2024 13:07:28 +0000 Subject: [PATCH 2/5] fix: update full_chain_test.py to follow full_chain_odd.py (#3956) * support `--output-obj` like #3180. Can also still use `-cc` to output all formats. * Fuse initial and final sim particles when `--edm4hep` input specified (sync with #3804) Next, we should remove the command-line options for `full_chain_odd.py`. ## Summary by CodeRabbit - **New Features** - Introduced a new `--output-obj` argument for enabling object file output. - **Improvements** - Updated help text for the `--output-csv` argument to clarify output options. - Enhanced output directory handling to support multiple output formats. --- Examples/Scripts/Python/full_chain_test.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/Examples/Scripts/Python/full_chain_test.py b/Examples/Scripts/Python/full_chain_test.py index e7c945094f6..f09be753d46 100755 --- a/Examples/Scripts/Python/full_chain_test.py +++ b/Examples/Scripts/Python/full_chain_test.py @@ -81,7 +81,12 @@ def parse_args(): "--output-csv", action="count", default=0, - help="Use CSV output instead of ROOT. Specify -cc to output both.", + help="Use CSV output instead of ROOT. Specify -cc to output all formats (ROOT, CSV, and obj).", + ) + parser.add_argument( + "--output-obj", + action="store_true", + help="Enable obj output", ) parser.add_argument( "-n", @@ -272,6 +277,11 @@ def full_chain(args): outputDirCsv = outputDir if args.output_csv != 0 else None outputDirLessCsv = outputDirLess if args.output_csv != 0 else None outputDirMoreCsv = outputDirMore if args.output_csv != 0 else None + outputDirObj = ( + outputDirLess + if args.output_obj + else outputDir if args.output_csv == 2 else None + ) # fmt: off if args.generic_detector: @@ -392,8 +402,7 @@ def full_chain(args): "LongStripEndcapReadout", ], outputParticlesGenerator="particles_input", - outputParticlesInitial="particles_initial", - outputParticlesFinal="particles_final", + outputParticlesSimulation="particles_simulated", outputSimHits="simhits", graphvizOutput="graphviz", dd4hepDetector=detector, @@ -483,6 +492,7 @@ def full_chain(args): postSelectParticles=postSelectParticles, outputDirRoot=outputDirRoot, outputDirCsv=outputDirCsv, + outputDirObj=outputDirObj, ) else: if s.config.numThreads != 1: @@ -508,6 +518,7 @@ def full_chain(args): killAfterTime=25 * u.ns, outputDirRoot=outputDirRoot, outputDirCsv=outputDirCsv, + outputDirObj=outputDirObj, ) addDigitization( From 595f2e67788ad608381ac4c75db5377865d0f2dd Mon Sep 17 00:00:00 2001 From: Benjamin Huth <37871400+benjaminhuth@users.noreply.github.com> Date: Wed, 11 Dec 2024 15:58:42 +0100 Subject: [PATCH 3/5] ci: Enable standard library assertions in debug builds (#3759) Should add assertions that check preconditions of calls to standardlibrary functions. ## Summary by CodeRabbit - **Chores** - Enhanced configuration for compiler flags to conditionally enable assertions. - Addressed compilation issues with GCC 13 by undefining specific preprocessor directives. - Maintained existing logic for compiler type checks without altering public entity signatures. - **Bug Fixes** - Adjusted the logic for generating random directions to ensure correct bounds for angles. - **Documentation** - Improved comments for clarity on the calculations in the random direction generation function. --- Core/src/Visualization/ObjVisualization3D.cpp | 2 +- Plugins/Json/src/DetrayJsonHelper.cpp | 7 +++++++ .../UnitTests/Core/Utilities/AlgebraHelpersTests.cpp | 11 +++-------- cmake/ActsCompilerOptions.cmake | 5 +++++ 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/Core/src/Visualization/ObjVisualization3D.cpp b/Core/src/Visualization/ObjVisualization3D.cpp index 777e883b3d3..6895ad56f03 100644 --- a/Core/src/Visualization/ObjVisualization3D.cpp +++ b/Core/src/Visualization/ObjVisualization3D.cpp @@ -63,7 +63,7 @@ void ObjVisualization3D::faces(const std::vector& vtxs, o.vertices.insert(o.vertices.end(), vtxs.begin(), vtxs.end()); for (const auto& face : faces) { if (face.size() == 2) { - o.lines.push_back({face[0] + vtxoffs, face[2] + vtxoffs}); + o.lines.push_back({face[0] + vtxoffs, face[1] + vtxoffs}); } else { FaceType rawFace; std::ranges::transform( diff --git a/Plugins/Json/src/DetrayJsonHelper.cpp b/Plugins/Json/src/DetrayJsonHelper.cpp index b04d3e62b11..d17e6cee1d5 100644 --- a/Plugins/Json/src/DetrayJsonHelper.cpp +++ b/Plugins/Json/src/DetrayJsonHelper.cpp @@ -6,6 +6,13 @@ // 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/. +// For whatever reason, this compilation unit does not compile +// with those assertions and GCC 13. For now just disable the +// flags in this case. +#if defined(_GLIBCXX_ASSERTIONS) && __GNUC__ == 13 +#undef _GLIBCXX_ASSERTIONS +#endif + #include "Acts/Plugins/Json/DetrayJsonHelper.hpp" namespace Acts::DetrayJsonHelper { diff --git a/Tests/UnitTests/Core/Utilities/AlgebraHelpersTests.cpp b/Tests/UnitTests/Core/Utilities/AlgebraHelpersTests.cpp index 87e47d3e3a9..a821682d795 100644 --- a/Tests/UnitTests/Core/Utilities/AlgebraHelpersTests.cpp +++ b/Tests/UnitTests/Core/Utilities/AlgebraHelpersTests.cpp @@ -82,18 +82,15 @@ BOOST_AUTO_TEST_CASE(SafeInverseFPESmallMatrix) { m(1, 1) = 1; auto mInv = Acts::safeInverse(m); + BOOST_REQUIRE(mInv.has_value()); auto mInvInv = Acts::safeInverse(*mInv); - - BOOST_CHECK(mInv); BOOST_CHECK(!mInvInv); ACTS_VERBOSE("Test: SafeInverseFPESmallMatrix" << "\n" << "m:\n" << m << "\n" << "mInv:\n" - << *mInv << "\n" - << "mInvInv [garbage]:\n" - << *mInvInv); + << *mInv); } BOOST_AUTO_TEST_CASE(SafeInverseFPELargeMatrix) { @@ -107,9 +104,7 @@ BOOST_AUTO_TEST_CASE(SafeInverseFPELargeMatrix) { ACTS_VERBOSE("Test: SafeInverseFPELargeMatrix" << "\n" << "m:\n" - << m << "\n" - << "mInv [garbage]:\n" - << *mInv); + << m); } /// This test should not compile diff --git a/cmake/ActsCompilerOptions.cmake b/cmake/ActsCompilerOptions.cmake index 64467c64665..44c215ca8d5 100644 --- a/cmake/ActsCompilerOptions.cmake +++ b/cmake/ActsCompilerOptions.cmake @@ -13,6 +13,11 @@ set(cxx_flags "-Wall -Wextra -Wpedantic -Wshadow -Wzero-as-null-pointer-constant -Wold-style-cast" ) +# Add assertions to standard libraries +if(ACTS_FORCE_ASSERTIONS) + set(cxx_flags "${cxx_flags} -D_GLIBCXX_ASSERTIONS -D_LIBCPP_DEBUG") +endif() + # This adds some useful conversion checks like float-to-bool, float-to-int, etc. # However, at the moment this is only added to clang builds, since GCC's -Wfloat-conversion # is much more aggressive and also triggers on e.g., double-to-float From 72fc7cb3c886aac01e5e0edc74e449abc7d0b320 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Wed, 11 Dec 2024 18:52:19 +0100 Subject: [PATCH 4/5] chore: Update .gitignore (#3979) - Un-ignore the `.github` folder - Restrict the `build` exclude pattern to the repository root. --- .gitignore | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index c0a676728ea..dba5bbf393b 100644 --- a/.gitignore +++ b/.gitignore @@ -15,7 +15,7 @@ genConf x86_64-slc6-gcc48-opt x86_64-slc6-gcc49-opt .* -build* +/build* .asetup.save #json output @@ -30,7 +30,7 @@ build* # dont ignore hidden configs !.clang-format -!.github/** +!.github !.gitignore !.gitlab-ci.yml !.clang-tidy From 2d5ed729db9cba874d560670f8f035400feb1fee Mon Sep 17 00:00:00 2001 From: Doga Elitez <108287101+delitez@users.noreply.github.com> Date: Thu, 12 Dec 2024 13:57:18 +0100 Subject: [PATCH 5/5] fix: Update full_chain_odd_LRT.py to follow full_chain_odd.py (#3977) In this PR full_chain_odd_LRT.py is updated according to full_chain_odd.py. ## Summary by CodeRabbit - **New Features** - Introduced a command-line argument `--output-obj` to toggle object output. - **Enhancements** - Updated ambiguity resolution configuration to use mathematical constants for `phiMax` and `phiMin`. - Added new parameters for seeding and track selection to improve configurability and precision. - Changed the vertex fitting method to a more advanced algorithm. These updates enhance the script's functionality and improve the particle tracking and reconstruction processes. --- Examples/Scripts/Python/full_chain_odd_LRT.py | 51 ++++++++++++++++--- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/Examples/Scripts/Python/full_chain_odd_LRT.py b/Examples/Scripts/Python/full_chain_odd_LRT.py index 36a552af0c8..9c1f57074f6 100644 --- a/Examples/Scripts/Python/full_chain_odd_LRT.py +++ b/Examples/Scripts/Python/full_chain_odd_LRT.py @@ -3,6 +3,7 @@ import os import argparse import pathlib +import math import acts import acts.examples @@ -12,6 +13,7 @@ EtaConfig, PhiConfig, ParticleConfig, + ParticleSelectorConfig, addPythia8, addFatras, addGeant4, @@ -134,6 +136,12 @@ default=True, action=argparse.BooleanOptionalAction, ) +parser.add_argument( + "--output-obj", + help="Switch obj output on/off", + default=True, + action=argparse.BooleanOptionalAction, +) args = parser.parse_args() @@ -224,7 +232,6 @@ ParticleConfig( args.gun_particles, acts.PdgParticle.eMuon, randomizeCharge=True ), - outputDirRoot=pathlib.Path("/home/aicha/Atlas/NewVertices"), vtxGen=acts.examples.GaussianDisplacedVertexPositionGenerator( rMean=2, rStdDev=0.0125 * u.mm, @@ -278,6 +285,7 @@ ), outputDirRoot=outputDir if args.output_root else None, outputDirCsv=outputDir if args.output_csv else None, + outputDirObj=outputDir if args.output_obj else None, rnd=rnd, killVolume=trackingGeometry.highestTrackingVolume, killAfterTime=25 * u.ns, @@ -306,6 +314,7 @@ enableInteractions=True, outputDirRoot=outputDir if args.output_root else None, outputDirCsv=outputDir if args.output_csv else None, + outputDirObj=outputDir if args.output_obj else None, rnd=rnd, ) @@ -324,6 +333,16 @@ s, trackingGeometry, field, + initialSigmas=[ + 1 * u.mm, + 1 * u.mm, + 1 * u.degree, + 1 * u.degree, + 0.1 * u.e / u.GeV, + 1 * u.ns, + ], + initialSigmaPtRel=0.1, + initialVarInflation=[1.0] * 6, geoSelectionConfigFile=oddSeedingSel, outputDirRoot=outputDir if args.output_root else None, outputDirCsv=outputDir if args.output_csv else None, @@ -354,12 +373,32 @@ maxOutliers=2, ), CkfConfig( + chi2CutOffMeasurement=15.0, + chi2CutOffOutlier=25.0, + numMeasurementsCutOff=10, seedDeduplication=True, stayOnSeed=True, - pixelVolumes={16, 17, 18}, - stripVolumes={23, 24, 25}, + pixelVolumes=[16, 17, 18], + stripVolumes=[23, 24, 25], maxPixelHoles=1, maxStripHoles=2, + constrainToVolumes=[ + 2, # beam pipe + 32, + 4, # beam pip gap + 16, + 17, + 18, # pixel + 20, # PST + 23, + 24, + 25, # short strip + 26, + 8, # long strip gap + 28, + 29, + 30, # long strip + ], ), outputDirRoot=outputDir if args.output_root else None, outputDirCsv=outputDir if args.output_csv else None, @@ -388,8 +427,8 @@ maxSharedTracksPerMeasurement=2, pTMax=1400, pTMin=0.5, - phiMax=3.14, - phiMin=-3.14, + phiMax=math.pi, + phiMin=-math.pi, etaMax=4, etaMin=-4, useAmbiguityFunction=False, @@ -413,7 +452,7 @@ addVertexFitting( s, field, - vertexFinder=VertexFinder.Iterative, + vertexFinder=VertexFinder.AMVF, outputDirRoot=outputDir if args.output_root else None, )