feat!: add correct computeJacobian method #485
Conversation
…teDifferenceSolver class and add computeJacobian
WalkthroughThe pull request introduces significant modifications to the Changes
Sequence DiagramsequenceDiagram
participant Solver as FiniteDifferenceSolver
participant State as State Coordinates
participant Function as Derivative Function
Solver->>State: Access initial state
Solver->>State: Apply perturbations
Solver->>Function: Compute derivatives
Solver->>Solver: Calculate State Transition Matrix or Jacobian
Solver-->>State: Return computed matrix
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: 0
🧹 Nitpick comments (3)
src/OpenSpaceToolkit/Astrodynamics/Solvers/FiniteDifferenceSolver.cpp (1)
141-141: Minor improvement suggestion for consistency.
Consider encapsulating the repeated pattern of computingstepSizein a small inline helper method to avoid duplication among your finite difference methods and to simplify code.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Solver/FiniteDifferenceSolver.cpp (1)
128-137: Docstring discrepancy.
The docstring mentions "Compute the jacobian," yet the method returns an STM. Ensure the docstring matches the actual functionality (STM).- Compute the jacobian. + Compute the state transition matrix (STM).src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.cpp (1)
662-662: Clarify function naming to avoid confusion.Here, you retrieve a 5×1 vector of partial derivatives from
computeStateTransitionMatrix, yet store that result in a variable namedjacobian. Since this code is computing the gradient of a scalar function Q, consider calling the newly introducedcomputeJacobianmethod, or at least renaming the variable to avoid mixing the conceptual “Jacobian” of Q with a “state transition matrix.” This will help maintain clarity and consistency for future maintainers.Here's a possible change to call
computeJacobian(assuming it produces the same 5×1 vector of partial derivatives) and rename the local variable:- const Vector5d jacobian = Eigen::Map<Vector5d>( - finiteDifferenceSolver_ - .computeStateTransitionMatrix(stateBuilder_.build(Instant::J2000(), aCOEVector), Instant::J2000(), getQ, 1) - .data(), - 5 - ); + const Vector5d dQ_dOE_numeric = Eigen::Map<Vector5d>( + finiteDifferenceSolver_ + .computeJacobian(stateBuilder_.build(Instant::J2000(), aCOEVector), Instant::J2000(), getQ, 1) + .data(), + 5 + ); + + return dQ_dOE_numeric;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Solver/FiniteDifferenceSolver.cpp(2 hunks)bindings/python/test/solvers/test_finite_difference_solver.py(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Solver/FiniteDifferenceSolver.hpp(3 hunks)src/OpenSpaceToolkit/Astrodynamics/GuidanceLaw/QLaw.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Solvers/FiniteDifferenceSolver.cpp(7 hunks)test/OpenSpaceToolkit/Astrodynamics/Solver/FiniteDifferenceSolver.test.cpp(14 hunks)
🔇 Additional comments (25)
src/OpenSpaceToolkit/Astrodynamics/Solvers/FiniteDifferenceSolver.cpp (6)
53-53: Great rename for clarity.
Renaming from a Jacobian-centric name tocomputeStateTransitionMatrixmore precisely reflects its purpose.
152-152: Useful overloaded method.
This overload improves usability when a single instant is involved, ensuring consistent usage patterns.
170-170: Concise bridging function.
The local lambda approach to adapt a single-instant function into a multi-instant context is both elegant and succinct.
181-181: Good structured approach.
Using the chosen finite difference scheme consistently incomputeGradienthelps maintain coded clarity across forward, backward, and central difference logic.
193-193: Validation: negative step durations.
Consider checking ifstepDuration_is positive before using it. Negative or zero durations could produce unexpected results or divide-by-zero errors.
219-306: Well-structured Jacobian computation.
The introduction ofcomputeJacobianseparately fromcomputeStateTransitionMatrixclarifies the difference between these matrices. Good job on distinct flows for forward, backward, and central differences.include/OpenSpaceToolkit/Astrodynamics/Solver/FiniteDifferenceSolver.hpp (3)
Line range hint
86-96: Accurate docstrings.
The documentation now aligns well with the actual computation of the state transition matrix (STM).
103-112: Helpful overload.
Providing an overload for a single instant usage scenario is user-friendly and consistent.
130-139: Clear separation of responsibilities.
DefiningcomputeJacobianas a separate public method helps disambiguate math concepts for end-users. Good clarity in the docstrings.bindings/python/test/solvers/test_finite_difference_solver.py (3)
129-136: Consistent method rename.
Refactoring the test totest_compute_state_transition_matrix_arrayaligns with the newly introduced naming in the solver.
148-155: Clear single-instant STM test.
Renaming totest_compute_state_transition_matrix_singleand verifying shape correctness ensures coverage for the single-instant path.
180-192: Robust check for the newly addedcompute_jacobian.
This test method properly ensures that the returned Jacobian is accurate. Great that it checks numerical precision.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Solver/FiniteDifferenceSolver.cpp (2)
99-108: Improved naming alignment.
Renaming the Python binding tocompute_state_transition_matrixclarifies usage for Python consumers.
178-185: New binding for Jacobian.
Properly exposescomputeJacobianto Python. The docstring precisely defines input and output expectations.test/OpenSpaceToolkit/Astrodynamics/Solver/FiniteDifferenceSolver.test.cpp (11)
7-12: Added Earth environment includes.
These headers are essential to test gravitational models. Good addition to broaden coverage.
25-28: Imported matrix objects.
Clear usage ofMatrixXdandVectorXd. Maintains consistency with the rest of the solver code.
Line range hint
48-73: Refactor function naming.
Renaming helper lambdas togenerateStatesCoordinates_andgenerateStateCoordinates_clarifies their usage.
98-98: Step duration in microseconds.
Using microseconds can improve numerical accuracy but also consider potential floating-point rounding issues when computations involve large intervals.
112-113: Convenient function objects.
Storing lambdas asstd::functions is flexible and keeps the test setup organized.
Line range hint
147-193: Thorough STM testing with multiple difference schemes.
These tests validate thatcomputeStateTransitionMatrixis consistent across central, forward, and backward difference methods. Excellent approach.
Line range hint
200-236: Single-instant STM coverage.
Well-structured single-instant test block verifies shape and numeric correctness with each difference scheme.
Line range hint
252-277: Gradient checks.
Validating the gradient result with known expected outputs is a solid approach. Has coverage across difference schemes.
280-354: Extended two-body problem gradient test.
Comprehensive coverage with real-world physics ensures correctness in more complex scenarios. Great job.
384-444: General Jacobian test.
Confirms functional correctness ofcomputeJacobianfor a 2D simple harmonic oscillator. Proper numerical tolerance checks.
446-523: Robust two-body Jacobian test.
Brings added confidence in the solver’s real-world utility. Great to see domain-specific checks with gravitational effects.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #485 +/- ##
==========================================
- Coverage 91.21% 91.20% -0.01%
==========================================
Files 86 86
Lines 8720 8769 +49
==========================================
+ Hits 7954 7998 +44
- Misses 766 771 +5 ☔ View full report in Codecov by Sentry. |
computeJacobiantocomputeStateTransitionMatrixas that is what the method was actually doingcomputeJacobianmethod that computes the jacobian of a state vectorSummary by CodeRabbit
New Features
Refactor
computeJacobianmethod tocomputeStateTransitionMatrixTests