refactor!: use state instead of instantposvel #493
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of local orbital frame generation across multiple components of the Open Space Toolkit. The primary change involves replacing separate parameters for instant, position, and velocity with a single Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
vishwa2710
force-pushed
the
refactor!/use-state-instead-of-instantposvel
branch
from
37d1c31 to
4e53f24
Compare
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp
Outdated
Show resolved
Hide resolved
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp
Outdated
Show resolved
Hide resolved
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp
Outdated
Show resolved
Hide resolved
bindings/python/test/trajectory/test_local_orbital_frame_factory.py
Outdated
Show resolved
Hide resolved
bindings/python/test/trajectory/test_local_orbital_frame_factory.py
Outdated
Show resolved
Hide resolved
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp
Show resolved
Hide resolved
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp
Outdated
Show resolved
Hide resolved
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp
Outdated
Show resolved
Hide resolved
![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
♻️ Duplicate comments (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (1)
55-58:⚠️ Potential issueFix usage of undefined variables
stateandstateInParentFrameThe variables
stateandstateInParentFrameare undefined in this context. They should beaStateandposVelStateInParentFrame, respectively.Apply this diff to correct the variable names:
- const StateBuilder posVelStateBuilder = StateBuilder(state.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()}); - const State posVelStateInParentFrame = posVelStateBuilder.reduce(aState).inFrame(parentFrameSPtr_); ... - const String name = this->generateFrameName(stateInParentFrame); + const StateBuilder posVelStateBuilder = StateBuilder(aState.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()}); + const State posVelStateInParentFrame = posVelStateBuilder.reduce(aState).inFrame(parentFrameSPtr_); ... + const String name = this->generateFrameName(posVelStateInParentFrame);
🧹 Nitpick comments (7)
bindings/python/test/trajectory/test_local_orbital_frame_factory.py (1)
55-61: Consider adding validation for input parameters.While the state fixture correctly combines instant, position, and velocity, it might be good to add validation to ensure the position and velocity are in compatible reference frames.
@pytest.fixture def state( instant: Instant, position: Position, velocity: Velocity, ) -> State: + if position.get_frame() != velocity.get_frame(): + raise ValueError("Position and velocity must be in the same reference frame") return State(instant, position, velocity)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameTransformProvider.cpp (1)
116-116: Consider adding type hints in the documentation.While the return type documentation is correct, it could be more helpful to include type hints for the State parameter.
- callable[[State], Transform]: The transform generator function. + callable[[State: ostk.astrodynamics.trajectory.State], Transform]: The transform generator function.include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
43-46: Consider adding validation requirements in documentation.While the method signatures are correctly updated, the documentation could benefit from specifying any requirements or constraints on the State parameter (e.g., reference frame compatibility).
/// @brief Generate a frame shared pointer based on current state input /// - /// @param aState A State + /// @param aState A State (position and velocity must be in compatible reference frames) /// /// @return A shared pointer to the frame createdAlso applies to: 152-155
test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.test.cpp (1)
61-62: Consider adding edge case tests.While the happy path is tested, consider adding tests for edge cases such as:
- State with zero velocity
- State with zero position
- State with position and velocity in opposite directions
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameFactory.cpp (1)
Line range hint
56-65: Enhance docstring with State parameter details.Consider adding more details about the State parameter in the docstring, such as its components (instant, position, velocity) and their expected units.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.test.cpp (1)
Line range hint
110-133: Consider extracting transform generation logic.The lambda function contains complex transform generation logic. Consider extracting this into a separate helper function for better maintainability and reusability.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp (1)
123-126: Consider const correctness for State access.The
accessCoordinates()calls could potentially be replaced withgetCoordinates()for better const correctness, as we're only reading the values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameFactory.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameTransformProvider.cpp(3 hunks)bindings/python/test/trajectory/test_local_orbital_frame_factory.py(4 hunks)bindings/python/test/trajectory/test_segment.py(0 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp(2 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp(2 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp(2 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.test.cpp(4 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.test.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Validation/Propagator.cross.validation.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- bindings/python/test/trajectory/test_segment.py
🧰 Additional context used
📓 Learnings (1)
src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp (1)
Learnt from: vishwa2710
PR: open-space-collective/open-space-toolkit-astrodynamics#492
File: src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp:37-74
Timestamp: 2025-01-10T22:45:19.607Z
Learning: In the OpenSpaceToolkit Astrodynamics library, validation for edge cases like zero direction vector (when satellite position coincides with target position) and zero velocity vector is not required in the `ComputeOffNadirAngles` function, as these scenarios are considered extremely unlikely in real-world applications.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Development Image / Build Development Image
🔇 Additional comments (16)
src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp (1)
51-51: Update to useaStateingenerateFrameThe change to use
aStateingenerateFrameimproves code clarity and aligns with the updated method signature.src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp (3)
20-22: IncludePositionandVelocityin using directivesAdding
PositionandVelocityto the using directives enhances code readability.
59-63: Initialization ofStateobjectThe
Stateobject is correctly initialized withanInstant,Position, andVelocity, which aligns with the refactoring efforts for better data encapsulation.
66-66: Consistent usage ofstatewithgenerateFrameUsing
statewithgenerateFramealigns with the updated method signatures and maintains consistency across the codebase.bindings/python/test/trajectory/test_local_orbital_frame_factory.py (2)
44-46: LGTM! Well-structured fixtures for position and velocity.The fixtures correctly initialize Position and Velocity objects with appropriate coordinate frames.
Also applies to: 49-52
83-83: LGTM! Clean test refactoring.The test has been properly updated to use the new state parameter, maintaining test clarity while reducing parameter count.
Also applies to: 85-85
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameTransformProvider.cpp (1)
22-22: LGTM! Clear and consistent documentation updates.The documentation has been properly updated to reflect the new State parameter usage, maintaining consistency with the C++ implementation.
Also applies to: 97-97, 103-103
include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (2)
15-16: LGTM! Proper include and using declaration.The State header is correctly included, and the using declaration is appropriately placed.
Also applies to: 35-36
90-94: LGTM! Clean method signature updates.The method signatures have been consistently updated to use the State parameter, improving the API's cohesion.
Also applies to: 102-102, 119-122
include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp (1)
40-40: LGTM! Clean typedef update.The TransformGenerator typedef has been properly updated to use the State parameter.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.test.cpp (1)
39-43: LGTM! Clean refactoring to use State.The consolidation of instant, position, and velocity into a single State object improves encapsulation and reduces parameter coupling.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameFactory.cpp (1)
184-193: LGTM! Clean signature update.The change from multiple parameters to a single State parameter simplifies the interface while maintaining functionality.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.test.cpp (1)
187-198: LGTM! Good test coverage.The test cases properly verify frame generation with the new State parameter.
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp (2)
146-151: Add missing axis direction comment.Previous review comments suggest adding the third axis direction comment for consistency.
Add a comment for the Y axis direction to maintain consistency with other frame definitions.
169-174: Standardize axis direction comments.For consistency with other frame definitions, consider standardizing the terminology:
- Use "along" instead of "toward" consistently
- Add the missing third axis direction comment
test/OpenSpaceToolkit/Astrodynamics/Trajectory/Validation/Propagator.cross.validation.cpp (1)
844-844: LGTM! Good use of the State object.The refactoring to use a
Stateobject instead of passing instant, position, and velocity separately improves encapsulation and reduces parameter count. This change aligns well with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp (2)
123-125: Consider using const references for better performance.When accessing state components, using const references can avoid unnecessary copies:
- const Instant& instant = aState.accessInstant(); - const Vector3d positionCoordinates = aState.getPosition().accessCoordinates(); - const Vector3d velocityCoordinates = aState.getVelocity().accessCoordinates(); + const Instant& instant = aState.accessInstant(); + const Vector3d& positionCoordinates = aState.getPosition().accessCoordinates(); + const Vector3d& velocityCoordinates = aState.getVelocity().accessCoordinates();
155-157: Track TBD angular velocity implementation.All frame implementations have a TBD comment for angular velocity. This should be tracked and implemented.
Would you like me to create a GitHub issue to track the implementation of angular velocity for all frames?
Also applies to: 178-180, 201-203, 224-226, 247-249
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (2)
61-62: Fix variable name inconsistency.The variable name 'state' should be 'aState' to match the parameter name and maintain consistency with the codebase's naming conventions.
- StateBuilder(state.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()}); + StateBuilder(aState.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()});
177-180: Consider using String::Format for better readability.The string concatenation could be made more readable using String::Format. Also, consider adding error handling for toString() calls.
- return LocalOrbitalFrameTransformProvider::StringFromType(type_) + "@" + aState.accessInstant().toString() + - aState.getPosition().getCoordinates().toString() + aState.getVelocity().getCoordinates().toString(); + return String::Format( + "{type}@{instant}{position}{velocity}", + { + {"type", LocalOrbitalFrameTransformProvider::StringFromType(type_)}, + {"instant", aState.accessInstant().toString()}, + {"position", aState.getPosition().getCoordinates().toString()}, + {"velocity", aState.getVelocity().getCoordinates().toString()} + } + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp(8 hunks)
🔇 Additional comments (6)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp (3)
39-42: LGTM! Clean interface simplification.The refactoring to use a single
Stateparameter improves the API by encapsulating related data and reducing parameter count.
70-76: LGTM! Consistent lambda adaptation.The transform generator lambda is correctly updated to match the new State-based interface.
146-150: Add consistent axis direction comments across all frames.Based on past review comments, each frame should have consistent documentation for all axes:
For LVLH frame:
- // X axis along position vector - // Z axis along orbital momentum + // X axis aligned with position vector + // Y axis perpendicular to orbital plane, opposite to orbital momentum + // Z axis completes right-handed systemFor VVLH frame:
- // Z axis along negative position vector - // Y axis along negative orbital momentum + // X axis completes right-handed system + // Y axis aligned with negative orbital momentum + // Z axis aligned with negative position vectorSimilar documentation should be added for QSW, TNW, and VNC frames.
Also applies to: 169-173, 192-196, 215-219, 238-242
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
18-20: LGTM! New includes align with the refactoring.The added includes and using declarations are necessary and properly organized for the State-based refactoring.
Also applies to: 42-44
59-66: LGTM! Implementation follows best practices.The refactored code properly handles state reduction and frame transformation, following the suggested approach from previous reviews.
177-180: Add error handling for toString operations.Consider adding validation to ensure that instant, position, and velocity are defined before calling toString().
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp
Show resolved
Hide resolved
vishwa2710
force-pushed
the
refactor!/use-state-instead-of-instantposvel
branch
from
d968510 to
583aa98
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp (1)
Line range hint
154-162: Track the TBD implementation of angular velocity.The angular velocity is currently set to zero with a TBD comment. This should be properly implemented to ensure accurate frame transformations.
Would you like me to open a GitHub issue to track the implementation of proper angular velocity calculations for each frame type?
Also applies to: 177-185, 200-208, 223-231, 246-254
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameFactory.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameTransformProvider.cpp(3 hunks)bindings/python/test/trajectory/test_local_orbital_frame_factory.py(4 hunks)bindings/python/test/trajectory/test_segment.py(0 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp(2 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp(2 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.test.cpp(4 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.test.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Validation/Propagator.cross.validation.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- bindings/python/test/trajectory/test_segment.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp
- src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameTransformProvider.cpp
- test/OpenSpaceToolkit/Astrodynamics/Trajectory/Validation/Propagator.cross.validation.cpp
- include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp
🔇 Additional comments (17)
bindings/python/test/trajectory/test_local_orbital_frame_factory.py (3)
44-52: LGTM! Well-structured fixtures for position and velocity.The fixtures are well-implemented with proper typing and use of the Frame.GCRF() reference frame.
55-61: LGTM! Clean state fixture implementation.The state fixture properly combines instant, position, and velocity into a single State object, improving encapsulation.
83-85: LGTM! Test case updated correctly.The test case has been properly updated to use the new state parameter, maintaining test coverage while adopting the new API.
include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (3)
90-94: LGTM! Method signature updated to use State.The Construct method now accepts a State parameter instead of separate instant, position, and velocity parameters, improving encapsulation.
102-103: LGTM! Transform generator signature updated.The GetTransformGenerator method now returns a function that takes a State parameter, maintaining consistency with the new API.
119-122: LGTM! Private method signature updated.The GenerateTransform method has been updated to use State parameter, and the method name has been properly capitalized.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.test.cpp (2)
39-43: LGTM! Well-structured state initialization.The state object is properly constructed with instant, position, and velocity, using appropriate units and reference frame.
60-62: LGTM! Test cases updated consistently.All test cases have been updated to use the state parameter, maintaining comprehensive test coverage for different frame types.
Also applies to: 66-68, 72-74, 78-80, 84-86, 90-92
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (1)
177-180: LGTM! Frame name generation updated.The generateFrameName method has been properly updated to extract components from the State object.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameFactory.cpp (2)
56-61: LGTM! Documentation updated for new parameter.The Python binding documentation has been properly updated to reflect the new state parameter.
184-193: LGTM! Construct method overload updated.The construct method overload has been properly updated to use State parameter in the transform generator signature.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.test.cpp (4)
49-49: LGTM!The addition of the
Statetype aligns with the PR objective to use a single state object instead of separate parameters.
62-66: LGTM!The
state_member variable is correctly initialized using existing member variables, with proper frame specifications.
110-116: Consider explicit frame conversion to GCRF.While the implementation works, consider explicitly converting the state to GCRF frame before accessing coordinates to ensure consistency with the coordinate frame assumptions in the transform calculations.
- const auto aTransformGenerator = [](const State& aState) -> Transform + const auto aTransformGenerator = [](const State& aState) -> Transform { - const Vector3d transformPosition = -aState.getPosition().getCoordinates(); - const Vector3d transformVelocity = -aState.getVelocity().getCoordinates(); + const State& stateInGCRF = aState.inFrame(Frame::GCRF()); + const Vector3d transformPosition = -stateInGCRF.getPosition().getCoordinates(); + const Vector3d transformVelocity = -stateInGCRF.getVelocity().getCoordinates();
187-187: LGTM!The calls to
generateFrameare correctly updated to use the newstate_parameter.Also applies to: 195-195
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp (2)
123-125: Consider explicit frame conversion to GCRF.While the implementation works, consider explicitly converting the state to GCRF frame before accessing coordinates to ensure consistency with the coordinate frame assumptions in the transform calculations.
- const Instant& instant = aState.accessInstant(); - const Vector3d positionCoordinates = aState.getPosition().accessCoordinates(); - const Vector3d velocityCoordinates = aState.getVelocity().accessCoordinates(); + const State& stateInGCRF = aState.inFrame(Frame::GCRF()); + const Instant& instant = stateInGCRF.accessInstant(); + const Vector3d positionCoordinates = stateInGCRF.getPosition().accessCoordinates(); + const Vector3d velocityCoordinates = stateInGCRF.getVelocity().accessCoordinates();
146-150: Consider adding consistent axis documentation.For better clarity and consistency, consider documenting the third axis direction for all frame types. This would help users understand the complete orientation of each frame.
For example:
case LocalOrbitalFrameTransformProvider::Type::LVLH: { // X axis along position vector // Z axis along orbital momentum + // Y axis completes the right-handed systemAlso applies to: 168-173, 192-196, 215-219, 238-242
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (2)
59-74: LGTM! Clean implementation of State-based frame generation.The implementation correctly handles the State object and follows good practices for frame transformation. Consider adding a comment explaining why we reduce to position/velocity state for better maintainability.
Add a comment like:
const StateBuilder positionVelocityStateBuilder = + // Reduce to position/velocity state as other coordinates might not be portable to another frame StateBuilder(aState.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()});
177-180: Consider using string formatting for better readability.While the implementation is functionally correct, the string concatenation could be made more readable using string formatting utilities.
Consider using std::format (C++20) or a string stream for better readability:
- return LocalOrbitalFrameTransformProvider::StringFromType(type_) + "@" + aState.accessInstant().toString() + - aState.getPosition().getCoordinates().toString() + aState.getVelocity().getCoordinates().toString(); + std::ostringstream nameStream; + nameStream << LocalOrbitalFrameTransformProvider::StringFromType(type_) << "@" + << aState.accessInstant().toString() + << aState.getPosition().getCoordinates().toString() + << aState.getVelocity().getCoordinates().toString(); + return nameStream.str();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp(4 hunks)
🔇 Additional comments (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (2)
18-20: LGTM! Necessary includes for State-based implementation.The new includes are correctly ordered and necessary for working with State objects, CartesianPosition, and CartesianVelocity.
42-44: LGTM! Consistent using declarations.The new using declarations follow the established pattern and provide convenient access to the required types.
alex-liang3
force-pushed
the
refactor!/use-state-instead-of-instantposvel
branch
from
a436d6e to
ee7953b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (1)
61-65: Consider optimizing state reduction.The current implementation could be optimized to avoid potential frame conversion issues by reducing the state first:
- const StateBuilder positionVelocityStateBuilder = - StateBuilder(aState.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()}); - - const State positionVelocityStateInParentFrame = - positionVelocityStateBuilder.reduce(aState).inFrame(parentFrameSPtr_); + const StateBuilder posVelStateBuilder = StateBuilder(aState.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()}) + const State posVelStateInParentFrame = posVelStateBuilder.reduce(aState).inFrame(parentFrameSPtr_);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameFactory.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameTransformProvider.cpp(3 hunks)bindings/python/test/trajectory/test_local_orbital_frame_factory.py(4 hunks)bindings/python/test/trajectory/test_segment.py(0 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp(2 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp(2 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.test.cpp(4 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.test.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Validation/Propagator.cross.validation.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- bindings/python/test/trajectory/test_segment.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp
- src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameTransformProvider.cpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameFactory.cpp
- test/OpenSpaceToolkit/Astrodynamics/Trajectory/Validation/Propagator.cross.validation.cpp
- include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test / Check Python Format
- GitHub Check: Test / Run C++ Unit Tests
- GitHub Check: Test / Check C++ Format
- GitHub Check: Test / Build Python bindings
🔇 Additional comments (11)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp (3)
39-44: LGTM! Good refactoring to use State object.The method signature changes consistently replace separate parameters with a single State object, improving encapsulation and reducing parameter coupling.
Also applies to: 70-77
123-125: Consider explicit frame conversion to GCRF.Previous discussions suggested explicitly converting to GCRF coordinates before calculations.
Consider updating the code as follows:
- const Vector3d positionCoordinates = aState.getPosition().accessCoordinates(); - const Vector3d velocityCoordinates = aState.getVelocity().accessCoordinates(); + const State& stateInGCRF = aState.inFrame(Frame::GCRF()); + const Vector3d positionCoordinates = stateInGCRF.getPosition().accessCoordinates(); + const Vector3d velocityCoordinates = stateInGCRF.getVelocity().accessCoordinates();
Line range hint
147-163: Add missing axis comments for consistency.Some frame types are missing axis comments. For consistency, add the missing comments:
- LVLH: Add "Y axis perpendicular to orbital plane"
- QSW: Add "Y axis completes right-handed system"
- TNW: Add "Y axis completes right-handed system"
- VNC: Add "Z axis completes right-handed system"
Also applies to: 193-209, 216-233, 239-256
bindings/python/test/trajectory/test_local_orbital_frame_factory.py (1)
Line range hint
1-24: LGTM! The changes improve code clarity and maintainability.The refactoring to use a single
Stateobject instead of separate parameters enhances encapsulation and aligns with the PR objectives. The addition of type hints forPositionandVelocityfollows Python best practices.include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (2)
15-16: LGTM! Required include for State type.The addition of
State.hppinclude is necessary to support the refactoring to useStateobjects.
90-94: LGTM! Method signatures refactored to use State object.The changes improve the API by:
- Reducing parameter count
- Enhancing encapsulation through
Stateobject- Following consistent naming conventions
Also applies to: 102-104, 119-122
test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.test.cpp (1)
39-43: LGTM! Test class updated to use State object.The changes improve test readability by encapsulating test data in a
Stateobject.src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (2)
18-20: LGTM! Required includes for state reduction.The addition of includes for CartesianPosition, CartesianVelocity, and StateBuilder support the state reduction implementation.
177-180: LGTM! Frame name generation updated to use State object.The implementation correctly uses State object methods to access instant, position, and velocity coordinates.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.test.cpp (2)
62-66: LGTM! Test class updated to use State object.The changes improve test readability by encapsulating test data in a State object.
Line range hint
110-131: LGTM! Transform generator updated to use State object.The implementation correctly uses State object methods to access position and velocity coordinates for transform generation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (1)
177-180: Consider reducing state before generating frame name.For consistency with
generateFrame, consider reducing the state to position and velocity components before accessing them. This ensures that only the required coordinates are used and maintains a consistent approach throughout the class.String LocalOrbitalFrameFactory::generateFrameName(const State& aState) const { + const StateBuilder positionVelocityStateBuilder = + StateBuilder(aState.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()}); + const State reducedState = positionVelocityStateBuilder.reduce(aState); return LocalOrbitalFrameTransformProvider::StringFromType(type_) + "@" + - aState.accessInstant().toString() + - aState.getPosition().getCoordinates().toString() + - aState.getVelocity().getCoordinates().toString(); + reducedState.accessInstant().toString() + + reducedState.getPosition().getCoordinates().toString() + + reducedState.getVelocity().getCoordinates().toString();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp(4 hunks)
🔇 Additional comments (3)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
18-20: LGTM! Well-organized includes and using declarations.The new includes and using declarations for state-related components are properly organized and necessary for the refactoring.
Also applies to: 42-44
59-65: Well-implemented state reduction!The implementation correctly follows best practices by:
- Explicitly declaring required coordinate subsets (position and velocity)
- Reducing the state before frame transformation
- Safely handling coordinate transformations
Line range hint
74-85: LGTM! Clean transform generation and frame creation.The implementation maintains the existing frame creation logic while cleanly integrating the new state-based approach.
Co-authored-by: Antoine Paletta <98616558+apaletta3@users.noreply.github.com> Co-authored-by: Pau Hebrero <65550121+phc1990@users.noreply.github.com>
vishwa2710
force-pushed
the
refactor!/use-state-instead-of-instantposvel
branch
from
8e571f9 to
8b6210f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (1)
59-66: Consider optimizing state reduction.The current implementation creates a new
StateBuilderand reduces the state to position and velocity components. Consider reducing it first to avoid potential issues with coordinate subset portability.Apply this diff to optimize the state reduction:
- const StateBuilder positionVelocityStateBuilder = - StateBuilder(aState.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()}); - - const State positionVelocityStateInParentFrame = - positionVelocityStateBuilder.reduce(aState).inFrame(parentFrameSPtr_); + const StateBuilder posVelStateBuilder = StateBuilder(aState.getFrame(), {CartesianPosition::Default(), CartesianVelocity::Default()}); + const State posVelStateInParentFrame = posVelStateBuilder.reduce(aState).inFrame(parentFrameSPtr_);src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp (1)
Line range hint
147-162: Reduce code duplication in transform construction.The transform construction pattern is repeated across all frame types with only axis calculations being different. Consider extracting the common logic into a helper function.
Example refactoring:
private: static Transform CreateTransform( const Instant& instant, const Vector3d& position, const Vector3d& velocity, const std::function<std::tuple<Vector3d, Vector3d, Vector3d>(const Vector3d&, const Vector3d&)>& computeAxes ) { const Vector3d transformPosition = -position; const Vector3d transformVelocity = -velocity; const auto [xAxis, yAxis, zAxis] = computeAxes(position, velocity); const Quaternion transformOrientation = Quaternion::RotationMatrix(RotationMatrix::Rows(xAxis, yAxis, zAxis)) .toNormalized() .rectify(); const Vector3d transformAngularVelocity = {0.0, 0.0, 0.0}; // TBD return { instant, transformPosition, transformVelocity, transformOrientation, transformAngularVelocity, Transform::Type::Passive }; }Usage example for LVLH:
case LocalOrbitalFrameTransformProvider::Type::LVLH: { return CreateTransform( instant, positionCoordinates, velocityCoordinates, [](const Vector3d& position, const Vector3d& velocity) { const Vector3d xAxis = position.normalized(); const Vector3d zAxis = position.cross(velocity).normalized(); const Vector3d yAxis = zAxis.cross(xAxis); return std::make_tuple(xAxis, yAxis, zAxis); } ); }Also applies to: 170-187, 193-209, 216-233, 239-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameFactory.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameTransformProvider.cpp(3 hunks)bindings/python/test/trajectory/test_local_orbital_frame_factory.py(4 hunks)bindings/python/test/trajectory/test_segment.py(0 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp(2 hunks)include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp(2 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp(4 hunks)src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.test.cpp(4 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.test.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Validation/Propagator.cross.validation.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- bindings/python/test/trajectory/test_segment.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/ConstantThrust.cpp
- src/OpenSpaceToolkit/Astrodynamics/Data/Provider/OffNadir.cpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameTransformProvider.cpp
- include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.hpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Trajectory/LocalOrbitalFrameFactory.cpp
- test/OpenSpaceToolkit/Astrodynamics/Trajectory/Validation/Propagator.cross.validation.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Development Image / Build Development Image
🔇 Additional comments (23)
bindings/python/test/trajectory/test_local_orbital_frame_factory.py (4)
44-45: LGTM! The position fixture is well-defined.The fixture correctly constructs a
Positionobject with the GCRF frame.
49-52: LGTM! The velocity fixture is well-defined.The fixture correctly constructs a
Velocityobject with the GCRF frame.
55-61: LGTM! The state fixture is well-defined.The fixture correctly constructs a
Stateobject by combining instant, position, and velocity.
83-85: LGTM! Test case updated to use State object.The test case has been properly updated to use the new
Stateparameter instead of separate parameters.include/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.hpp (5)
15-16: LGTM! New include directive added.The include directive for
State.hppis correctly added to support the new parameter type.
35-36: LGTM! Using directive added.The using directive for
Stateis correctly added.
90-94: LGTM! Method signature updated.The
Constructmethod signature is correctly updated to useStateinstead of separate parameters.
102-104: LGTM! Method signature updated.The
GetTransformGeneratormethod signature is correctly updated to useStateinstead of separate parameters.
119-122: LGTM! Method signature updated.The
GenerateTransformmethod signature is correctly updated to useStateinstead of separate parameters.test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.test.cpp (4)
18-19: LGTM! Using directives added.The using directives for
Frame,Position,Velocity, andStateare correctly added.Also applies to: 21-21, 28-28
39-43: LGTM! State member variable added.The
state_member variable is correctly initialized with instant, position, and velocity.
60-62: LGTM! Test cases updated.All test cases are correctly updated to use the new
Stateparameter instead of separate parameters.Also applies to: 66-68, 72-74, 78-80, 84-86, 90-92, 97-97, 103-103
136-136: LGTM! Transform generator tests updated.All transform generator tests are correctly updated to use the new
Stateparameter.Also applies to: 146-146, 156-156, 166-166, 176-176
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.cpp (3)
18-20: LGTM! New includes added.The includes for state-related headers are correctly added.
42-44: LGTM! Using directives added.The using directives for state-related types are correctly added.
177-180: LGTM! Frame name generation updated.The frame name generation is correctly updated to use the state object's methods.
test/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameFactory.test.cpp (4)
49-49: LGTM! Using directive added.The using directive for
Stateis correctly added.
62-66: LGTM! State member variable added.The
state_member variable is correctly initialized with instant, position, and velocity.
Line range hint
110-132: LGTM! Transform generator lambda updated.The transform generator lambda is correctly updated to use the state object's methods for accessing position and velocity.
187-187: LGTM! Test cases updated.The test cases are correctly updated to use the new
Stateparameter.Also applies to: 195-195
src/OpenSpaceToolkit/Astrodynamics/Trajectory/LocalOrbitalFrameTransformProvider.cpp (3)
124-125: Consider explicitly converting state to GCRF frame.The position and velocity coordinates are accessed directly from the state without ensuring they're in the GCRF frame, which could lead to incorrect transformations if the state is provided in a different frame.
#!/bin/bash # Search for any frame conversions or GCRF usage in the codebase rg -A 2 "inFrame\(Frame::GCRF\(\)\)" || echo "No explicit GCRF conversions found"Consider converting the state to GCRF explicitly:
- const Vector3d positionCoordinates = aState.getPosition().accessCoordinates(); - const Vector3d velocityCoordinates = aState.getVelocity().accessCoordinates(); + const State& stateInGCRF = aState.inFrame(Frame::GCRF()); + const Vector3d positionCoordinates = stateInGCRF.getPosition().accessCoordinates(); + const Vector3d velocityCoordinates = stateInGCRF.getVelocity().accessCoordinates();
169-173: Add missing axis direction comment and standardize axis comments.The VVLH frame has incomplete axis comments. For consistency with other frames, add the missing X axis direction comment and standardize the axis direction terminology across all frame types.
- // Z axis along negative position vector - // Y axis along negative orbital momentum + // X axis toward velocity vector + // Y axis along negative orbital momentum + // Z axis along negative position vector
147-150: Consider implementing angular velocity calculations.The angular velocity is currently set to zero with a TBD comment across all frame types. This could affect the accuracy of frame transformations, especially for rapidly changing orbits.
Would you like me to help implement the angular velocity calculations for each frame type? This would involve:
- Deriving the angular velocity expressions for each frame type
- Implementing the calculations using the state vectors
- Adding unit tests to verify the implementations
Also applies to: 170-173, 193-196, 216-219, 239-242
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #493 +/- ##
=======================================
Coverage 91.22% 91.23%
=======================================
Files 87 87
Lines 8789 8796 +7
=======================================
+ Hits 8018 8025 +7
Misses 771 771 ☔ View full report in Codecov by Sentry. |
Closes #433
Summary by CodeRabbit
New Features
Stateobject that consolidates instant, position, and velocity parameters across multiple components.Refactor
Stateobject.Stateobject.Documentation
Tests
Stateobject.