feat: add state isNear method #539
Conversation
WalkthroughThis update introduces two new overloaded Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PythonState
participant CppState
User->>PythonState: Call is_near(other_state, tolerance_map)
PythonState->>CppState: Call isNear(other_state, tolerance_map)
CppState-->>PythonState: Return dict of (subset: bool or [bool])
PythonState-->>User: Return result
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp:513
- Consider refining the error message thrown at line 514 to clearly indicate that the provided tolerance map contains an unknown coordinate subset, which may aid debugging.
if (!coordinatesBrokerSPtr_->hasSubset(subset)) {
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
bindings/python/test/trajectory/test_state.py:626
- [nitpick] The type annotation suggests values of type bool, but the expected data for each coordinate subset is a list of booleans. Consider updating the annotation for clarity.
expected_is_near_result: dict[CoordinateSubset, bool] = {
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
bindings/python/test/trajectory/test_state.py (2)
614-630: Fix the type annotation in the expected result dictionary.The expected result dictionary's type annotation doesn't match what appears to be the actual return type. The annotation suggests
dict[CoordinateSubset, bool]but the values are lists of booleans.- expected_is_near_result: dict[CoordinateSubset, bool] = { + expected_is_near_result: dict[CoordinateSubset, list[bool]] = { CartesianPosition.default(): [True, True, True], CartesianVelocity.default(): [True, True, True], }🧰 Tools
🪛 Ruff (0.8.2)
618-618: Undefined name
CoordinateSubset(F821)
622-622: Undefined name
CoordinateSubset(F821)
626-626: Undefined name
CoordinateSubset(F821)
600-603: Consider adding an explicit import for CoordinateSubset to resolve static analysis warnings.The static analysis tool is flagging
CoordinateSubsetas undefined. While this doesn't affect functionality since you're already importing the concrete subclasses, adding an explicit import would resolve the warnings.from ostk.astrodynamics.trajectory.state.coordinate_subset import ( CartesianPosition, CartesianVelocity, + CoordinateSubset, )🧰 Tools
🪛 Ruff (0.8.2)
600-600: Undefined name
CoordinateSubset(F821)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (2)
500-528: Implementation of isNear method with scalar tolerances looks good.The implementation correctly handles the requirements for proximity checking with scalar tolerances. The method properly validates inputs, computes state differences, and returns a map of boolean results for each coordinate subset.
One minor suggestion:
Consider providing a more detailed error message on line 506 to help with troubleshooting:
- throw ostk::core::error::runtime::Wrong("Tolerance Map"); + throw ostk::core::error::runtime::Wrong( + "Tolerance Map size mismatch: expected " + + std::to_string(coordinatesBrokerSPtr_->accessSubsets().getSize()) + + ", but got " + std::to_string(aToleranceMap.size()) + );
530-569: Well-implemented element-wise tolerance checking method.The second overload of
isNearwith vector tolerances correctly implements element-wise comparison, allowing for more granular control over proximity checks.A couple of suggestions:
Similar to the first method, consider improving the error message on line 536 with size information.
For line 553, provide more context in the error message:
- throw ostk::core::error::runtime::Wrong("Tolerance Array Size"); + throw ostk::core::error::runtime::Wrong( + "Tolerance Array Size mismatch for subset '" + subsetSPtr->getName() + + "': expected " + std::to_string(deltaCoordinates.size()) + + ", but got " + std::to_string(toleranceArray.size()) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/State.cpp(5 hunks)bindings/python/test/trajectory/test_state.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/State.test.cpp(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
bindings/python/test/trajectory/test_state.py (2)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp (5)
State(57-62)State(73-78)State(90-97)State(104-104)State(109-109)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (5)
State(30-45)State(47-58)State(60-95)State(97-153)State(155-161)
🪛 Ruff (0.8.2)
bindings/python/test/trajectory/test_state.py
600-600: Undefined name CoordinateSubset
(F821)
604-604: Undefined name CoordinateSubset
(F821)
608-608: Undefined name CoordinateSubset
(F821)
618-618: Undefined name CoordinateSubset
(F821)
622-622: Undefined name CoordinateSubset
(F821)
626-626: Undefined name CoordinateSubset
(F821)
🔇 Additional comments (10)
bindings/python/test/trajectory/test_state.py (1)
596-613: Well-structured test for scalar tolerance proximity checking.The new test for the scalar tolerance version of
is_nearis well-designed, comparing a state against itself with small tolerance values. The dictionaries are properly constructed and the assertions are clear.🧰 Tools
🪛 Ruff (0.8.2)
600-600: Undefined name
CoordinateSubset(F821)
604-604: Undefined name
CoordinateSubset(F821)
608-608: Undefined name
CoordinateSubset(F821)
test/OpenSpaceToolkit/Astrodynamics/Trajectory/State.test.cpp (2)
1327-1377: Good test coverage for scalar tolerance proximity checking.This test thoroughly validates the
isNearmethod with scalar tolerances, including both successful operation and proper exception handling for various error conditions. The test effectively verifies that the method:
- Correctly identifies which coordinate subsets are within tolerance
- Throws exceptions for incomplete tolerance maps
- Throws exceptions for incorrect coordinate subsets
- Throws exceptions for negative tolerance values
This comprehensive approach ensures robust functionality and proper error handling.
1379-1468: Comprehensive test coverage for element-wise vector tolerance checking.The
isNearElementWisetest provides excellent coverage of the vector-based tolerance comparison functionality. The test effectively validates both correct operation with proper inputs and exception handling for all error cases, including:
- Properly identifying element-by-element proximity
- Detecting incomplete tolerance maps
- Rejecting incorrect coordinate subsets
- Catching wrong-sized tolerance arrays
- Validating that negative tolerances trigger exceptions
This thorough testing approach ensures the method is robust and handles edge cases appropriately.
include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp (3)
7-7: Appropriate header inclusion for Real type.Adding
#include <OpenSpaceToolkit/Core/Type/Real.hpp>is necessary for the new method signatures that use theRealtype.
30-30: Proper using declaration for Real type.Adding
using ostk::core::type::Real;ensures theRealtype is accessible within the namespace scope, which is consistent with the codebase style.
244-261: Well-designed isNear method declarations with clear documentation.The two new overloaded
isNearmethods are well-declared with proper const-qualification and clear documentation. The methods provide:
- A scalar tolerance version that returns per-subset boolean results
- A vector tolerance version that returns per-element boolean results for each subset
The method signatures are consistent with the codebase style, and the documentation clearly explains the purpose, parameters, and return values of each method.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/State.cpp (2)
280-297: Well-implemented Python binding for scalar tolerance proximity checking.The Python binding for the scalar tolerance version of
isNearis correctly implemented usingoverload_castto disambiguate between the overloaded C++ methods. The docstring clearly explains:
- The method's purpose
- The expected input parameters
- The structure of the return value
The method naming follows Python conventions by using snake_case (
is_near) for the Python binding of the camelCase C++ method (isNear).
299-316: Well-implemented Python binding for element-wise vector tolerance checking.The Python binding for the vector tolerance version of
isNearis properly implemented with clear parameter naming and a comprehensive docstring. The binding correctly:
- Uses
overload_castto select the appropriate C++ method- Preserves const-qualification
- Documents the expected numpy array input format
- Explains the list of booleans return structure
This implementation makes the functionality easily accessible from Python with clear usage documentation.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp (2)
509-509: Efficient implementation using existing subtraction.Good work leveraging the existing subtraction operator to compute the delta state. This approach is clean and reuses existing functionality.
555-558: Good validation of tolerance values.The validation correctly ensures that all tolerance values are positive using Eigen's array operations.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #539 +/- ##
==========================================
+ Coverage 91.64% 91.68% +0.03%
==========================================
Files 102 102
Lines 9783 9827 +44
==========================================
+ Hits 8966 9010 +44
Misses 817 817 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
apaletta3
force-pushed
the
feat/add-state-isnear-method
branch
from
6d6dda8 to
886563b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bindings/python/test/trajectory/test_state.py (1)
614-630: Consider enhancing test coverage for edge cases.The element-wise tolerance test is implemented correctly, but the test suite would benefit from additional tests for edge cases.
Consider adding tests for:
- Different states with coordinates just within and just outside the tolerance
- Error handling for mismatched array lengths
- Invalid tolerance values (e.g., negative values)
def test_is_near_with_different_states(self): # Create a slightly different state modified_position = Position.meters([6371000.0 + 1e-11, 0.0, 0.0], Frame.GCRF()) modified_state = State(state.get_instant(), modified_position, state.get_velocity()) # Test with tolerance smaller than difference (should be False) tolerance_map = {CartesianPosition.default(): 1e-12, CartesianVelocity.default(): 1e-10} is_near_result = state.is_near(state=modified_state, tolerance_map=tolerance_map) assert is_near_result[CartesianPosition.default()] is False # Test with tolerance larger than difference (should be True) tolerance_map = {CartesianPosition.default(): 1e-10, CartesianVelocity.default(): 1e-10} is_near_result = state.is_near(state=modified_state, tolerance_map=tolerance_map) assert is_near_result[CartesianPosition.default()] is True🧰 Tools
🪛 Ruff (0.8.2)
618-618: Undefined name
CoordinateSubset(F821)
622-622: Undefined name
CoordinateSubset(F821)
626-626: Undefined name
CoordinateSubset(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/State.cpp(5 hunks)bindings/python/test/trajectory/test_state.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/State.test.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/State.cpp
- src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp
- test/OpenSpaceToolkit/Astrodynamics/Trajectory/State.test.cpp
🧰 Additional context used
🪛 Ruff (0.8.2)
bindings/python/test/trajectory/test_state.py
600-600: Undefined name CoordinateSubset
(F821)
604-604: Undefined name CoordinateSubset
(F821)
608-608: Undefined name CoordinateSubset
(F821)
618-618: Undefined name CoordinateSubset
(F821)
622-622: Undefined name CoordinateSubset
(F821)
626-626: Undefined name CoordinateSubset
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test / Run C++ Unit Tests
- GitHub Check: Test / Check Python Format
- GitHub Check: Test / Build Python bindings
- GitHub Check: Test / Check C++ Format
apaletta3
force-pushed
the
feat/add-state-isnear-method
branch
from
5776089 to
700a4e9
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Vishwa Shah <vishwa2710@gmail.com>
apaletta3
force-pushed
the
feat/add-state-isnear-method
branch
from
700a4e9 to
866df65
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
bindings/python/test/trajectory/test_state.py (2)
596-613: Missing import forCoordinateSubsettype.The
test_is_nearmethod correctly tests the scalar tolerance version of the newis_nearfunction, but theCoordinateSubsettype is used in type hints without being imported. This was previously flagged in a static analysis.Add the missing import:
from ostk.astrodynamics.trajectory.state.coordinate_subset import ( CartesianPosition, CartesianVelocity, + CoordinateSubset, )🧰 Tools
🪛 Ruff (0.8.2)
600-600: Undefined name
CoordinateSubset(F821)
604-604: Undefined name
CoordinateSubset(F821)
608-608: Undefined name
CoordinateSubset(F821)
614-630: Missing import forCoordinateSubsetin element-wise test.The
test_is_near_element_wisemethod correctly tests the element-wise tolerance version of the newis_nearfunction, but also uses theCoordinateSubsettype without it being imported.The import fix mentioned in the earlier comment will address this issue as well.
🧰 Tools
🪛 Ruff (0.8.2)
618-618: Undefined name
CoordinateSubset(F821)
622-622: Undefined name
CoordinateSubset(F821)
626-626: Undefined name
CoordinateSubset(F821)
🧹 Nitpick comments (2)
bindings/python/test/trajectory/test_state.py (2)
596-613: Test case only verifies equality with self.The test correctly verifies that a state is near itself with tight tolerances, but it would be valuable to also test cases where states are slightly different or beyond the tolerance threshold.
Consider adding test cases with:
- States that differ by less than the tolerance (should return
True)- States that differ by more than the tolerance (should return
False)def test_is_near( self, state: State, + position: Position, + velocity: Velocity, + instant: Instant, ): tolerance_map: dict[CoordinateSubset, float] = { CartesianPosition.default(): 1e-10, CartesianVelocity.default(): 1e-10, } is_near_result: dict[CoordinateSubset, bool] = state.is_near( state=state, tolerance_map=tolerance_map, ) expected_is_near_result: dict[CoordinateSubset, bool] = { CartesianPosition.default(): True, CartesianVelocity.default(): True, } assert is_near_result == expected_is_near_result + # Test with a slightly modified state (within tolerance) + slightly_modified_position = Position.meters([position.get_coordinates()[0] + 1e-11, position.get_coordinates()[1], position.get_coordinates()[2]], position.get_frame()) + slightly_modified_state = State(instant, slightly_modified_position, velocity) + is_near_result = state.is_near( + state=slightly_modified_state, + tolerance_map=tolerance_map, + ) + assert is_near_result[CartesianPosition.default()] is True + + # Test with a significantly modified state (outside tolerance) + modified_position = Position.meters([position.get_coordinates()[0] + 1e-9, position.get_coordinates()[1], position.get_coordinates()[2]], position.get_frame()) + modified_state = State(instant, modified_position, velocity) + is_near_result = state.is_near( + state=modified_state, + tolerance_map=tolerance_map, + ) + assert is_near_result[CartesianPosition.default()] is False🧰 Tools
🪛 Ruff (0.8.2)
600-600: Undefined name
CoordinateSubset(F821)
604-604: Undefined name
CoordinateSubset(F821)
608-608: Undefined name
CoordinateSubset(F821)
614-630: Consider adding test cases for non-identical states.Similar to the scalar version, this test only verifies that a state is element-wise near itself. Adding tests for states that differ by varying amounts would provide more complete test coverage.
Consider adding test cases that verify behavior when individual elements exceed their tolerance:
def test_is_near_element_wise( self, state: State, + position: Position, + velocity: Velocity, + instant: Instant, ): tolerance_array_map: dict[CoordinateSubset, np.ndarray] = { CartesianPosition.default(): np.array([1e-10, 1e-10, 1e-10]), CartesianVelocity.default(): np.array([1e-10, 1e-10, 1e-10]), } is_near_result: dict[CoordinateSubset, list[bool]] = state.is_near( state=state, tolerance_array_map=tolerance_array_map, ) expected_is_near_result: dict[CoordinateSubset, list[bool]] = { CartesianPosition.default(): [True, True, True], CartesianVelocity.default(): [True, True, True], } assert is_near_result == expected_is_near_result + # Test with one element outside tolerance + position_coords = position.get_coordinates().copy() + position_coords[0] += 1e-9 # Exceed x-coordinate tolerance + modified_position = Position.meters(position_coords, position.get_frame()) + modified_state = State(instant, modified_position, velocity) + + is_near_result = state.is_near( + state=modified_state, + tolerance_array_map=tolerance_array_map, + ) + expected_is_near_result = { + CartesianPosition.default(): [False, True, True], + CartesianVelocity.default(): [True, True, True], + } + assert is_near_result == expected_is_near_result🧰 Tools
🪛 Ruff (0.8.2)
618-618: Undefined name
CoordinateSubset(F821)
622-622: Undefined name
CoordinateSubset(F821)
626-626: Undefined name
CoordinateSubset(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/State.cpp(5 hunks)bindings/python/test/trajectory/test_state.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/State.test.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/OpenSpaceToolkit/Astrodynamics/Trajectory/State.test.cpp
- src/OpenSpaceToolkit/Astrodynamics/Trajectory/State.cpp
- include/OpenSpaceToolkit/Astrodynamics/Trajectory/State.hpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/State.cpp
🧰 Additional context used
🪛 Ruff (0.8.2)
bindings/python/test/trajectory/test_state.py
600-600: Undefined name CoordinateSubset
(F821)
604-604: Undefined name CoordinateSubset
(F821)
608-608: Undefined name CoordinateSubset
(F821)
618-618: Undefined name CoordinateSubset
(F821)
622-622: Undefined name CoordinateSubset
(F821)
626-626: Undefined name CoordinateSubset
(F821)
|
Holding on merging as there might be some improvements to the interface we'd like to make, such as:
|
This is something we do quite often, so might as well formalize it here to shortcut it in the future
Summary by CodeRabbit
New Features
Bug Fixes
Tests