fix: TryAllNavigationPolicy config exposed to python #3822
Conversation
WalkthroughChange significant, it is. The constructor of Changes
Poem
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 (
|
github-actions
bot
added
Component - Core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (2)
Core/src/Navigation/TryAllNavigationPolicy.cpp (1)
Line range hint
25-29: Documentation improvement, suggest I do.Clear the code is, but wisdom in comments, share we must. Help future Padawans understand the empty config's purpose, it would.
Add this comment above the constructor, you should:
+// Convenience constructor that delegates to the main constructor with default config TryAllNavigationPolicy::TryAllNavigationPolicy(const GeometryContext& gctx, const TrackingVolume& volume, const Logger& logger) : TryAllNavigationPolicy(gctx, volume, logger, {}) {}Examples/Python/src/Navigation.cpp (1)
Line range hint
1-224: Architecture advice, share I must!Consider these suggestions, young Padawan:
- Documentation comments for Python bindings, add you should. Help users understand Config parameters, it will.
- Type hints in Python interface, beneficial they would be. Clear API contract, they provide.
Generate documentation and type hints for you, shall I?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
Core/include/Acts/Navigation/TryAllNavigationPolicy.hpp(1 hunks)Core/src/Navigation/TryAllNavigationPolicy.cpp(2 hunks)Examples/Python/src/Navigation.cpp(5 hunks)Examples/Python/tests/test_navigation.py(1 hunks)
🧰 Additional context used
🪛 Ruff
Examples/Python/tests/test_navigation.py
45-45: Missing return type annotation for public function test_try_all_arguments
Add return type annotation: None
(ANN201)
47-47: Trailing comma missing
Add trailing comma
(COM812)
🔇 Additional comments (8)
Core/src/Navigation/TryAllNavigationPolicy.cpp (2)
Line range hint 16-23: Hmmmm, good changes these are!
Wise, the new parameter order is. GeometryContext first, as the Force flows through all things, it should be. Strong and clear, the initialization remains.
Line range hint 16-29: Verify all usages of TryAllNavigationPolicy, we must.
Changed the constructor parameter order has. Ensure all instantiations throughout the codebase updated are, we must.
Core/include/Acts/Navigation/TryAllNavigationPolicy.hpp (1)
33-36:
Hmmmm, breaking change in constructor parameters, I sense.
Moved to the end, the Config parameter has been. A breaking change this is, affecting all existing code that creates TryAllNavigationPolicy instances. Wise decision it may be, as optional parameter at the end it becomes, matching the signature of the other constructor it does.
Examples/Python/src/Navigation.cpp (5)
40-43: Hmmmm, good addition this is!
Pure virtual function for TryAllNavigationPolicy with Config, added it has been. Consistent with existing pattern for SurfaceArrayNavigationPolicy, it is.
68-72: Implementation, well structured it is!
Follows the established pattern, this override does. Forwards configuration correctly to the templated add method, it does.
121-125: Clear and concise, this implementation is!
Similar to addSurfaceArray method, consistency maintained it has. Return reference for method chaining, wise choice it is.
190-190: Python method exposed, complete the circle does!
New addTryAll method, properly bound to Python it is. Matches C++ interface, harmony achieved it has.
172-181: Python bindings, properly scoped they are!
In local scope, Config class defined it is. ACTS_PYTHON_STRUCT macros, correctly used they are. But verify the exposed members we must!
✅ Verification successful
I need to see both the test usage and the actual implementation to verify if the exposed members (portals and sensitives) are correctly bound. This will help us determine if the Force is balanced in our Python bindings.
Hmm, in the Core directory it was not. Search the entire repository, we must! The path of the implementation file, find we shall. Then verify the Config members, we can.
Balanced, the Force in these bindings is!
Verified the exposed members have been, hmm! In the source code, two members we find - portals and sensitives, both boolean they are. In the Python test, used correctly they are, as shown here: Config(sensitives=True). Match perfectly with the C++ implementation, these bindings do!
The path to enlightenment shows:
- In
Core/include/Acts/Navigation/TryAllNavigationPolicy.hpp, Config members defined they are - In Python tests, properly accessed these members are
- ACTS_PYTHON_MEMBER macros, correctly expose both
portalsandsensitivesthey do
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TryAllNavigationPolicy::Config usage in tests
rg -l "TryAllNavigationPolicy.*Config" "Examples/Python/tests/"
Length of output: 104
Script:
#!/bin/bash
# Examine the test file to see how Config is used
cat Examples/Python/tests/test_navigation.py
Length of output: 1212
Script:
#!/bin/bash
# Check the actual TryAllNavigationPolicy class definition
rg -A 10 "class TryAllNavigationPolicy" Examples/Core/include/
Length of output: 127
Script:
#!/bin/bash
# Look for Config struct definition
ast-grep --pattern 'struct Config {
$$$
}'
Length of output: 46
Script:
#!/bin/bash
# Search for TryAllNavigationPolicy in the entire codebase
rg -A 10 "class TryAllNavigationPolicy"
Length of output: 961
Script:
#!/bin/bash
# Look for the implementation file
fd "TryAllNavigationPolicy"
Length of output: 130
|
I previously neglected to include the overload with the configuration in the python bindings. This PR does that and also includes test coverage for this behavior. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new method to the navigation policy factory for adding `TryAllNavigationPolicy` with configuration parameters. - Added Python bindings for the `TryAllNavigationPolicy::Config` class. - **Bug Fixes** - Adjusted constructor parameter order for `TryAllNavigationPolicy`, enhancing clarity and usability. - **Tests** - Added a new test function to validate the addition of `TryAllNavigationPolicy` with specific configuration options. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
I previously neglected to include the overload with the configuration in the python bindings. This PR does that and also includes test coverage for this behavior.
Summary by CodeRabbit
New Features
TryAllNavigationPolicywith configuration parameters.TryAllNavigationPolicy::Configclass.Bug Fixes
TryAllNavigationPolicy, enhancing clarity and usability.Tests
TryAllNavigationPolicywith specific configuration options.