feat: Gen3 blueprint geometry construction #3869
Conversation
WalkthroughA significant update to the ACTS project, this pull request introduces several new classes and enhances existing ones to improve geometric construction capabilities. Key additions include the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
paulgessinger
force-pushed
the
feat/gen3-blueprints
branch
from
7e42d9a to
9f3ba06
Compare
github-actions
bot
added
Component - Core
This is useful in combination with `std::visit`. With this, you can do something like
```cpp
struct A {};
std::variant<int, double, A> var;
var = 42;
std::visit(overloaded{
[](int) { BOOST_CHECK(true); },
[](double) { BOOST_CHECK(false); },
[](A) { BOOST_CHECK(false); },
},
var);
```
which can be convenient.
I'm using this in #3869
paulgessinger
force-pushed
the
feat/gen3-blueprints
branch
3 times, most recently
from
96b31ed to
fcc1af9
Compare
paulgessinger
force-pushed
the
feat/gen3-blueprints
branch
from
d52ca29 to
61422d9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (29)
Core/include/Acts/Geometry/StaticBlueprintNode.hpp (4)
18-18: Typo in documentation, corrected it must be."constructred" should be "constructed".
Apply this diff to fix the typo:
- /// The static blueprint node wraps a single already-constructred @c TrackingVolume. + /// The static blueprint node wraps a single already-constructed @c TrackingVolume.
53-54: By const reference, pass 'navigationPolicyFactory', consider you should.Passing
std::shared_ptrby value may incur unnecessary overhead. More efficient it is to pass by const reference if ownership transfer not intended.Apply this diff to enhance efficiency:
- virtual StaticBlueprintNode& setNavigationPolicyFactory( - std::shared_ptr<NavigationPolicyFactory> navigationPolicyFactory); + virtual StaticBlueprintNode& setNavigationPolicyFactory( + const std::shared_ptr<NavigationPolicyFactory>& navigationPolicyFactory);
56-57: Avoid returning raw pointers, perhaps you should.Returning a raw pointer in
navigationPolicyFactory()may be unsafe. Consider returning aconst std::shared_ptr<NavigationPolicyFactory>&instead.Apply this diff to improve safety:
- const NavigationPolicyFactory* navigationPolicyFactory() const; + const std::shared_ptr<NavigationPolicyFactory>& navigationPolicyFactory() const;
65-65: Redundant initialization to nullptr, it is.Explicitly initializing
std::shared_ptrtonullptrunnecessary, as default initialization already sets it tonullptr.Apply this diff to simplify the code:
- std::shared_ptr<NavigationPolicyFactory> m_navigationPolicyFactory = nullptr; + std::shared_ptr<NavigationPolicyFactory> m_navigationPolicyFactory;Core/include/Acts/Utilities/Delegate.hpp (1)
Line range hint
104-105: Typographical errors, correct we must.In comments, 'owner ship' should be 'ownership', and 'it's lifetime' should be 'its lifetime'.
Apply this diff to fix the errors:
-/// @note @c Delegate does not assume owner ship over @p callable. You need to ensure -/// it's lifetime is longer than that of @c Delegate. +/// @note @c Delegate does not assume ownership over @p callable. You need to ensure +/// its lifetime is longer than that of @c Delegate.Also applies to: 139-140, 152-153, 203-204
Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp (3)
20-21: Typo in comment, correct it you must"and how do to so" — change to "and how to do so", you should.
22-23: Typo in comment, "constuction" it is"constuction" — should be "construction", fix it you must.
29-31: Simplify BinningConfig type, consider you shouldOnly one type in
std::variantyou have. Directly use the type, perhaps you can.Core/include/Acts/Geometry/Blueprint.hpp (1)
50-51: Typo in comment, "inteded" it is"inteded" — should be "intended", fix this you must.
Core/src/Geometry/Blueprint.cpp (1)
85-112: Refactor bounds modification, consider you should.The logic for adjusting
CylinderVolumeBoundsis sound, but refactoring into a helper function, you might. Enhance readability and reusability, it will.Extract the bounds adjustment code into a separate method, consider.
Core/src/Geometry/MaterialDesignatorBlueprintNode.cpp (1)
69-130: Type-specific handling withstd::visit, good practice it is.Using
std::visitto handle different binning configurations, you are. However, extensive nested lambdas can reduce readability.Extract lambdas into named functions or simplify the visitor pattern, consider you should, to improve clarity.
Core/include/Acts/Geometry/BlueprintNode.hpp (3)
176-176: Consider renaming parameter 'cylinder' to 'node', you should.Improve clarity, this will, as the parameter represents a
StaticBlueprintNode.
188-188: Consider renaming parameter 'cylinder' to 'node', you should.Improve clarity, this will, as the parameter represents a
StaticBlueprintNode.
261-261: Correct the spelling of 'representation', you must.A typographical error in the comment there is. 'representatiohn' should be 'representation'.
docs/core/geometry/index.md (1)
Fix the link reference, you must, young padawan
The Force reveals that
layerless_geometryanchor exists indocs/core/geometry/layerless/layerless.md. Change the link to[](layerless/layerless), you should, for proper Markdown reference it is.🔗 Analysis chain
Line range hint
12-16: Incomplete, the layerless geometry link appears to be, hmm.Reference to
[](layerless_geometry)broken it seems. Verify the correct anchor or path, you must.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for layerless_geometry anchor in documentation rg -g "*.md" "layerless_geometry"Length of output: 275
Script:
#!/bin/bash # Check the content of the layerless geometry documentation cat docs/core/geometry/layerless/layerless.mdLength of output: 6417
Core/include/Acts/Geometry/BlueprintOptions.hpp (1)
17-26: Strong with the Force, this design is.Well-structured the
BlueprintOptionsclass is, with proper memory management throughunique_ptrit shows. Yet, improvements suggest I must:
- Documentation for the class and its members, missing it is
- Consider
[[nodiscard]]formakeDefaultNavigationPolicyFactory, prevent ignored results we shouldApply these changes, you should:
struct BlueprintOptions { + /// Factory for creating default navigation policies std::unique_ptr<NavigationPolicyFactory> defaultNavigationPolicyFactory{ makeDefaultNavigationPolicyFactory()}; + /// Validates the options configuration + /// @throw std::invalid_argument if navigation policy factory is null void validate() const; private: + /// Creates the default navigation policy factory + /// @return Unique pointer to the created factory + [[nodiscard]] static std::unique_ptr<NavigationPolicyFactory> makeDefaultNavigationPolicyFactory(); };Core/src/Geometry/BlueprintOptions.cpp (1)
16-20: More descriptive, the error message should be, hmm.Simple but effective the validation is, yet more context in error message provide we should.
Improve the error message thus:
if (!defaultNavigationPolicyFactory) { - throw std::invalid_argument("Navigation policy factory is nullptr"); + throw std::invalid_argument( + "BlueprintOptions: defaultNavigationPolicyFactory is nullptr. " + "A valid navigation policy factory must be provided for geometry construction."); }Examples/Python/tests/test_blueprint.py (4)
1-2: Remove unused import, you must.Import pytest without use, I sense. Clean code, we maintain.
-import pytest🧰 Tools
🪛 Ruff (0.8.0)
1-1:
pytestimported but unusedRemove unused import:
pytest(F401)
15-22: Documentation missing, it is. Add docstring, you should.Function purpose and parameters, document we must. Help future Jedi understand the code, it will.
def test_zdirection_container_blueprint(tmp_path): + """Test blueprint construction with z-direction containers. + + Args: + tmp_path: Temporary directory path for test artifacts + """
29-33: Magic numbers in geometry calculations, I sense.Constants for geometric values, define we should. More maintainable and clear, the code will be.
+# Define geometric constants +RADIUS_INCREMENT = 50 * mm +BARREL_THICKNESS = 20 * mm +BARREL_LENGTH = 200 * mm + r = 25 * mm for i in range(1, 3): - r += 50 * mm - bounds = acts.CylinderVolumeBounds(r, r + 20 * mm, 200 * mm) + r += RADIUS_INCREMENT + bounds = acts.CylinderVolumeBounds(r, r + BARREL_THICKNESS, BARREL_LENGTH)
17-21: Cleanup of generated files, consider you must.Files generated during tests, cleanup we should. Use context manager or teardown, we can.
+ def cleanup_files(stage): + gz = tmp_path / f"blueprint_{stage}.dot" + if gz.exists(): + gz.unlink() + def write(root: acts.BlueprintNode, stage: int): gz = tmp_path / f"blueprint_{stage}.dot" print(gz) with gz.open("w") as fh: root.graphViz(fh) + # Optional: cleanup after test completion + # cleanup_files(stage)Examples/Scripts/Python/blueprint.py (2)
61-64: Combine nested with statements, we shall.Cleaner code structure, this will create. Multiple contexts in single with statement, use we must.
- with pixel.Material() as mat: - with mat.CylinderContainer( - direction=acts.BinningValue.binZ, name="PixelNegEndcap" - ) as ec: + with pixel.Material() as mat, mat.CylinderContainer( + direction=acts.BinningValue.binZ, name="PixelNegEndcap" + ) as ec:🧰 Tools
🪛 Ruff (0.8.0)
61-64: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
10-14: Configuration values, extract we must.Separate configuration from logic, better practice it is. More maintainable and flexible, the code becomes.
+# Blueprint configuration +BLUEPRINT_CONFIG = { + "envelope": acts.ExtentEnvelope(r=[10 * mm, 10 * mm]), + "container_name": "Pixel", +} + -root = acts.Blueprint(envelope=acts.ExtentEnvelope(r=[10 * mm, 10 * mm])) +root = acts.Blueprint(envelope=BLUEPRINT_CONFIG["envelope"]) -pixel = root.addCylinderContainer(direction=acts.BinningValue.binZ, name="Pixel") +pixel = root.addCylinderContainer( + direction=acts.BinningValue.binZ, + name=BLUEPRINT_CONFIG["container_name"] +)Plugins/Json/src/GridJsonConverter.cpp (1)
442-446: Wise error handling, this is! Prevent silent failures, it will.Handle unsupported types explicitly now, we do. Better error messages for developers, this brings.
Consider adding error details to the message, you should:
- throw std::invalid_argument( - "GridAccessJsonConverter: boundToGridLocal type not supported."); + throw std::invalid_argument( + "GridAccessJsonConverter: boundToGridLocal type not supported. " + "Supported types are: LocalSubspace, BoundCylinderToZPhi.");docs/core/geometry/construction.md (1)
12-28: Empty sections, a path to confusion they are.Hmmmm. Each section, at least a brief description or placeholder content, should have. Help users understand what to expect, it will.
Assist in drafting content for these sections, I can. Create a new issue to track this documentation task, shall we? The force of good documentation, strong it must be.
docs/core/geometry/concepts.md (2)
15-38: The portal system, explain better we must.Hmmmm. The relationship between different portal types, unclear it is. Before each class reference, explain its purpose and use cases, we should.
## Portals + +Portals serve as gateways between volumes in the tracking geometry. +They enable navigation and define how particles traverse the detector. :::{doxygenclass} Acts::Portal ::: ### Portal links + +Portal links define how volumes are connected through portals: + +- TrivialPortalLink: Simple direct connections +- GridPortalLink: Grid-based volume connections +- CompositePortalLink: Complex multi-portal connections
42-42: Navigation policy, crucial for the force of geometry traversal it is.Empty, this important section remains. Document how navigation decisions are made and how they integrate with the portal system, we must.
Help create comprehensive content for navigation policies, I can. A new issue to track this task, shall we create?
Core/include/Acts/Geometry/LayerBlueprintNode.hpp (2)
67-68: Pass surfaces by const reference, you shouldTo avoid unnecessary copying and enhance efficiency, pass the surfaces parameter as a const reference, we must.
100-101: Return LayerType by value, you shouldAs LayerType is an enum of small size, returning by value more appropriate it is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (37)
Core/include/Acts/Geometry/Blueprint.hpp(1 hunks)Core/include/Acts/Geometry/BlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/BlueprintOptions.hpp(1 hunks)Core/include/Acts/Geometry/CompositePortalLink.hpp(2 hunks)Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/GridPortalLink.hpp(8 hunks)Core/include/Acts/Geometry/LayerBlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/Portal.hpp(4 hunks)Core/include/Acts/Geometry/StaticBlueprintNode.hpp(1 hunks)Core/include/Acts/Material/ProtoVolumeMaterial.hpp(1 hunks)Core/include/Acts/Seeding/PathSeeder.hpp(0 hunks)Core/include/Acts/Utilities/Delegate.hpp(7 hunks)Core/include/Acts/Utilities/GridAccessHelpers.hpp(1 hunks)Core/src/Geometry/Blueprint.cpp(1 hunks)Core/src/Geometry/BlueprintNode.cpp(1 hunks)Core/src/Geometry/BlueprintOptions.cpp(1 hunks)Core/src/Geometry/CMakeLists.txt(1 hunks)Core/src/Geometry/CylinderContainerBlueprintNode.cpp(1 hunks)Core/src/Geometry/LayerBlueprintNode.cpp(1 hunks)Core/src/Geometry/MaterialDesignatorBlueprintNode.cpp(1 hunks)Core/src/Geometry/PortalLinkBase.cpp(4 hunks)Core/src/Geometry/StaticBlueprintNode.cpp(1 hunks)Examples/Python/CMakeLists.txt(1 hunks)Examples/Python/src/Blueprint.cpp(1 hunks)Examples/Python/src/Geometry.cpp(4 hunks)Examples/Python/tests/test_blueprint.py(1 hunks)Examples/Scripts/Python/blueprint.py(1 hunks)Plugins/Json/src/GridJsonConverter.cpp(1 hunks)Tests/UnitTests/Core/Detector/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp(1 hunks)Tests/UnitTests/Core/Geometry/BlueprintTests.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CMakeLists.txt(1 hunks)docs/core/geometry/concepts.md(1 hunks)docs/core/geometry/construction.md(1 hunks)docs/core/geometry/index.md(2 hunks)docs/core/geometry/layerless/layerless.md(1 hunks)
💤 Files with no reviewable changes (1)
- Core/include/Acts/Seeding/PathSeeder.hpp
✅ Files skipped from review due to trivial changes (2)
- Core/include/Acts/Material/ProtoVolumeMaterial.hpp
- Core/include/Acts/Geometry/CompositePortalLink.hpp
🧰 Additional context used
🪛 Ruff (0.8.0)
Examples/Scripts/Python/blueprint.py
61-64: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
Examples/Python/tests/test_blueprint.py
1-1: pytest imported but unused
Remove unused import: pytest
(F401)
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp
[error] 246-246: syntax error
(syntaxError)
Tests/UnitTests/Core/Geometry/BlueprintTests.cpp
[error] 75-75: syntax error
(syntaxError)
Examples/Python/src/Geometry.cpp
[error] 702-702: Boolean value assigned to pointer.
(assignBoolToPointer)
[error] 703-703: Boolean value assigned to pointer.
(assignBoolToPointer)
🪛 LanguageTool
docs/core/geometry/concepts.md
[grammar] ~3-~3: It appears that a hyphen is missing in the noun “to-do” (= task) or did you mean the verb “to do”?
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geom...
(TO_DO_HYPHEN)
[style] ~3-~3: Consider using “incomplete” to avoid wordiness.
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geometry ## Volum...
(NOT_ABLE_PREMIUM)
docs/core/geometry/construction.md
[grammar] ~3-~3: It appears that a hyphen is missing in the noun “to-do” (= task) or did you mean the verb “to do”?
Context: # Construction :::{todo} Not complete yet ::: ## Blueprint tra...
(TO_DO_HYPHEN)
[style] ~3-~3: Consider using “incomplete” to avoid wordiness.
Context: # Construction :::{todo} Not complete yet ::: ## Blueprint tracking geometry...
(NOT_ABLE_PREMIUM)
🔇 Additional comments (40)
Tests/UnitTests/Core/Geometry/CMakeLists.txt (1)
38-39: Hmmmm, good these additions are!
Properly placed, the new tests are. In the Force of unit testing, strong these shall make us.
Verify the dependencies, we must. Run this script, I shall:
✅ Verification successful
Hmmmm, clear the path forward is!
Found the test files are, and their dependencies revealed they have. No imports from blocked PRs exist, only standard Acts framework components and testing utilities used they do. Ready for the Force of testing, these files are!
BlueprintTests.cppandBlueprintApiTests.cpp, present in the correct location they are- Standard Acts geometry components they use, such as
Acts/Geometry/Blueprint.hppandActs/Geometry/BlueprintNode.hpp - Boost test framework and common helpers, properly included they are
- No external dependencies on blocked PRs feat(util): Add
overloadedhelper #3816 and feat(geo): Add Gen3 hierarchy search tolowestTrackingVolume#3818, observed there are
Safe to proceed, we are. May the Force be with these tests!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the test files exist and their dependencies
# Search for the test files
echo "Search for the test files, we shall..."
fd "Blueprint.*\.cpp$" Tests/UnitTests/Core/Geometry/
# Check for includes in the test files that might come from blocked PRs
echo "Look for dependencies from blocked PRs #3816 and #3818, we must..."
rg -l "#include" Tests/UnitTests/Core/Geometry/Blueprint*.cpp | xargs rg "include"
Length of output: 5210
Core/src/Geometry/CMakeLists.txt (2)
46-52: Hmmmm, well organized these new files are!
Grouped together logically, the Blueprint-related source files are. A clear hierarchy they follow, from base classes to specialized implementations, yes.
46-52: Verify the completeness of Blueprint implementation, we must!
Check for missing implementation files or test coverage, we should.
✅ Verification successful
Complete and balanced, the Blueprint implementation is!
Hmmmm, observed carefully I have:
- For each header file in Core/include/Acts/Geometry, a corresponding implementation file exists
- Well-tested the code is, with comprehensive test coverage in Tests/UnitTests/Core
- Proper virtual method declarations and implementations, present they are
- Both Detector and Geometry Blueprint implementations, covered they are
The Force is strong with this codebase, yes!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for corresponding header files and test files
# Search for Blueprint-related header files
echo "Looking for corresponding headers, we are..."
fd --type f --extension hpp --extension h Blueprint Core/include
# Search for Blueprint-related test files
echo "Searching for test files, hmmmm..."
fd --type f Blueprint Tests
Length of output: 1129
Script:
#!/bin/bash
# Check implementation completeness by comparing headers and source files
echo "Comparing implementation files with headers, we must..."
# Get base names of implementation files from CMakeLists.txt
echo "Implementation files in CMakeLists.txt:"
grep "Blueprint.*\.cpp" Core/src/Geometry/CMakeLists.txt | sort
echo -e "\nHeader files base names:"
fd --type f --extension hpp --extension h Blueprint Core/include | sed 's/.*\///' | sort
# Check implementation details of Blueprint classes
echo -e "\nImplementation details, searching for we are..."
ast-grep --pattern 'class $_ : public Blueprint {
$$$
}'
# Look for virtual method implementations
echo -e "\nVirtual method implementations, finding we must..."
rg "virtual.*Blueprint" Core/include/Acts
Length of output: 1613
Examples/Python/CMakeLists.txt (2)
19-19: Hmmmm, proper integration this is. Approve it, I do!
Added correctly to the Python bindings module, the new Blueprint.cpp file is. Follows the established pattern of source file organization, it does.
Verify the integration with other components, we must:
✅ Verification successful
The force guides me to examine the implementation and its integration with the Geometry module, before making final judgment, hmmmm.
Correct and well-integrated, this Blueprint addition is!
Examined the implementation thoroughly, I have. The force reveals:
- Proper integration with Python bindings, it maintains
- All necessary headers, included they are
- Correctly integrated with Geometry module through
addBlueprintfunction, it is - Complete Python class hierarchy for Blueprint nodes, established it has
- Navigation and construction capabilities, properly exposed they are
The path of the code, clear and balanced it is. No disturbance in the force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related Blueprint files and their dependencies
# Search for related Blueprint implementation files
echo "Related Blueprint files, find we shall:"
fd "Blueprint.*\.(hpp|cpp)$"
# Check if Blueprint.cpp includes necessary headers
echo "Dependencies of Blueprint.cpp, inspect we must:"
rg -l "Blueprint" "Core/include" "Examples/Python/src"
Length of output: 2517
Script:
#!/bin/bash
# Inspect the Python bindings implementation, we must
echo "Content of the Python Blueprint bindings, reveal we shall:"
cat "Examples/Python/src/Blueprint.cpp"
# Check integration with Geometry module
echo "Integration with Geometry module, examine we must:"
rg -A 5 "Blueprint" "Examples/Python/src/Geometry.cpp"
Length of output: 17065
19-19: Consider the build configuration dependencies, you must!
In WIP state this PR is, and by other PRs it is blocked. Ensure that Blueprint functionality, dependent on merged changes from blocking PRs #3816 and #3818, it is not.
Run these commands, we shall:
If dependencies on blocking PRs exist, conditional compilation guards consider you should. Prevent build failures when PRs merge in different order, this will.
✅ Verification successful
Safe to proceed with Blueprint changes, it is!
Through the Force, seen I have that Blueprint functionality self-contained it is, within Examples/Python directory. No build configuration dependencies on PRs #3816 and #3818 exist, hmmmm. The Blueprint implementation, independent it stands:
- In Examples/Python/src/Blueprint.cpp, contained the core functionality is
- In Examples/Python/tests, isolated the tests remain
- In Examples/Scripts/Python, example usage exists, but standalone it is
Proceed with confidence, you may. The path forward, clear it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential build configuration dependencies
# Look for related configuration checks
echo "Build configuration dependencies, search for we must:"
rg -l "ACTS_BUILD_PLUGIN_BLUEPRINT|ACTS_BUILD_GEN3" "CMakeLists.txt"
# Search for related target dependencies
echo "Target dependencies, examine we shall:"
rg "Blueprint" "cmake"
Length of output: 293
Script:
#!/bin/bash
# Hmmmm, broader search we need, yes
# Search for Blueprint in all CMake files, recursive we must be
echo "In all CMake files, searching for Blueprint references we are:"
fd -e cmake -e txt -g "CMakeLists.txt" --exec rg -l "Blueprint" {}
# Dependencies in Examples directory, examine we shall
echo "In Examples directory, dependencies we seek:"
fd -e cmake -e txt -g "CMakeLists.txt" --exec rg -A 5 -B 5 "Blueprint" {} Examples/
# Git history of the file, inspect we must
echo "Recent changes to Examples/Python/CMakeLists.txt, reveal we shall:"
git show HEAD -- Examples/Python/CMakeLists.txt
# Related source files, find we must
echo "Blueprint source files, locate we shall:"
fd Blueprint
Length of output: 74252
Core/include/Acts/Utilities/Delegate.hpp (2)
83-90: Constructor hidden from documentation, intentional this is?
Using /// @cond and /// @endcond hides this constructor from generated documentation. Confirm if intentional, you must.
Line range hint 161-182: Function hidden from documentation, intentional is this?
The use of /// @cond and /// @endcond hides this function from documentation. Verify if this is intended, you should.
Core/src/Geometry/LayerBlueprintNode.cpp (1)
55-57: Confirm 'using enum', compatible it is
'using enum' a C++20 feature is. Ensure the codebase C++20 supports, you should.
Core/src/Geometry/Blueprint.cpp (6)
28-32: Logic error, there is.
The build method throws a std::logic_error, stating that the root node cannot be built. Ensure this is the intended behavior, you must. If the root node should be buildable, adjust the implementation, you should.
Confirm that throwing a std::logic_error here is appropriate, let us.
34-38: Root node cannot be connected, hmm?
Similarly, the connect method throws a std::logic_error. Verify if preventing the root node from connecting is desired, you should.
Ensure that this restriction aligns with the design, we must.
40-45: Finalize the root node, we cannot?
The finalize method also throws a std::logic_error. Confirm that this is intentional, you should. If the root node requires finalization, modify the method accordingly, you must.
Consider if the root node needs to participate in the finalization process, we should.
60-62: Informative logging, this is.
The debug and info logs provide valuable insights during construction. Good practice, it is.
69-72: Child count validation, properly done it is.
Ensuring exactly one child for the root node, you are. Appropriate for maintaining tree integrity, this is.
142-150: Duplicate volume name detection, good this is.
Checking for unique volume names to prevent conflicts, you are. Prevents runtime errors and ensures integrity, it does.
Core/src/Geometry/StaticBlueprintNode.cpp (4)
27-29: Volume validity checked, you have.
Throwing an exception when the volume is not built, you are. Essential for robustness, this is.
45-47: Volume presence ensured, it must be.
Verifying that the volume is present before connecting, you are. Wise, this is.
82-90: Checks for built volume and shell, prudent they are.
Ensuring both the volume and shell are built before finalization, you are. Prevents unexpected behavior, this does.
103-109: Navigation policy assignment, correct it is.
Assigning the navigation policy to the volume using the provided factory, you are. Adheres to the design pattern, this does.
Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp (2)
95-110: Setter methods clarity, appreciate I do.
Methods for setting stacking direction and strategies, you provide. Fluent interface style, it follows.
127-131: Gap volume identification helper, helpful it is.
The isGapVolume method aids in distinguishing gap volumes. Useful for maintainability, this will be.
Core/src/Geometry/MaterialDesignatorBlueprintNode.cpp (3)
32-38: Single child enforced, it must be.
Checking for exactly one child in build method, you are. Necessary for this node's design, it is.
40-43: Binning configuration required, it is.
Ensuring binning is set before proceeding, you are. Prevents misconfiguration, this does.
147-167: Graphviz representation, informative it is.
Providing detailed labels for visualization aids in debugging. Ensure all possible binning types are handled, you should.
Core/src/Geometry/CylinderContainerBlueprintNode.cpp (4)
44-47: Volume already built, cannot rebuild it.
Throwing an error if build is called more than once, you are. Maintains integrity of the build process, this does.
89-119: Gap volume handling, carefully managed it is.
Distinguishing gap volumes from child volumes and handling them appropriately, you are. Ensures correct assembly of the volume stack, this does.
126-137: Sanity checks on shells, performed they are.
Validating shell consistency after assembly, you are. Essential for preventing runtime errors, this is.
190-217: Immutable configuration after build, enforced it must be.
Preventing changes to direction and strategies after the build phase, you are. Protects the integrity of the built structure, this does.
Examples/Python/src/Geometry.cpp (1)
702-703: False positive from static analysis tool, ignore you can.
No issue with assigning boolean to pointer there is. Code as intended functions.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 702-702: Boolean value assigned to pointer.
(assignBoolToPointer)
[error] 703-703: Boolean value assigned to pointer.
(assignBoolToPointer)
docs/core/geometry/index.md (1)
Line range hint 2-11: Approve this introduction, I do. Clear and informative, it is.
Well-structured explanation of the ACTS geometry model and its relationship with ATLAS Tracking geometry, this section provides. Surface-based description and integration with track propagation, properly emphasized it has.
Core/src/Geometry/BlueprintOptions.cpp (1)
22-27: Elegant, the factory method implementation is.
Clean and concise, the fluent interface usage is. The Force is strong with this pattern.
Tests/UnitTests/Core/Detector/CMakeLists.txt (1)
1-1: Hmm, versioning strategy clarification needed, it is.
Renamed from Blueprint to Gen2Blueprint, the test has been. Wise decision for backward compatibility, this is. Yet clarity on versioning strategy between Gen2 and Gen3, we seek.
Run this command to understand the blueprint generations, we must:
docs/core/geometry/layerless/layerless.md (1)
7-9: Clear and informative, this documentation is, hmm!
Well documented, the evolution from Gen 1 through Gen 3 geometry models is. Help future developers understand the system's progression, this will.
Core/src/Geometry/PortalLinkBase.cpp (2)
112-112: Wise decision to elevate grid-trivial merge warnings, it is!
More attention from developers, these complex portal merging operations deserve. Help prevent potential issues during geometry construction, this will.
Also applies to: 131-131
144-144: Appropriate reduction of log levels for simpler merges, this is!
Less critical operations to VERBOSE level, you have moved. Cleaner logs during normal operation, this will create, while maintaining detailed information for debugging.
Also applies to: 159-159, 164-164
Core/include/Acts/Geometry/Portal.hpp (2)
Line range hint 113-122: Clear and helpful, the diagram is! Approve this documentation, I do.
Illustrate the portal fusion process well, this ASCII diagram does. Help developers understand the spatial relationships between portals, it will.
Line range hint 145-155: Excellent visualization of portal merging, this is!
Understand the merging process better, developers will. Clear and precise, the diagram is.
Core/include/Acts/Geometry/GridPortalLink.hpp (3)
Line range hint 114-188: Most impressive, these diagrams are! Clarity to complex merging scenarios, they bring.
Well-documented and visually clear, the 1D merge scenarios are. Help developers understand the geometric relationships, they will.
Line range hint 201-246: Masterful explanation of mixed-dimension merges, this is!
Understand the transformation from 1D to 2D grids better, developers will. Clear and comprehensive, the documentation is.
Line range hint 254-287: Strong with the documentation, this one is! Approve the 2D merge explanations, I do.
Clearly show how 2D grids combine, these diagrams do. Important note about shared axis requirement, this documentation makes.
Core/include/Acts/Geometry/LayerBlueprintNode.hpp (1)
124-124: Ensure operator<< is defined, we must
Declared the operator<< for LayerType is, but defined it in this file is not. Verify its definition elsewhere exists, you should.
github-actions
bot
added
Component - Documentation
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
docs/core/geometry/concepts.md (1)
15-56: Context between class references, add we must.Well-structured the hierarchy is, but explanatory text between classes, missing it remains. For each component:
- Brief description before class reference
- Usage examples
- Common patterns and gotchas
Example for Portal section:
## Portals + +Portals serve as navigation surfaces between volumes in the detector geometry. +They enable efficient traversal and boundary handling during particle tracking. :::{doxygenclass} Acts::Portal :::Examples/Python/tests/test_blueprint.py (3)
15-26: Document your test's purpose, you should!Missing, a docstring is. Help future Jedi understand the test's purpose, it would.
def test_zdirection_container_blueprint(tmp_path): + """ + Test the construction of Blueprint with cylinder containers. + + Validates: + - Container creation and nesting + - Volume addition and positioning + - Depth tracking + - Container direction handling + """
32-41: Constants, define we should, for magic numbers used!Clear your mind must be, if you are to discover the real meaning of these numbers. Extract constants, we must!
+ # Define geometric constants + RADIUS_INCREMENT = 50 * mm + INITIAL_RADIUS = 25 * mm + VOLUME_THICKNESS = 20 * mm + VOLUME_LENGTH = 200 * mm + r = 25 * mm for i in range(1, 3): - r += 50 * mm - bounds = acts.CylinderVolumeBounds(r, r + 20 * mm, 200 * mm) + r += RADIUS_INCREMENT + bounds = acts.CylinderVolumeBounds(r, r + VOLUME_THICKNESS, VOLUME_LENGTH)
80-80: More assertions, strengthen your test they will!Validate the final structure more thoroughly, we should. Additional assertions about container relationships and final depths, add we must!
write(root, 4) + + # Verify final structure + assert len(det.getChildren()) == 3 # barrel + 2 endcaps + assert root.depth == 0 + assert det.depth == 1 + for child in det.getChildren(): + assert child.depth == 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
Core/include/Acts/Geometry/BlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/BlueprintOptions.hpp(1 hunks)Core/src/Geometry/BlueprintNode.cpp(1 hunks)Examples/Python/src/Blueprint.cpp(1 hunks)Examples/Python/tests/test_blueprint.py(1 hunks)docs/core/geometry/concepts.md(1 hunks)docs/core/geometry/layerless/layerless.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Core/include/Acts/Geometry/BlueprintOptions.hpp
- docs/core/geometry/layerless/layerless.md
- Examples/Python/src/Blueprint.cpp
- Core/include/Acts/Geometry/BlueprintNode.hpp
🧰 Additional context used
📓 Learnings (1)
Core/src/Geometry/BlueprintNode.cpp (1)
Learnt from: paulgessinger
PR: acts-project/acts#3869
File: Core/src/Geometry/BlueprintNode.cpp:29-31
Timestamp: 2024-12-05T20:20:43.416Z
Learning: In `Core/src/Geometry/BlueprintNode.cpp`, within the function `hasDescendent`, when iterating over `ancestor.children()`, the variable `child` is of type `const BlueprintNode&`, so passing `child` directly to `hasDescendent` is correct; no dereferencing is needed.
🪛 LanguageTool
docs/core/geometry/concepts.md
[grammar] ~3-~3: It appears that a hyphen is missing in the noun “to-do” (= task) or did you mean the verb “to do”?
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geom...
(TO_DO_HYPHEN)
[style] ~3-~3: Consider using “incomplete” to avoid wordiness.
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geometry ## Volum...
(NOT_ABLE_PREMIUM)
🪛 Ruff (0.8.0)
Examples/Python/tests/test_blueprint.py
1-1: pytest imported but unused
Remove unused import: pytest
(F401)
🔇 Additional comments (8)
docs/core/geometry/concepts.md (2)
1-5: 🛠️ Refactor suggestion
Introduction and proper TODO syntax, missing they are.
Before diving into specific concepts, a high-level overview provide, we must. The path to understanding, clearer it will be.
Apply this diff, you should:
# Concepts
+
+The ACTS geometry system consists of several key concepts that work together
+to represent and navigate complex tracking detector geometries:
+
+- Volumes: The basic building blocks of detector geometry
+- Portals: Surfaces connecting different volumes
+- Navigation: Systems for traversing the geometry
-:::{todo}
-Not complete yet
-:::
+.. todo::
+
+ Complete the following sections with detailed explanations and examples.🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: It appears that a hyphen is missing in the noun “to-do” (= task) or did you mean the verb “to do”?
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geom...
(TO_DO_HYPHEN)
[style] ~3-~3: Consider using “incomplete” to avoid wordiness.
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geometry ## Volum...
(NOT_ABLE_PREMIUM)
57-72:
Documentation for navigation policy, crucial it is. Missing implementation details, I sense.
Found extensive navigation code in tests, I have, but sparse documentation in concepts.md, it remains. Navigation policy, a fundamental concept for geometry system it is, yet undocumented it stays.
Add these essential concepts, you must:
- Portal navigation mechanisms
- Volume navigation strategies
- Navigation policy configuration
- Navigation state management
The Force guides us to maintain proper documentation, hmm? Yes.
Core/src/Geometry/BlueprintNode.cpp (4)
23-36: Well-implemented the 'hasDescendent' function is.
Prevent cycles it does, by recursively checking child nodes. Robust and correct, this logic is.
43-59: Exception handling in 'addChild', commendable it is.
Null pointers you check for, and previous parent associations you validate. Cyclic dependencies you avoid, ensuring integrity of the node tree.
73-78: Proper depth propagation in 'setDepth', you have.
When depth you set, to child nodes it cascades. Consistency of hierarchy this maintains.
149-154: Children clearing, correctly handled it is.
Depth of children reset you do, and the list you clear. Memory management and state consistency ensured they are.
Examples/Python/tests/test_blueprint.py (2)
1-13: Hmmmm. False positive, this static analysis warning is!
The pytest import, required it is, for the tmp_path fixture to function. Remove it, we must not! Constants and context objects, well-defined they are.
🧰 Tools
🪛 Ruff (0.8.0)
1-1: pytest imported but unused
Remove unused import: pytest
(F401)
17-22: Handle file operations with care, we must!
The Dark Side of file operations, dangerous it can be. Error handling, add we must.
def write(root: acts.BlueprintNode, stage: int):
gz = tmp_path / f"blueprint_{stage}.dot"
print(gz)
- with gz.open("w") as fh:
- root.graphViz(fh)
+ try:
+ with gz.open("w") as fh:
+ root.graphViz(fh)
+ except IOError as e:
+ print(f"Failed to write blueprint stage {stage}: {e}")
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
docs/known-warnings.txt (1)
9-9: Document the specialization cases, we must.Cryptic, this comment about specialization is. Future Padawans, confused they will be. Expand the comment to explain which specialization cases trigger these warnings, you should.
Apply this change, you must:
-# I think these are because we use specialization +# Warnings from template specializations in the BDT (Binary Decision Tree) implementation +# These are expected due to how Sphinx processes specialized template classes🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: A period might be missing here.
Context: ...s_.* # I think these are because we use specialization .*undefined label: .bdt.(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
docs/core/geometry/concepts.md (2)
15-56: Portal concepts, explain better we must.Between class documentation, explanatory text add we should. Portal system, complex it is. Understanding, easier make we must.
## Portals + +Portals, boundaries between volumes they are. Through them, particles travel: +- Connection points they provide +- Navigation decisions they guide +- Performance optimization they enable :::{doxygenclass} Acts::Portal ::: ### Portal links + +Links, portal connections they manage: +- Different strategies they implement +- Performance characteristics they define #### Trivial portal link + +Simplest form of portal link this is: +- Direct connections it manages +- When to use, explain we must
74-75: ProtoLayer documentation, context it needs.Mysterious, this ProtoLayer struct appears. Its purpose and relationships, explain we must.
+## Layer Construction + +Layer building process, understand we must: +- ProtoLayer: Template for layer creation it is +- Configuration: Options available they are +- Integration: With volumes how works it does + :::{doxygenstruct} Acts::ProtoLayer :::
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docs/core/geometry/concepts.md(1 hunks)docs/core/geometry/layerless/layerless.md(1 hunks)docs/known-warnings.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/core/geometry/layerless/layerless.md
🧰 Additional context used
🪛 LanguageTool
docs/core/geometry/concepts.md
[grammar] ~3-~3: It appears that a hyphen is missing in the noun “to-do” (= task) or did you mean the verb “to do”?
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geom...
(TO_DO_HYPHEN)
[style] ~3-~3: Consider using “incomplete” to avoid wordiness.
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geometry ## Volum...
(NOT_ABLE_PREMIUM)
🔇 Additional comments (4)
docs/known-warnings.txt (1)
8-8: Broad, this pattern is. More specific, it should be.
Hmmmm. Suppress many warnings, this pattern does. A path to the dark side, it could be. Consider more specific patterns for known cases, you should.
Run this script to see what warnings we might hide, we must:
docs/core/geometry/concepts.md (3)
57-72: Navigation policy documentation, crucial yet incomplete it remains.
Navigation system, heart of geometry traversal it is. Documentation, expanded it must be.
### Navigation policy
+
+Navigation policies, path through geometry they guide:
+
+#### Core Concepts
+- Decision making: How choices made they are
+- Performance impact: Speed vs accuracy balance
+- Configuration options: Flexibility they provide
+
+#### Implementation Patterns
+- Policy selection: When to use each type
+- Composition: How policies combine they do
+- Extension: Custom policies create how
:::{doxygenclass} Acts::INavigationPolicy
:::1-5: 🛠️ Refactor suggestion
Introduction missing and TODO syntax incorrect, it is.
Hmm, previous feedback acknowledged it is, yet introduction still missing remains. Additionally, TODO syntax, proper Sphinx format it should follow.
Apply this change, you must:
# Concepts
+
+The ACTS geometry system, fundamental building blocks it has:
+- Volumes: Basic containers of detector elements they are
+- Portals: Connections between volumes they provide
+- Navigation: Path through geometry it guides
-:::{todo}
-Not complete yet
-:::
+.. todo::
+
+ Following sections, complete they must be:
+ - Volume bounds: Detailed explanation needed
+ - Tracking volume: Examples required
+ - Navigation policy: Implementation details missing🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: It appears that a hyphen is missing in the noun “to-do” (= task) or did you mean the verb “to do”?
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geom...
(TO_DO_HYPHEN)
[style] ~3-~3: Consider using “incomplete” to avoid wordiness.
Context: # Concepts :::{todo} Not complete yet ::: ## Tracking geometry ## Volum...
(NOT_ABLE_PREMIUM)
7-14: 🛠️ Refactor suggestion
Empty sections, fill with wisdom we must.
Each section, structure and content it requires. Template for sections, provide I shall:
## Tracking geometry
+
+The tracking geometry, hierarchical structure it follows:
+- Purpose: Track reconstruction it serves
+- Components: Volumes and surfaces it contains
+- Examples: Common configurations show we must
## Volume
+
+Volumes, building blocks of geometry they are:
+- Types: Different volume shapes explain
+- Properties: Geometric attributes describe
+- Usage: Common patterns demonstrate
### Volume bounds
+
+Bounds, volume shapes they define:
+- Available bounds: List and describe
+- Parameters: Configuration options explain
+- Visualization: Diagrams add we should
## Tracking volume
+
+Special volume type, tracking volume is:
+- Purpose: Track reconstruction role explain
+- Features: Special capabilities highlight
+- Integration: With other components showLikely invalid or redundant comment.
This allows using it with range algorithms
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
Core/include/Acts/Geometry/BlueprintNode.hpp (1)
256-263: Naming consistency between 'graphviz' methods, consider you should.Consistency improves readability, it does. Recommend using either 'graphviz' or 'addToGraphviz' for both methods.
Examples/Scripts/Python/blueprint.py (1)
61-64: Cleaner code structure, achieve we can!Combine the nested with statements, we should. More readable, it will be.
- with pixel.Material() as mat: - with mat.CylinderContainer( - direction=acts.BinningValue.binZ, name="PixelNegEndcap" - ) as ec: + with pixel.Material() as mat, mat.CylinderContainer( + direction=acts.BinningValue.binZ, name="PixelNegEndcap" + ) as ec:🧰 Tools
🪛 Ruff (0.8.0)
61-64: Use a single
withstatement with multiple contexts instead of nestedwithstatementsCombine
withstatements(SIM117)
Core/include/Acts/Utilities/TransformRange.hpp (1)
233-235: Strong with modern C++, this code is!Enable borrowed range capability, you have. With C++20 ranges library, work seamlessly it will. A path to greater flexibility and interoperability, this is.
Tests/UnitTests/Core/Utilities/TransformRangeTests.cpp (2)
105-111: Approve these changes, I do. Yet a suggestion, I have.Good test coverage for transform functionality, this provides. Clear and thorough, the implementation is.
Consider naming the test more descriptively, we should:
- std::vector<int> unpacked; + std::vector<int> transformedValues;
149-153: More explicit, the expected values should be.Good test for const correctness, this is. Yet clearer it could be.
Consider making the expected values more explicit:
- BOOST_CHECK(unpacked == std::vector<int>({1, 2, 3})); + const std::vector<int> expectedValues = {1, 2, 3}; + BOOST_CHECK_EQUAL_COLLECTIONS(unpacked.begin(), unpacked.end(), + expectedValues.begin(), expectedValues.end());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
Core/include/Acts/Geometry/Blueprint.hpp(1 hunks)Core/include/Acts/Geometry/BlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/BlueprintOptions.hpp(1 hunks)Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/LayerBlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/StaticBlueprintNode.hpp(1 hunks)Core/include/Acts/Utilities/TransformRange.hpp(3 hunks)Core/src/Geometry/Blueprint.cpp(1 hunks)Core/src/Geometry/BlueprintNode.cpp(1 hunks)Core/src/Geometry/BlueprintOptions.cpp(1 hunks)Core/src/Geometry/CylinderContainerBlueprintNode.cpp(1 hunks)Core/src/Geometry/LayerBlueprintNode.cpp(1 hunks)Core/src/Geometry/MaterialDesignatorBlueprintNode.cpp(1 hunks)Examples/Python/src/Blueprint.cpp(1 hunks)Examples/Python/tests/test_blueprint.py(1 hunks)Examples/Scripts/Python/blueprint.py(1 hunks)Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp(1 hunks)Tests/UnitTests/Core/Geometry/BlueprintTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/TransformRangeTests.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- Core/include/Acts/Geometry/BlueprintOptions.hpp
- Core/src/Geometry/BlueprintOptions.cpp
- Examples/Python/src/Blueprint.cpp
- Core/include/Acts/Geometry/StaticBlueprintNode.hpp
- Core/src/Geometry/Blueprint.cpp
- Core/include/Acts/Geometry/Blueprint.hpp
- Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp
- Core/include/Acts/Geometry/LayerBlueprintNode.hpp
- Core/src/Geometry/LayerBlueprintNode.cpp
- Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp
🧰 Additional context used
📓 Learnings (2)
Core/src/Geometry/BlueprintNode.cpp (1)
Learnt from: paulgessinger
PR: acts-project/acts#3869
File: Core/src/Geometry/BlueprintNode.cpp:29-31
Timestamp: 2024-12-05T20:20:43.416Z
Learning: In `Core/src/Geometry/BlueprintNode.cpp`, within the function `hasDescendent`, when iterating over `ancestor.children()`, the variable `child` is of type `const BlueprintNode&`, so passing `child` directly to `hasDescendent` is correct; no dereferencing is needed.
Core/src/Geometry/CylinderContainerBlueprintNode.cpp (1)
Learnt from: paulgessinger
PR: acts-project/acts#3869
File: Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp:28-29
Timestamp: 2024-12-05T20:21:24.253Z
Learning: In `CylinderContainerBlueprintNode`, validation that child volumes are cylinders is enforced by `CylinderVolumeStack`.
🪛 Ruff (0.8.0)
Examples/Python/tests/test_blueprint.py
1-1: pytest imported but unused
Remove unused import: pytest
(F401)
Examples/Scripts/Python/blueprint.py
61-64: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp
[error] 246-246: syntax error
(syntaxError)
Tests/UnitTests/Core/Geometry/BlueprintTests.cpp
[error] 75-75: syntax error
(syntaxError)
🔇 Additional comments (20)
Core/src/Geometry/BlueprintNode.cpp (4)
25-35: Function 'hasDescendent' well-implemented, it is.
Properly checks for cycles and recursion, the code does. Efficient and correct, it appears.
42-62: Robust checks in 'addChild', you have.
Null pointers, existing parents, and cycles, handled they are. Solid error handling, this shows.
76-81: Depth management correctly recursive, it is.
'setDepth' updates children's depth appropriately, ensuring tree integrity it does.
152-157: Children clearing appropriately done, it is.
Depth reset and children cleared, they are. Consistent state maintained, the node has.
Core/src/Geometry/MaterialDesignatorBlueprintNode.cpp (3)
29-45: Proper validation in 'build' method, I see.
Ensures exactly one child and binning set, the code does. Throws exceptions when necessary, it does.
96-138: 'connect' method handles cylinder binning well, it does.
Correctly applies material to portals, the function does. Error handling for unsupported shells, present it is.
151-178: Graphviz representation informative, it is.
Node details and binning information included, they are. Helpful for visualization, this will be.
Core/src/Geometry/CylinderContainerBlueprintNode.cpp (3)
174-184: Redundancy in policy creation, addressed it has been.
One call to 'policyFactory->build', there is. Efficient and clean, the code now is.
70-114: Child shells collected accurately, they are.
Gap volumes and child volumes handled properly, the function does. Exception safety considered, it is.
228-241: Graphviz output consistent and clear, it is.
Node information and child connections displayed well, aiding in debugging it will.
Core/include/Acts/Geometry/BlueprintNode.hpp (2)
269-271: Stream operator implemented correctly, it is.
Overloads '<<' for 'BlueprintNode', useful this will be. Enhances debugging capabilities, it does.
274-277: 'toStream' method appropriately defined, it is.
Allows subclasses to customize output, the virtual method does. Good design practice, this is.
Examples/Python/tests/test_blueprint.py (3)
17-21: Safe file operations, implement we must!
Handle file operations with care, we should. The Dark Side of file operations, dangerous it can be.
def write(root: acts.BlueprintNode, stage: int):
gz = tmp_path / f"blueprint_{stage}.dot"
print(gz)
- with gz.open("w") as fh:
- root.graphviz(fh)
+ try:
+ with gz.open("w") as fh:
+ root.graphviz(fh)
+ except IOError as e:
+ print(f"Failed to write blueprint stage {stage}: {e}")
+ raise23-31: Strong with the Force, this code is!
Proper initialization and verification, I sense. Well-structured assertions, you have used.
49-79: 🛠️ Refactor suggestion
Duplicate code, I sense in you. A helper function, create we must!
Similar patterns in nEC and pEC creation, I see. Extract common logic into a helper function, we should!
+def create_endcap(det, name: str, z_direction: int):
+ """Create endcap volumes with specified direction.
+
+ Args:
+ det: Parent container
+ name: Name prefix for volumes
+ z_direction: Direction multiplier (1 for positive, -1 for negative)
+ """
+ with det.CylinderContainer(f"{name}EC", direction=bv.binZ) as ec:
+ assert ec.depth == 2
+ z = 200 * z_direction
+ for i in range(1, 3):
+ z += 200 * mm * z_direction
+ bounds = acts.CylinderVolumeBounds(100 * mm, 150 * mm, 50 * mm)
+ trf = base * acts.Translation3(acts.Vector3(0, 0, z))
+ vol = ec.addStaticVolume(trf, bounds, name=f"{name}EC_{i}")
+ assert vol.depth == 3
+ return ec
-with det.CylinderContainer("nEC"...:
+create_endcap(det, "n", -1)
-with det.CylinderContainer("pEC"...:
+create_endcap(det, "p", 1)Examples/Scripts/Python/blueprint.py (1)
86-99: File operations, safer they must be. Remove commented code, we should.
Error handling for file operations, add we must. Commented print statement, remove we shall.
-with open("blueprint.dot", "w") as fh:
- root.graphviz(fh)
+try:
+ with open("blueprint.dot", "w") as fh:
+ root.graphviz(fh)
+except IOError as e:
+ print(f"Failed to write blueprint: {e}")
-with Path("blueprint.obj").open("w") as fh:
- vis.write(fh)
-# print("DONE")
+try:
+ with Path("blueprint.obj").open("w") as fh:
+ vis.write(fh)
+except IOError as e:
+ print(f"Failed to write visualization: {e}")Core/include/Acts/Utilities/TransformRange.hpp (1)
172-173: Complete, the iterator interface now is!
Wise additions to the TransformRangeIterator, these are. Default constructor and post-increment operator, essential they are for standard iterator compliance.
Also applies to: 189-195
Tests/UnitTests/Core/Utilities/TransformRangeTests.cpp (1)
123-128: Well structured, this test is.
Proper verification of const dereferenced ranges, it provides. Clear and consistent with previous test case, the implementation is.
Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp (1)
109-244: 🛠️ Refactor suggestion
Refactor this large function, we must.
Too complex for a single function, this is. Break it down into smaller, focused functions, we should.
Consider extracting these responsibilities:
- Navigation initialization
- Candidate processing
- Position updating
- Intersection handling
Also, remove commented code at line 201, we must:
- // position += delta / (substeps + 1);And define constants for magic numbers:
+const size_t MAX_ITERATIONS = 100;
+const double CM_TO_MM = 10_mm;
- for (std::size_t i = 0; i < 100; i++) {
+ for (std::size_t i = 0; i < MAX_ITERATIONS; i++) {Likely invalid or redundant comment.
Tests/UnitTests/Core/Geometry/BlueprintTests.cpp (1)
75-525: Well structured and comprehensive, these tests are.
Good coverage of error cases and proper separation of concerns, I see. Clear naming and purpose, each test has.
Particularly strong in testing:
- Invalid configurations
- Child node management
- Depth calculations
- Material assignment
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 75-75: syntax error
(syntaxError)
|
acts-project-service
added
the
Breaks Athena build
This PR implements building a tracking geometry using
the blueprint building mechanism.
The construction phases are documented in @c BlueprintNode, which is the
base class for all nodes in the tree.
Part of:
Blocked by:
overloadedhelper #3816lowestTrackingVolume#3818--- END COMMIT MESSAGE ---
Summary by CodeRabbit
New Features
Blueprintclass for hierarchical tracking geometry construction.BlueprintNodeand various specialized node classes for managing geometric structures.BlueprintOptionsfor configuration and validation of navigation policies.CompositePortalLinkandGridPortalLinkclasses.Bug Fixes
Blueprintand related classes.Documentation
Tests
Blueprint,BlueprintApi, and geometry functionalities.