feat: add utility methods to compare orbits and states #596
Conversation
Updated docstrings for compare_orbit_to_states and compare_states_to_states to accurately reflect the actual return values. The functions return dictionaries with detailed position and velocity residuals including both L2 norms (dr, dv) and component-wise differences (dr_x, dr_y, dr_z, dv_x, dv_y, dv_z), along with timestamps. Updated all related tests to check for the correct dictionary keys and structure instead of the previously incorrect return types. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
=======================================
Coverage 91.98% 91.98%
=======================================
Files 113 113
Lines 10686 10686
=======================================
Hits 9830 9830
Misses 856 856 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThese changes introduce state and orbit comparison utilities to the dataframe module, enabling residual calculations between state sequences and orbit-to-state comparisons. Comprehensive test coverage is added with fixtures for timestamps, frames, positions, velocities, and states to validate the new functionality. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Suite
participant CompareFunc as compare_orbit_to_states()
participant CompareFuncBase as compare_states_to_states()
participant Orbit as Orbit
participant State as State
Test->>CompareFunc: compare_orbit_to_states(orbit, states)
CompareFunc->>State: derive Instant from each state
CompareFunc->>State: convert Instant to UTC datetime
CompareFunc->>Orbit: get orbit state at each UTC datetime
CompareFunc->>CompareFuncBase: compare_states_to_states(orbit_states, input_states)
CompareFuncBase->>CompareFuncBase: compute residuals (dr, dv, components)<br/>in GCRF frame
CompareFuncBase-->>CompareFunc: return list of dicts with<br/>timestamp, dr, dv, components
CompareFunc-->>Test: return comparison results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/python/tools/python/ostk/astrodynamics/dataframe.py (1)
1-689: Run the project formatter to fix formatting issues.The pipeline indicates that the file needs reformatting.
Run the following command to fix formatting:
make formatOr use the project's configured Python formatter (likely Black or Ruff) to reformat this file.
🧹 Nitpick comments (1)
bindings/python/tools/python/ostk/astrodynamics/dataframe.py (1)
636-636: Addstrict=Trueto zip() for safer iteration.Without
strict=True,zip()silently truncates to the shorter list ifstates_1andstates_2have different lengths, potentially masking bugs.Apply this diff:
- for state_1, state_2 in zip(states_1, states_2): + for state_1, state_2 in zip(states_1, states_2, strict=True):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bindings/python/test/test_dataframe.py(2 hunks)bindings/python/tools/python/ostk/astrodynamics/dataframe.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
bindings/python/test/test_dataframe.py (2)
bindings/python/tools/python/ostk/astrodynamics/dataframe.py (4)
generate_states_from_dataframe(130-269)compare_states_to_states(612-659)compare_orbit_to_states(662-689)generate_orbit_from_dataframe(388-440)bindings/python/tools/python/ostk/astrodynamics/converters.py (1)
coerce_to_datetime(14-39)
bindings/python/tools/python/ostk/astrodynamics/dataframe.py (4)
bindings/python/test/test_dataframe.py (7)
frame(48-49)frame(1168-1169)states_1(1200-1201)states_2(1204-1205)state_1(1188-1191)state_2(1194-1197)orbit_states(89-90)bindings/python/test/trajectory/state/test_numerical_solver.py (2)
frame(50-51)coordinate_subsets(33-34)bindings/python/tools/python/ostk/astrodynamics/converters.py (1)
coerce_to_datetime(14-39)test/OpenSpaceToolkit/Astrodynamics/Trajectory/Orbit/Model/Tabulated.test.cpp (1)
i(48-58)
🪛 GitHub Actions: Build and Test
bindings/python/test/test_dataframe.py
[error] 1262-1262: compare_states_to_states_different_states: dr_x residual sign mismatch. Expected +100.0, got -100.0.
[error] 1352-1352: compare_orbit_to_states_datetime_ordering: timestamp mismatch due to timezone awareness. Expected naive UTC datetime, got aware datetime with UTC tz.
bindings/python/tools/python/ostk/astrodynamics/dataframe.py
[error] 1-1: Python formatting check failed. 1 file would be reformatted (dataframe.py). Run the formatter (e.g., 'make format' / the project's formatter) to fix.
🪛 Ruff (0.14.3)
bindings/python/tools/python/ostk/astrodynamics/dataframe.py
636-636: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
685-685: Local variable datetimes is assigned to but never used
Remove assignment to unused variable datetimes
(F841)
🔇 Additional comments (7)
bindings/python/tools/python/ostk/astrodynamics/dataframe.py (2)
5-5: LGTM! New imports support the comparison utilities.The
datetimeandScaleimports are properly used in the new comparison functions for timestamp handling and UTC conversion.Also applies to: 16-16
603-609: LGTM! Shared state builder improves code reuse.The module-level
BASE_STATE_BUILDERconfigured for GCRF frame with Cartesian position/velocity subsets is appropriate for the comparison utilities.bindings/python/test/test_dataframe.py (5)
29-29: LGTM! Imports support the new test class.The new imports for
Interpolator,datetime, and the comparison functions are properly used inTestCompareUtilities.Also applies to: 31-31, 37-39
1163-1230: LGTM! Test fixtures are well-structured.The fixtures provide comprehensive test data with a deliberate 100m position difference in the x-direction for testing residual calculations.
1232-1248: LGTM! Test correctly validates zero residuals for identical states.The test uses appropriate tolerances for floating-point comparisons.
1267-1287: LGTM! Test thoroughly validates the return structure.The test correctly checks that the function returns a list of dictionaries with all expected keys and correct types.
1289-1332: LGTM! Tests validate orbit comparison functionality.The tests correctly verify the return structure and validate that comparing an orbit to its own states yields near-zero residuals (accounting for interpolation).
| def test_compare_states_to_states_different_states( | ||
| self, states_1: list[State], states_2: list[State] | ||
| ) -> None: | ||
| """Test comparing different states returns non-zero residuals.""" | ||
| residuals = compare_states_to_states(states_1, states_2) | ||
|
|
||
| assert len(residuals) == 3 | ||
| # Expected difference is 100m in x-direction | ||
| expected_residual = 100.0 | ||
| for residual in residuals: | ||
| assert isinstance(residual, dict) | ||
| assert residual["dr"] == pytest.approx(expected_residual, rel=1e-6) | ||
| assert residual["dr_x"] == pytest.approx(expected_residual, rel=1e-6) | ||
| assert residual["dr_y"] == pytest.approx(0.0, abs=1e-10) | ||
| assert residual["dr_z"] == pytest.approx(0.0, abs=1e-10) | ||
| assert residual["dv"] == pytest.approx(0.0, abs=1e-10) |
There was a problem hiding this comment.
Fix the expected residual sign.
The test expects dr_x = +100.0, but the actual result is -100.0. Given that position_1.x = 7000000.0 and position_2.x = 7000100.0, the residual state_1 - state_2 correctly yields -100.0 for the x-component.
Apply this diff:
assert len(residuals) == 3
- # Expected difference is 100m in x-direction
- expected_residual = 100.0
+ # Expected difference is -100m in x-direction (state_1 - state_2)
+ expected_residual_x = -100.0
for residual in residuals:
assert isinstance(residual, dict)
- assert residual["dr"] == pytest.approx(expected_residual, rel=1e-6)
- assert residual["dr_x"] == pytest.approx(expected_residual, rel=1e-6)
+ assert residual["dr"] == pytest.approx(100.0, rel=1e-6) # L2 norm is always positive
+ assert residual["dr_x"] == pytest.approx(expected_residual_x, rel=1e-6)
assert residual["dr_y"] == pytest.approx(0.0, abs=1e-10)
assert residual["dr_z"] == pytest.approx(0.0, abs=1e-10)
assert residual["dv"] == pytest.approx(0.0, abs=1e-10)📝 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.
| def test_compare_states_to_states_different_states( | |
| self, states_1: list[State], states_2: list[State] | |
| ) -> None: | |
| """Test comparing different states returns non-zero residuals.""" | |
| residuals = compare_states_to_states(states_1, states_2) | |
| assert len(residuals) == 3 | |
| # Expected difference is 100m in x-direction | |
| expected_residual = 100.0 | |
| for residual in residuals: | |
| assert isinstance(residual, dict) | |
| assert residual["dr"] == pytest.approx(expected_residual, rel=1e-6) | |
| assert residual["dr_x"] == pytest.approx(expected_residual, rel=1e-6) | |
| assert residual["dr_y"] == pytest.approx(0.0, abs=1e-10) | |
| assert residual["dr_z"] == pytest.approx(0.0, abs=1e-10) | |
| assert residual["dv"] == pytest.approx(0.0, abs=1e-10) | |
| def test_compare_states_to_states_different_states( | |
| self, states_1: list[State], states_2: list[State] | |
| ) -> None: | |
| """Test comparing different states returns non-zero residuals.""" | |
| residuals = compare_states_to_states(states_1, states_2) | |
| assert len(residuals) == 3 | |
| # Expected difference is -100m in x-direction (state_1 - state_2) | |
| expected_residual_x = -100.0 | |
| for residual in residuals: | |
| assert isinstance(residual, dict) | |
| assert residual["dr"] == pytest.approx(100.0, rel=1e-6) # L2 norm is always positive | |
| assert residual["dr_x"] == pytest.approx(expected_residual_x, rel=1e-6) | |
| assert residual["dr_y"] == pytest.approx(0.0, abs=1e-10) | |
| assert residual["dr_z"] == pytest.approx(0.0, abs=1e-10) | |
| assert residual["dv"] == pytest.approx(0.0, abs=1e-10) |
🧰 Tools
🪛 GitHub Actions: Build and Test
[error] 1262-1262: compare_states_to_states_different_states: dr_x residual sign mismatch. Expected +100.0, got -100.0.
🤖 Prompt for AI Agents
In bindings/python/test/test_dataframe.py around lines 1250 to 1265, the test
asserts the wrong sign for the x residual: it expects +100.0 but
compare_states_to_states computes state_1 - state_2 yielding -100.0; change
expected_residual to -100.0 and use that for the dr and dr_x assertions (keep
dr_y/dr_z/dv checks at 0.0). Ensure the test reflects the subtraction order by
updating the expected_residual value used in the pytest.approx checks.
| def test_compare_orbit_to_states_datetime_ordering( | ||
| self, orbit_dataframe: pd.DataFrame, instant: Instant | ||
| ) -> None: | ||
| """Test that timestamps in result match the input states.""" | ||
| orbit = generate_orbit_from_dataframe( | ||
| orbit_dataframe, interpolation_type=Interpolator.Type.Linear | ||
| ) | ||
| states = generate_states_from_dataframe(orbit_dataframe) | ||
|
|
||
| result = compare_orbit_to_states(orbit, states) | ||
|
|
||
| expected_datetimes = [ | ||
| instant.get_date_time(Scale.UTC), | ||
| (instant + Duration.seconds(60.0)).get_date_time(Scale.UTC), | ||
| ] | ||
|
|
||
| for i, entry in enumerate(result): | ||
| # Both are Python datetime objects, direct comparison should work | ||
| assert entry["timestamp"] == expected_datetimes[i] |
There was a problem hiding this comment.
Fix timezone awareness mismatch in datetime comparison.
The test fails because entry["timestamp"] is timezone-aware (from coerce_to_datetime) while expected_datetimes contains naive UTC datetimes (from instant.get_date_time(Scale.UTC)).
Apply this diff:
+from datetime import timezone
+
...
expected_datetimes = [
- instant.get_date_time(Scale.UTC),
- (instant + Duration.seconds(60.0)).get_date_time(Scale.UTC),
+ instant.get_date_time(Scale.UTC).replace(tzinfo=timezone.utc),
+ (instant + Duration.seconds(60.0)).get_date_time(Scale.UTC).replace(tzinfo=timezone.utc),
]
for i, entry in enumerate(result):
- # Both are Python datetime objects, direct comparison should work
assert entry["timestamp"] == expected_datetimes[i]Note: Add from datetime import timezone at the top of the file if not already present.
🧰 Tools
🪛 GitHub Actions: Build and Test
[error] 1352-1352: compare_orbit_to_states_datetime_ordering: timestamp mismatch due to timezone awareness. Expected naive UTC datetime, got aware datetime with UTC tz.
🤖 Prompt for AI Agents
In bindings/python/test/test_dataframe.py around lines 1334 to 1352, the test
compares a timezone-aware timestamp (entry["timestamp"]) with naive UTC
datetimes from instant.get_date_time(Scale.UTC), causing failures; convert the
expected_datetimes to timezone-aware UTC by replacing or wrapping those
datetimes with timezone.utc (e.g., call .replace(tzinfo=timezone.utc) or
otherwise attach datetime.timezone.utc) and add "from datetime import timezone"
at the top of the file if it's not already imported so the comparison uses
matching timezone-aware datetimes.
| - 'dv_y': Velocity residual y-component in meters/second | ||
| - 'dv_z': Velocity residual z-component in meters/second | ||
| """ | ||
| residuals: list[float] = [] |
There was a problem hiding this comment.
Fix the type hint.
The variable is initialized with type list[float] but should be list[dict[str, datetime | float]] to match the actual content.
Apply this diff:
- residuals: list[float] = []
+ residuals: list[dict[str, datetime | float]] = []📝 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.
| residuals: list[float] = [] | |
| residuals: list[dict[str, datetime | float]] = [] |
🤖 Prompt for AI Agents
In bindings/python/tools/python/ostk/astrodynamics/dataframe.py around line 634,
the residuals variable is annotated as list[float] but actually holds entries of
type dict[str, datetime | float]; change the type hint to list[dict[str,
datetime | float]] and ensure the datetime symbol is imported or referenced
correctly so the annotation resolves (use from datetime import datetime or
fully-qualified datetime.datetime as appropriate).
| - 'dv_z': Velocity residual z-component in meters/second | ||
| """ | ||
| instants: list[Instant] = [s.get_instant() for s in states] | ||
| datetimes: list[datetime] = [i.get_date_time(Scale.UTC) for i in instants] |
There was a problem hiding this comment.
Remove unused variable.
The datetimes variable is assigned but never used.
Apply this diff:
instants: list[Instant] = [s.get_instant() for s in states]
- datetimes: list[datetime] = [i.get_date_time(Scale.UTC) for i in instants]
orbit_states: list[State] = orbit.get_states_at(instants)🧰 Tools
🪛 Ruff (0.14.3)
685-685: Local variable datetimes is assigned to but never used
Remove assignment to unused variable datetimes
(F841)
🤖 Prompt for AI Agents
In bindings/python/tools/python/ostk/astrodynamics/dataframe.py around line 685,
the variable `datetimes` is assigned from a list comprehension but never used;
remove that unused assignment line (or if the datetimes are intended to be used
later, replace the unused variable with the actual use). Simply delete the
`datetimes: list[datetime] = [i.get_date_time(Scale.UTC) for i in instants]`
line to eliminate the dead code.
| } | ||
|
|
||
|
|
||
| BASE_STATE_BUILDER: StateBuilder = StateBuilder( |
There was a problem hiding this comment.
should this go at the top with the other "constants" ?
| Returns: | ||
| list[dict[str, datetime | float]]: List of dictionaries containing: | ||
| - 'timestamp': UTC datetime of the state (Python datetime object) | ||
| - 'dr': Position residual in meters (L2 norm of position difference) |
There was a problem hiding this comment.
is it really necessary to specify L2 norm here? Do we ever use any other type of norm?
| { | ||
| "timestamp": coerce_to_datetime(d_state.get_instant()), | ||
| "dr": float(np.linalg.norm(dr)), | ||
| "dr_x": dr[0], |
There was a problem hiding this comment.
I think these also need to be coerced to non-numpy floats
| residuals: list[float] = [] | ||
|
|
||
| for state_1, state_2 in zip(states_1, states_2): | ||
| d_state: State = ( |
There was a problem hiding this comment.
let's beef up/clarify the naming here - states_1/states_2 should probably be clearer in which is the reference and which isn't, and d_state probably isn't very clear imo
| def test_compare_states_to_states_identical_states( | ||
| self, states_1: list[State] | ||
| ) -> None: | ||
| """Test comparing identical states returns zero residuals.""" |
There was a problem hiding this comment.
not needed
| """Test comparing identical states returns zero residuals.""" |
| def test_compare_states_to_states_different_states( | ||
| self, states_1: list[State], states_2: list[State] | ||
| ) -> None: | ||
| """Test comparing different states returns non-zero residuals.""" |
There was a problem hiding this comment.
ditto
| """Test comparing different states returns non-zero residuals.""" |
| """ | ||
| residuals: list[float] = [] | ||
|
|
||
| for state_1, state_2 in zip(states_1, states_2): |
There was a problem hiding this comment.
| for state_1, state_2 in zip(states_1, states_2): | |
| for state_1, state_2 in zip(states_1, states_2, strict=True): |
Summary by CodeRabbit
New Features
Tests