-
Notifications
You must be signed in to change notification settings - Fork 14
feat: extract maneuvers for custom dynamics #565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe code introduces an overloaded method for extracting maneuvers from a trajectory segment solution, allowing users to specify thruster dynamics explicitly. Type alias naming is standardized, and both C++ and Python bindings are updated to support the new overload. Corresponding tests are extended to verify the new method signature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SegmentSolution
participant Dynamics
User->>SegmentSolution: extractManeuvers(frame)
SegmentSolution->>SegmentSolution: Find thruster dynamics internally
SegmentSolution->>SegmentSolution: extractManeuvers(frame, dynamics)
SegmentSolution->>Dynamics: Use dynamics to extract maneuvers
SegmentSolution-->>User: Return maneuvers
User->>SegmentSolution: extractManeuvers(frame, dynamics)
SegmentSolution->>Dynamics: Use provided dynamics to extract maneuvers
SegmentSolution-->>User: Return maneuvers
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp
(1 hunks)bindings/python/test/trajectory/test_segment.py
(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp
(2 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp
(4 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
extractManeuvers
(127-226)extractManeuvers
(127-129)extractManeuvers
(228-258)extractManeuvers
(228-228)
bindings/python/test/trajectory/test_segment.py (4)
bindings/python/test/trajectory/test_sequence.py (3)
thruster_dynamics
(208-214)segment_solution
(312-324)dynamics
(166-167)include/OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp (2)
Thruster
(40-44)Thruster
(46-46)bindings/python/test/trajectory/test_state.py (1)
frame
(32-33)bindings/python/test/dynamics/test_thruster.py (1)
dynamics
(68-76)
🔇 Additional comments (8)
bindings/python/test/trajectory/test_segment.py (1)
189-197
: LGTM! Test coverage for the new overload is appropriate.The test correctly verifies both the original
extract_maneuvers
method and the new overload that accepts explicit dynamics. The approach of checking for non-None results is appropriate for this level of integration testing.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
206-238
: Excellent implementation of the Python bindings for the new overload.The binding correctly exposes both versions of
extract_maneuvers
usingoverload_cast
, with comprehensive documentation for each overload. The parameter types are consistent with the C++ interface, and the argument names provide clear guidance for Python users.include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
49-49
: Good naming consistency improvement.Renaming the type alias from
flightManeuver
toFlightManeuver
follows proper naming conventions and improves code consistency.
117-130
: Excellent API design with the new overload.The addition of the overloaded
extractManeuvers
method that accepts explicit dynamics provides more flexibility while maintaining backward compatibility. The design allows users to specify custom dynamics when needed while still supporting the original convenience method that automatically finds thruster dynamics.src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (4)
127-129
: Excellent method signature enhancement for explicit dynamics control.The addition of the
aDynamicsSPtr
parameter allows users to explicitly specify the thruster dynamics for maneuver extraction, providing better control and avoiding the overhead of internal dynamics searching when the caller already knows which dynamics to use.
142-142
: Proper usage of the provided dynamics parameter.The method now correctly uses the explicitly passed dynamics parameter instead of performing internal search, which is consistent with the new method signature and improves performance.
196-196
: Good type alias standardization.The change from
flightManeuver
toFlightManeuver
follows proper naming conventions with consistent PascalCase, improving code readability and consistency across the codebase.Also applies to: 222-222
228-258
:✅ Verification successful
Well-designed backward compatibility overload with proper error handling.
This overload maintains backward compatibility while cleanly delegating to the new implementation. The thruster dynamics search logic is appropriate, and the error handling for missing thruster dynamics is clear and informative.
Consider adding a verification to ensure the
Thruster
type is properly included.
🏁 Script executed:
#!/bin/bash # Verify that Thruster type is properly included/forward declared in this file or its headers rg -A 3 -B 3 "class Thruster|#include.*Thruster|using.*Thruster"Length of output: 29869
Thruster header inclusion confirmed
The
Segment.hpp
file already includes<OpenSpaceToolkit/Astrodynamics/Dynamics/Thruster.hpp>
and bringsostk::astrodynamics::dynamics::Thruster
into scope, so no additional include or forward declaration is needed. Approving the changes.
// Check that a maneuver can be extracted when a thruster dynamics is provided as argument | ||
{ | ||
const Array<Maneuver> maneuversWithThruster = | ||
maneuveringSegmentSolution.extractManeuvers(defaultFrameSPtr_, defaultThrusterDynamicsSPtr_); | ||
|
||
EXPECT_EQ(1, maneuvers.getSize()); | ||
EXPECT_EQ(maneuveringSegmentSolution.states.getSize(), maneuvers[0].getStates().getSize()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the test assertions to use the correct variable.
The test extracts maneuvers using the new overload into maneuversWithThruster
but then performs assertions on the original maneuvers
variable. This makes the test ineffective and could miss potential bugs in the new functionality.
Apply this diff to fix the test:
// Check that a maneuver can be extracted when a thruster dynamics is provided as argument
{
const Array<Maneuver> maneuversWithThruster =
maneuveringSegmentSolution.extractManeuvers(defaultFrameSPtr_, defaultThrusterDynamicsSPtr_);
- EXPECT_EQ(1, maneuvers.getSize());
- EXPECT_EQ(maneuveringSegmentSolution.states.getSize(), maneuvers[0].getStates().getSize());
+ EXPECT_EQ(1, maneuversWithThruster.getSize());
+ EXPECT_EQ(maneuveringSegmentSolution.states.getSize(), maneuversWithThruster[0].getStates().getSize());
}
📝 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.
// Check that a maneuver can be extracted when a thruster dynamics is provided as argument | |
{ | |
const Array<Maneuver> maneuversWithThruster = | |
maneuveringSegmentSolution.extractManeuvers(defaultFrameSPtr_, defaultThrusterDynamicsSPtr_); | |
EXPECT_EQ(1, maneuvers.getSize()); | |
EXPECT_EQ(maneuveringSegmentSolution.states.getSize(), maneuvers[0].getStates().getSize()); | |
} | |
// Check that a maneuver can be extracted when a thruster dynamics is provided as argument | |
{ | |
const Array<Maneuver> maneuversWithThruster = | |
maneuveringSegmentSolution.extractManeuvers(defaultFrameSPtr_, defaultThrusterDynamicsSPtr_); | |
EXPECT_EQ(1, maneuversWithThruster.getSize()); | |
EXPECT_EQ(maneuveringSegmentSolution.states.getSize(), maneuversWithThruster[0].getStates().getSize()); | |
} |
🤖 Prompt for AI Agents
In test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp around lines
353 to 361, the test extracts maneuvers into the variable maneuversWithThruster
but mistakenly asserts on the old maneuvers variable. Update the EXPECT_EQ
assertions to use maneuversWithThruster instead of maneuvers to correctly
validate the new extraction functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I think its worth marking the old method as depracated and redirecting directly to the new method if you accept my suggestion
@@ -136,25 +138,8 @@ Array<flightManeuver> Segment::Solution::extractManeuvers(const Shared<const Fra | |||
return {}; | |||
} | |||
|
|||
// Loop through dynamics to find Thruster dynamics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its worth keeping this logic but generalizing it to any dynamics from the provided argument
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #565 +/- ##
==========================================
- Coverage 91.79% 91.78% -0.02%
==========================================
Files 106 106
Lines 10031 10038 +7
==========================================
+ Hits 9208 9213 +5
- Misses 823 825 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This allows us to extract maneuvers from segments where the maneuver was a tabulated dynamic. This is rare, but there can be situations where we would want to re-solve the maneuver with a fixed attitude for example.