feat: GeoShapes to DetectorVolumes and GeoMaterial Converters #3268
Conversation
github-actions
bot
added
Infrastructure
Matthewharri
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3268 +/- ##
==========================================
- Coverage 47.37% 47.31% -0.06%
==========================================
Files 511 512 +1
Lines 30249 30437 +188
Branches 14677 14795 +118
==========================================
+ Hits 14329 14400 +71
- Misses 5386 5404 +18
- Partials 10534 10633 +99 ☔ View full report in Codecov by Sentry. |
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelToDetVol.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelToDetVol.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelTree.hpp
Outdated
Show resolved
Hide resolved
Matthewharri
changed the title
github-actions
bot
added
the
Infrastructure
|
Apparently there was, but I think it should be fixed now. |
Looking at the compile command It seems that GeoModel is not included using |
|
This is blocked by #3476 which is blocked by having an external build for GeoModel |
Bumps GeoModel version and deals with the necessary changes. This is a subset of #3268 which isolates the version change and tackles the CI errors faced there. --------- Co-authored-by: Paul Gessinger <[email protected]>
github-actions
bot
removed
the
Infrastructure
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelMaterialConverter.hpp
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelMaterialConverter.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelMaterialConverter.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelToDetectorVolume.hpp
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelToDetectorVolume.hpp
Outdated
Show resolved
Hide resolved
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelToDetectorVolume.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Andreas Stefl <[email protected]>
|
@andiwand Ok, I addressed your things, so assuming nothing got messed up the CI should be fine, and then this is good to go |
|
should we override sonarcloud in this case @paulgessinger ? |
|
WalkthroughNew source and header files added to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GeoModel
participant DetectorVolume
User->>GeoModel: Request material conversion
GeoModel->>GeoModel: geoMaterialConverter(GeoMaterial)
GeoModel-->>User: Return Acts::Material
User->>GeoModel: Request volume conversion
GeoModel->>GeoModel: convertVolume(GeoShape)
GeoModel->>DetectorVolume: Create DetectorVolume
GeoModel-->>User: Return DetectorVolume
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelMaterialConverter.hpp (3)
13-13: Specify namespace for forward declaration, hmmmm.Improve clarity and prevent potential naming conflicts, we must. Add namespace specification to forward declaration, I suggest.
-class GeoMaterial; +namespace GeoModel { class GeoMaterial; }
17-21: Documentation incomplete, it is. Missing parameter details, I sense.Document the
useMolarDensityparameter and its effects, we must. Help users understand the conversion behavior better, this will./// @brief Convert GeoMaterial to Acts::Material /// /// @param gm The GeoMaterial to be converted +/// @param useMolarDensity Whether to use molar density in conversion /// @return the Acts::Material
21-22: Excessive indentation, I observe.Align parameters with more conventional spacing, we should. Improve readability, this will.
-Material geoMaterialConverter(const GeoMaterial* gm, - bool useMolarDensity = true); +Material geoMaterialConverter(const GeoMaterial* gm, + bool useMolarDensity = true);Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelToDetectorVolume.hpp (1)
19-26: Incomplete documentation and specifications, the Force reveals.Improve the documentation and function specifications, we must:
- Missing @param for 'name' parameter, it is
- Return value description, needs more detail it does
- Thread safety and ownership transfer, unclear they remain
Apply these changes, you should:
/// @brief Convert a GeoModel shape to a DetectorVolume /// /// @param shape the GeoModel shape + /// @param context the geometry context + /// @param name the volume name /// @param transform the transform to be applied - /// @return the DetectorVolume + /// @return shared pointer to the created DetectorVolume + /// + /// @note Thread-safe: yes + /// @note The returned volume takes ownership of all created components std::shared_ptr<Experimental::DetectorVolume> convertVolume( const GeometryContext& context, const GeoShape* shape, - const std::string& name, const GeoTrf::Transform3D transform); + const std::string& name, const GeoTrf::Transform3D transform) noexcept;Plugins/GeoModel/src/GeoModelMaterialConverter.cpp (3)
18-18: Document the density conversion factor, we must!Mysterious, this conversion factor is. Add a comment explaining its purpose and units, you should.
+ // Convert density from GeoModel units (g) to Acts units (g/cm³) constexpr double densityCnvFactor = 1. / GeoModelKernelUnits::gram;
24-32: Improve the loop's readability and safety, we shall!More descriptive comments and range-based loop, beneficial they are. Magic numbers, avoid we must.
- // Get number elements - int numberOfElements = gm->getNumElements(); - // Loop - for (int iEl = 0; iEl < numberOfElements; ++iEl) { - const GeoElement* geoEl = gm->getElement(iEl); - double fraction = gm->getFraction(iEl); + // Accumulate atomic mass and number weighted by element fractions + const auto numberOfElements = gm->getNumElements(); + for (auto iEl = 0; iEl < numberOfElements; ++iEl) { + const auto* geoEl = gm->getElement(iEl); + const auto fraction = gm->getFraction(iEl);
16-38: Consider thread safety and caching, you must!Thread-safe, this converter is, as no shared state it maintains. Yet, for performance, cache frequently converted materials, you might. A singleton cache, helpful it could be, if same materials converted often they are.
Plugins/GeoModel/src/GeoModelToDetectorVolume.cpp (4)
83-83: Careful with floating-point comparisons, you must be.Directly comparing floating-point values using
<=, you are. Due to precision errors, unexpected results may occur. Use an epsilon tolerance for comparison, consider.Apply this diff to adjust the comparison:
- if (x1 <= x2) { + if (x1 <= x2 + epsilon) {Define
epsilonas a small value appropriately.
159-164: Consistent, the conditional structure is not.An
ifcondition outside theelse ifchain, there is. Include it within the chain, you should, for consistency and readability.Apply this diff to adjust the conditional structure:
- if (shape->typeID() == GeoShapeShift::getClassTypeID()) { + else if (shape->typeID() == GeoShapeShift::getClassTypeID()) {
129-129: Improve the error message, you should.More informative, the error message could be. Include dimensions or shape details, you might, to aid in debugging.
Apply this diff to enhance the error message:
- throw std::runtime_error("FATAL: Translating GeoTrd to ACTS failed"); + throw std::runtime_error("FATAL: Translating GeoTrd to ACTS failed. Unsupported configuration with x1 != x2 and y1 != y2.");
165-165: Include the shape typeID in the error message, you should.When throwing for unsupported shapes, include
shape->typeID()in the message, you should, to aid in debugging.Apply this diff to enhance the error message:
- throw std::runtime_error("FATAL: Unsupported GeoModel shape"); + throw std::runtime_error("FATAL: Unsupported GeoModel shape with typeID: " + std::to_string(shape->typeID()));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Plugins/GeoModel/CMakeLists.txt(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelMaterialConverter.hpp(1 hunks)Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelToDetectorVolume.hpp(1 hunks)Plugins/GeoModel/src/GeoModelMaterialConverter.cpp(1 hunks)Plugins/GeoModel/src/GeoModelToDetectorVolume.cpp(1 hunks)
🔇 Additional comments (6)
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelMaterialConverter.hpp (1)
21-22: Verify the usage of this converter function across the codebase, we must.
Search for potential callers and ensure consistent usage patterns, we shall.
Plugins/GeoModel/include/Acts/Plugins/GeoModel/GeoModelToDetectorVolume.hpp (1)
1-9: Correct, the header protection is, hmmmm.
Strong with the Force, this header protection is. Well-structured license header, I see.
Plugins/GeoModel/CMakeLists.txt (2)
Line range hint 29-35: Sufficient these dependencies are, but verify we must!
Public dependencies declared correctly they are:
- ActsCore
- GeoModelCore::GeoModelKernel
- GeoModelIO::GeoModelDBManager
- GeoModelIO::GeoModelRead
But check for version compatibility, we should!
✅ Verification successful
Verified the dependencies are, clear version requirements we found!
Found the GeoModel version requirements we have:
- GeoModelCore 6.3.0 required it is
- GeoModelIO 6.3.0 required it is
- Properly managed through CMake variable
_acts_geomodel_versionthese dependencies are
In harmony with the Force these dependencies are, properly declared and versioned they remain. No disturbance in the version compatibility I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check GeoModel version requirements
# Search for version requirements in CMake files
echo "=== GeoModel version requirements, searching for we are ==="
rg -t cmake "find_package.*GeoModel" || echo "No explicit version requirements found, concerning this is"
Length of output: 341
Script:
#!/bin/bash
# Search for the value of _acts_geomodel_version variable
echo "=== The GeoModel version variable, seeking we are ==="
rg -t cmake "_acts_geomodel_version" -B 2 -A 2
# Check if there are any other version-related variables or requirements
echo -e "\n=== Additional version requirements, searching for we are ==="
rg -t cmake "GeoModel.*VERSION|VERSION.*GeoModel"
Length of output: 921
8-8: Hmmmm, proper additions these source files are!
Align with the PR objectives, these new files do. For shape and material conversion, essential they are.
Verify the dependencies we must, hmmmm:
Also applies to: 12-12
✅ Verification successful
In balance, the dependencies are, young padawan!
Examined the Force flowing through the code, I have. The dependencies in CMakeLists.txt, sufficient they are! Hmmmm.
ActsCoreprovides the geometry and material components, it doesGeoModelKernelsupports all shape conversions, yesGeoModelDBManagerandGeoModelReadfor proper functionality, present they are
All required headers in the new files, properly included they are:
- For shape conversion, geometry bounds and navigation, the Force is strong
- For material conversion, the necessary components present they are
No disturbance in the Force, I sense. The CMake configuration, complete and balanced it is!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if new files have additional dependencies
# Search for includes in new files
echo "=== Dependencies in new files, searching for we are ==="
rg -t cpp --no-filename "^#include" src/GeoModelToDetectorVolume.cpp src/GeoModelMaterialConverter.cpp || echo "No includes found, suspicious this is"
Length of output: 457
Script:
#!/bin/bash
# Search for the correct path of source files, we must
echo "=== Search for the source files location, we shall ==="
fd -t f "GeoModelToDetectorVolume.cpp|GeoModelMaterialConverter.cpp"
echo "=== Structure of the plugin directory, examine we must ==="
ls -R Plugins/GeoModel/
Length of output: 1672
Script:
#!/bin/bash
# Examine the includes in the correct path, we must
echo "=== Dependencies in GeoModelToDetectorVolume.cpp, reveal we shall ==="
rg -t cpp --no-filename "^#include" Plugins/GeoModel/src/GeoModelToDetectorVolume.cpp
echo "=== Dependencies in GeoModelMaterialConverter.cpp, investigate we must ==="
rg -t cpp --no-filename "^#include" Plugins/GeoModel/src/GeoModelMaterialConverter.cpp
echo "=== CMake dependencies, check we shall ==="
cat Plugins/GeoModel/CMakeLists.txt
Length of output: 2826
Plugins/GeoModel/src/GeoModelMaterialConverter.cpp (2)
1-15: Correct, the header organization is, hmmmm!
Well-structured, the includes are. In proper order, they flow - from local to external dependencies, they do.
33-38: Wise choice of factory methods, this is!
Clear and concise, the creation of Material objects is. Proper use of factory methods, you have shown.
| #include "Acts/Detector/DetectorVolume.hpp" | ||
|
|
||
| #include "GeoModelKernel/GeoDefinitions.h" | ||
|
|
||
| class GeoShape; |
There was a problem hiding this comment.
Missing includes, I sense.
Required for your journey, these includes are:
- GeometryContext, used in function signature it is
- Transform3D from GeoTrf namespace, needed it appears
Add these includes, you must:
#include "Acts/Detector/DetectorVolume.hpp"
+#include "Acts/Geometry/GeometryContext.hpp"
#include "GeoModelKernel/GeoDefinitions.h"
+#include "GeoModelKernel/GeoTrf.h"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include "Acts/Detector/DetectorVolume.hpp" | |
| #include "GeoModelKernel/GeoDefinitions.h" | |
| class GeoShape; | |
| #include "Acts/Detector/DetectorVolume.hpp" | |
| #include "Acts/Geometry/GeometryContext.hpp" | |
| #include "GeoModelKernel/GeoDefinitions.h" | |
| #include "GeoModelKernel/GeoTrf.h" | |
| class GeoShape; |
| std::shared_ptr<Experimental::DetectorVolume> convertVolume( | ||
| const GeometryContext& context, const GeoShape* shape, | ||
| const std::string& name, const GeoTrf::Transform3D transform); |
There was a problem hiding this comment.
💡 Codebase verification
Limited shape handling, I sense in this code.
In GeoModelToDetectorVolume.cpp, handle only these shapes, the implementation does:
- GeoShapeUnion
- GeoShapeSubtraction
- GeoPcon
- GeoShapeShift
But many more shapes in the GeoModel universe, there are. A default error throwing for unsupported shapes, I see. Dangerous this path is.
- Missing handling for basic shapes like boxes, tubes, and trapezoids, the implementation is
- Default error message "FATAL: Unsupported GeoModel shape" provides little guidance to young padawans
- Consider implementing converters for all fundamental GeoModel shapes, you must
🔗 Analysis chain
Verify shape handling completeness, we must.
Ensure that all GeoModel shapes, handled they are. Search the implementation, we shall.
The search continues, young padawan. More information about shape handling, we must find. The implementation of convertVolume and shape converters, reveal they will.
Hmm, elusive the implementation is. Through different paths, search we must. The Force guides us to look deeper into the GeoModel plugin's implementation files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all GeoShape derived classes and verify implementation handles them
# Find all GeoShape derived classes
echo "=== GeoShape derived classes ==="
ast-grep --pattern 'class $_ : public GeoShape'
# Find shape handling in implementation
echo -e "\n=== Shape handling in implementation ==="
rg "dynamic_cast.*GeoShape" -A 5
Length of output: 2378
Script:
#!/bin/bash
# Search for implementation of convertVolume
echo "=== Implementation of convertVolume ==="
rg "convertVolume.*GeoShape.*transform" -A 20
# Find all shape converters
echo -e "\n=== Shape converters ==="
fd "converter" --exec rg "class.*Converter" {}
Length of output: 443
Script:
#!/bin/bash
# Search for implementation files in GeoModel plugin
echo "=== GeoModel implementation files ==="
fd "GeoModelTo.*cpp$"
# Search for shape handling in implementation
echo -e "\n=== Shape handling patterns ==="
rg "GeoShape.*\{" -A 10 --type cpp
Length of output: 9474
| Acts::Material Acts::GeoModel::geoMaterialConverter(const GeoMaterial* gm, | ||
| bool useMolarDensity) { | ||
| constexpr double densityCnvFactor = 1. / GeoModelKernelUnits::gram; | ||
| double x0 = gm->getRadLength(); | ||
| double l0 = gm->getIntLength(); | ||
| double density = gm->getDensity() * densityCnvFactor; | ||
| double A = 0.; |
There was a problem hiding this comment.
Validate the input pointer, we must!
Dangerous, null pointers are. Check them, we should, before proceeding.
Acts::Material Acts::GeoModel::geoMaterialConverter(const GeoMaterial* gm,
bool useMolarDensity) {
+ if (gm == nullptr) {
+ throw std::invalid_argument("Null GeoMaterial pointer provided");
+ }
constexpr double densityCnvFactor = 1. / GeoModelKernelUnits::gram;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Acts::Material Acts::GeoModel::geoMaterialConverter(const GeoMaterial* gm, | |
| bool useMolarDensity) { | |
| constexpr double densityCnvFactor = 1. / GeoModelKernelUnits::gram; | |
| double x0 = gm->getRadLength(); | |
| double l0 = gm->getIntLength(); | |
| double density = gm->getDensity() * densityCnvFactor; | |
| double A = 0.; | |
| Acts::Material Acts::GeoModel::geoMaterialConverter(const GeoMaterial* gm, | |
| bool useMolarDensity) { | |
| if (gm == nullptr) { | |
| throw std::invalid_argument("Null GeoMaterial pointer provided"); | |
| } | |
| constexpr double densityCnvFactor = 1. / GeoModelKernelUnits::gram; | |
| double x0 = gm->getRadLength(); | |
| double l0 = gm->getIntLength(); | |
| double density = gm->getDensity() * densityCnvFactor; | |
| double A = 0.; |
| double xmin{0}, xmax{0}, ymin{0}, ymax{0}, zmin{0}, zmax{0}; | ||
| brep->extent(xmin, ymin, zmin, xmax, ymax, zmax); | ||
| std::shared_ptr<CuboidVolumeBounds> bounds = | ||
| std::make_shared<CuboidVolumeBounds>( | ||
| (xmax - xmin) / 2, (ymax - ymin) / 2, (zmax - zmin) / 2); | ||
| return Experimental::DetectorVolumeFactory::construct( | ||
| portalGenerator, context, name, transform, bounds, | ||
| Experimental::tryAllPortalsAndSurfaces()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the duplicated code, you should.
Duplicate code for calculating CuboidVolumeBounds from shape extents, I see. A helper function, create, to improve maintainability and reduce repetition.
|
how do we proceed with this @Matthewharri @noemina @junggjo9 @paulgessinger ? |
|
I will close this as it was merged in with a different PR |
This PR introduces a converter to convert geoShapes to DetectorVolumes. Some of the shapes added are very simple converters, i.e. simple bounding boxes which will probably be changed later on. I also bump the GeoModel version from 4.6.0 to 6.0.0 since Athena is using a much newer version. I also change (with the help of @paulgessinger) how cmake looks for the system installation of GeoModel.
I also include a way to convert
GeoMaterialtoActs::Material@junggjo9
blocked by
Summary by CodeRabbit
New Features
Bug Fixes
Documentation