feat: add getInterval methods to the Solution classes for Sequence/Solution classes #455
Conversation
…gment (and tests)
WalkthroughThe changes introduce a new method 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 (
|
![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 and nitpick comments (7)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (1)
65-67: Documentation could be more descriptive.Consider enhancing the documentation to better describe the method's purpose and return value:
- /// @brief Get interval - /// @return Interval + /// @brief Get the time interval spanning this solution + /// @return Time interval from start to end instant of this solutioninclude/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (1)
91-93: Documentation could be more descriptive.Consider enhancing the documentation to better describe the method's purpose and return value:
- /// @brief Get interval - /// @return Interval + /// @brief Get the time interval spanned by this solution + /// @return Time interval between the solution's start and end instantsbindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
132-142: Documentation could be enhanced while implementation looks good.The implementation is clean and well-placed. Consider enhancing the docstring to be more specific about the return type and add cross-references to related methods.
.def( "get_interval", &Segment::Solution::getInterval, R"doc( Get the time interval of the solution. + + The interval represents the time span between the segment's start + and end instants. Returns: - Interval: The interval. + Interval: The time interval spanning from start_instant to end_instant. + + See Also: + access_start_instant: Get the instant at which the segment starts. + access_end_instant: Get the instant at which the segment ends. )doc" )src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (1)
36-39: LGTM! Consider adding documentation.The implementation is correct and follows the class's established patterns. Consider adding a brief comment to document that this returns a closed interval (inclusive of both start and end instants).
Add documentation above the method:
/// Returns a closed interval [start, end] containing both the start and end instants Interval getInterval() constbindings/python/test/trajectory/test_sequence.py (2)
354-393: Enhance test coverage and documentation.While the test structure is good, consider these improvements:
- Add docstrings to describe the purpose of each test method
- Enhance assertions with actual value verification:
- Verify the actual values returned by getters
- Add assertions for state properties in
calculate_states_at- Add edge cases:
- Test with empty instants list
- Test with invalid numerical solver
Example enhancement for the getters test:
def test_getters( self, sequence_solution: Sequence.Solution, ): + """Test that all getter methods return expected values.""" assert sequence_solution.access_start_instant() is not None + assert sequence_solution.access_start_instant() == sequence_solution.segment_solutions[0].access_start_instant() assert sequence_solution.access_end_instant() is not None + assert sequence_solution.access_end_instant() == sequence_solution.segment_solutions[0].access_end_instant() assert sequence_solution.get_states() is not None + assert len(sequence_solution.get_states()) > 0 assert sequence_solution.get_initial_mass() is not None + assert sequence_solution.get_initial_mass() == sequence_solution.segment_solutions[0].get_initial_mass()
383-385: Add dedicated test for get_interval method.The
get_interval()method is used but not explicitly tested. Consider adding a dedicated test method to verify:
- The interval bounds match the solution's start and end instants
- Grid generation with different step sizes
- Error handling for invalid step sizes
Example test method:
def test_get_interval( self, sequence_solution: Sequence.Solution, ): """Test that get_interval returns correct time bounds and grid generation.""" interval = sequence_solution.get_interval() # Verify interval bounds assert interval.get_start() == sequence_solution.access_start_instant() assert interval.get_end() == sequence_solution.access_end_instant() # Test grid generation grid = interval.generate_grid(Duration.seconds(10.0)) assert len(grid) > 0 assert grid[0] == interval.get_start() assert grid[-1] == interval.get_end()src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (1)
66-74: LGTM! Consider adding documentation.The implementation is correct and follows the class's patterns for error handling and accessor usage. Consider adding documentation to describe:
- The return type and its meaning (closed interval)
- Possible exceptions that can be thrown
Add documentation above the method:
/// Returns a closed interval between the start and end instants of the segment solution. /// /// @return Closed interval containing the segment's time span /// @throw ostk::core::error::RuntimeError If no states are available
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Sequence.cpp(1 hunks)bindings/python/test/trajectory/test_segment.py(2 hunks)bindings/python/test/trajectory/test_sequence.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp(4 hunks)
🔇 Additional comments (14)
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp (2)
12-12: LGTM: Clean header inclusion and namespace usage.
The new include and using declaration for Interval are properly placed and follow the project's conventions.
Also applies to: 32-32
65-67: Verify related implementation changes.
The header changes look good, but please ensure:
- The corresponding implementation file has been updated
- Unit tests cover the new method
- Python bindings expose this functionality
✅ Verification successful
Based on the comprehensive search results, I can now provide a definitive response about the implementation status of getInterval():
All required implementation components are present and verified
The implementation is complete with:
- Header declaration in
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp - Implementation in
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp:
Interval Sequence::Solution::getInterval() const
{
return Interval::Closed(this->accessStartInstant(), this->accessEndInstant());
}- Unit tests in
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp - Python bindings in
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Sequence.cpp:
.def(
"get_interval",
&Sequence::Solution::getInterval,
R"doc(
Get the interval.
Returns:
Interval: The interval.
)doc"
)The implementation is properly integrated into the codebase with full test coverage and Python bindings exposure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify implementation, tests, and bindings exist for getInterval
# Check for implementation
echo "Checking for implementation file..."
fd -e cpp "Sequence" | xargs rg "getInterval.*const.*{" -A 5
# Check for tests
echo "Checking for test coverage..."
fd -e cpp -e hpp "test.*Sequence" | xargs rg "TEST.*getInterval"
# Check for Python bindings
echo "Checking for Python bindings..."
fd -e cpp "bindings.*Sequence" | xargs rg "\.def.*getInterval"
Length of output: 521
Script:
#!/bin/bash
# Description: Verify implementation, tests, and bindings exist for getInterval
# Check for implementation
echo "Checking for implementation file..."
fd -e cpp "Sequence" | xargs rg "getInterval\(\)\s+const"
# Check for tests
echo "Checking for test coverage..."
fd -e cpp -e hpp "test.*Sequence" | xargs rg "getInterval"
# Check for Python bindings
echo "Checking for Python bindings..."
fd -e cpp "bindings.*Sequence" | xargs rg "getInterval"
# Also check the implementation file content
echo "Checking Sequence implementation file..."
fd -e cpp "Sequence" | xargs cat
Length of output: 75784
include/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.hpp (2)
16-16: LGTM: Header inclusion and namespace usage follow existing patterns.
The new include and using declaration for Interval are properly placed within their respective Physics-related sections, maintaining the established file organization.
Also applies to: 43-43
91-93: Verify implementation uses existing accessors.
The implementation of getInterval() should utilize the existing accessStartInstant() and accessEndInstant() methods for consistency.
✅ Verification successful
Looking at the search results, I can see the implementation of getInterval() in src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp:
Interval Segment::Solution::getInterval() const
{
if (this->states.isEmpty())
{
throw ostk::core::error::RuntimeError("No solution available.");
}
return Interval::Closed(accessStartInstant(), accessEndInstant());
}The implementation correctly uses the accessStartInstant() and accessEndInstant() methods as suggested in the review comment. It also includes proper error handling for empty states.
Implementation correctly uses suggested accessors
The getInterval() method is properly implemented using accessStartInstant() and accessEndInstant() methods, maintaining consistency with the codebase's patterns. The implementation also includes appropriate error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify getInterval implementation uses existing accessors
# Expected: Implementation should call accessStartInstant() and accessEndInstant()
# Search for the implementation
rg -A 10 "getInterval\(\)\s+const" "src/"
Length of output: 6713
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Sequence.cpp (1)
101-111: Enhance documentation for the get_interval method.
The current documentation is minimal compared to other methods in the file. Consider adding more details about:
- What the interval represents (e.g., time span of the sequence)
- The format and components of the returned Interval object
- Any potential exceptions or edge cases
Consider updating the docstring to:
.def(
"get_interval",
&Sequence::Solution::getInterval,
R"doc(
- Get the interval.
+ Get the time interval spanning this sequence solution.
+
+ The interval represents the time span from the start of the first segment
+ to the end of the last segment in this sequence solution.
Returns:
- Interval: The interval.
+ Interval: A time interval object containing the start and end times of the sequence.
)doc"
)Let's verify the C++ implementation:
✅ Verification successful
Documentation enhancement suggestion for get_interval method
The C++ implementation is properly defined in include/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.hpp, and the method is consistently implemented across related classes (Segment, Pass, etc.). The suggested documentation improvements would enhance clarity:
R"doc(
Get the time interval spanning this sequence solution.
The interval represents the time span from the start of the first segment
to the end of the last segment in this sequence solution.
Returns:
Interval: A time interval object containing the start and end times of the sequence.
)doc"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the getInterval method exists in the C++ header
# Expected: Find the method declaration in the Sequence.hpp file
rg -A 2 "getInterval|get_interval" include/
Length of output: 2369
bindings/python/test/trajectory/test_segment.py (1)
15-15: LGTM: Import addition is appropriate.
The added import for Dynamics is necessary for type annotations in the new fixture.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/Segment.cpp (1)
132-142: Verify integration with existing time-related methods.
Let's ensure the new method complements existing functionality without duplication.
✅ Verification successful
The get_interval method is well-integrated and consistent with the codebase
The verification shows that:
- The method follows a consistent pattern used across multiple components (Sequence, Pass, Orbit, Maneuver, etc.)
- Test coverage exists in
test_segment.py - The implementation aligns with similar interval-related functionality in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any potential conflicts or duplications with existing time-related methods
# Test 1: Look for other interval-related methods to ensure no duplication
echo "Checking for other interval-related methods..."
rg -i "interval" bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/
# Test 2: Look for related test coverage
echo "Checking for test coverage..."
rg -i "get_interval|getInterval" bindings/python/test/trajectory/test_segment.py
# Test 3: Check if the method is used in examples or documentation
echo "Checking usage in examples..."
rg -i "get_interval" bindings/python/docs/
Length of output: 7858
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.cpp (1)
36-39: Verify consistent usage of Interval::Closed.
Let's verify that Interval::Closed is used consistently for similar time ranges across the codebase.
✅ Verification successful
Interval::Closed is used consistently across the codebase for time ranges
Based on the search results, Interval::Closed is consistently used throughout the codebase for representing time ranges, particularly in:
- Trajectory intervals (Sequence, Segment, Orbit)
- Flight profiles and maneuvers
- Access windows (AOS/LOS)
- Analysis and validation intervals
No instances of Interval::Open or Interval::HalfOpen were found, confirming that the usage in the reviewed code follows the established pattern in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Interval construction patterns to ensure consistency
rg "Interval::(Closed|Open|HalfOpen)" -A 2
Length of output: 13075
bindings/python/test/trajectory/test_sequence.py (2)
342-352: LGTM! Well-structured fixture.
The fixture follows good pytest practices by:
- Properly injecting dependencies
- Creating a minimal test object with clear state
- Setting explicit completion status
354-393: Verify test coverage for all sequence solution scenarios.
Please ensure that the test suite covers all possible sequence solution scenarios:
- Empty sequence (no segments)
- Failed execution (execution_is_complete = False)
- Multiple segments with different types
src/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.cpp (1)
66-74: Verify usage of getInterval method.
Let's verify that this new method is properly integrated and used in the codebase.
✅ Verification successful
Based on the results, I can now generate the final response:
The getInterval() method is well integrated and actively used across the codebase
The method is properly integrated with:
- Test coverage in
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp - Consistent usage pattern across similar classes like
Pass,Sequence,Maneuver, andAccess - Used in both core functionality and string representations (e.g., in
toString()methods) - The implementation aligns with existing time-related calculations in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential usage of the new getInterval method
# and verify it's not replacing any existing time-related method calls
# Look for existing usage of the new method
echo "Checking for usage of getInterval..."
rg "getInterval\(\)" --type cpp
# Look for potential places where it could be used
# by finding time-related calculations
echo -e "\nChecking for potential usage opportunities..."
rg "accessEndInstant\(\)\s*-\s*accessStartInstant\(\)" --type cpp
Length of output: 5995
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Segment.test.cpp (3)
23-23: LGTM: Include and using directive additions are appropriate.
The additions of the Interval header and its corresponding using directive are properly placed and necessary for the new getInterval functionality.
Also applies to: 77-77
222-222: LGTM: Error case test for getInterval is well-placed.
The test properly verifies that getInterval throws RuntimeError when the segment has no states, following the established pattern of error case testing in this file.
233-236: LGTM: Success case test for getInterval is comprehensive.
The test properly verifies that getInterval returns the correct closed interval defined by the initial and final states' instants.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #455 +/- ##
=======================================
Coverage 90.92% 90.93%
=======================================
Files 85 85
Lines 8618 8624 +6
=======================================
+ Hits 7836 7842 +6
Misses 782 782 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Antoine Paletta <98616558+apaletta3@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
bindings/python/test/trajectory/test_segment.py(2 hunks)bindings/python/test/trajectory/test_sequence.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/python/test/trajectory/test_sequence.py
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
bindings/python/test/trajectory/test_segment.py(2 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp(2 hunks)
🔇 Additional comments (6)
bindings/python/test/trajectory/test_segment.py (2)
138-148: Previous review comment about enhancing the fixture is still applicable.
The fixture could be improved as suggested in the previous review to be more flexible and robust.
151-205: Previous review comment about enhancing test coverage is still applicable.
The test methods could be improved as suggested in the previous review to include validation of actual return values, error conditions, and edge cases.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Sequence.test.cpp (4)
48-48: LGTM!
The addition of the Interval import aligns with the PR objectives to support the new getInterval method.
144-154: Well-structured state generation!
The lambda function generateState improves code reusability and readability by encapsulating the state creation logic.
178-187: Comprehensive test coverage!
The test assertions thoroughly verify the sequence solution properties:
- Start and end instants
- Interval retrieval
- Mass properties
- Propagation duration
- Delta-V and mass calculations
196-201: LGTM!
The test for calculateStatesAt properly verifies the state calculation functionality with clear expectations for the propagated states' instants.
Summary by CodeRabbit
Release Notes
New Features
get_intervalmethod in bothSegment::SolutionandSequence::Solutionclasses, allowing users to retrieve the time interval associated with segment and sequence solutions.Bug Fixes
get_intervalmethods to manage empty states.Tests
Segment.SolutionandSequence.Solutionto validate the functionality of the new interval retrieval methods, ensuring comprehensive coverage.