feat: add Close Approach module #593
Conversation
WalkthroughAdds a CloseApproach value type and a close‑approach Generator with C++ headers/implementations, pybind11 Python bindings, and C++/Python tests for construction, accessors, miss‑distance computations, and trajectory-based close‑approach search. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Generator
participant TemporalConditionSolver
participant StateBuilder
participant CloseApproach
User->>Generator: computeCloseApproaches(otherTrajectory, searchInterval)
activate Generator
Generator->>TemporalConditionSolver: configure(step, tolerance, cond=relativeDistanceDerivative>0)
activate TemporalConditionSolver
TemporalConditionSolver-->>Generator: moving-apart intervals
deactivate TemporalConditionSolver
loop for each intervalStart (except searchInterval.start)
Generator->>StateBuilder: reduce states to reference frame (pos+vel)
StateBuilder-->>Generator: reduced states
Generator->>CloseApproach: construct CloseApproach(stateRef, stateOther)
activate CloseApproach
CloseApproach->>StateBuilder: compute relative state & miss distance (frame/local orbital)
StateBuilder-->>CloseApproach: components
CloseApproach-->>Generator: CloseApproach instance
deactivate CloseApproach
end
Generator-->>User: sorted list of CloseApproach
deactivate Generator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp
Outdated
Show resolved
Hide resolved
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp
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: 6
🧹 Nitpick comments (17)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (1)
95-115: Avoid unnecessary copies; use const references and noexcept/nodiscard for trivial getters.
Statemembers are stored by value; getters can returnconst&to avoid copies, and trivial getters can benoexcept/[[nodiscard]].Example header tweaks:
- State getObject1State() const; - State getObject2State() const; - Instant getInstant() const; - Length getMissDistance() const; + [[nodiscard]] const State& getObject1State() const noexcept; + [[nodiscard]] const State& getObject2State() const noexcept; + [[nodiscard]] Instant getInstant() const; // keep by value if computed elsewhere + [[nodiscard]] Length getMissDistance() const; // likely computed; keep by valueAlso consider
[[nodiscard]] bool isDefined() const noexcept;.Also applies to: 115-125
bindings/python/test/conjunction/close_approach/test_close_approach.py (2)
42-61: Remove unused fixture argumentgcrf_frame.
gcrf_frameis not used inobject_1_state; drop it to satisfy Ruff ARG001 and reduce noise.-@pytest.fixture -def object_1_state(instant: Instant, gcrf_frame: Frame, earth: Celestial) -> State: +@pytest.fixture +def object_1_state(instant: Instant, earth: Celestial) -> State:
63-82: Remove unused fixture argumentgcrf_frame.Same for
object_2_state.-@pytest.fixture -def object_2_state(instant: Instant, gcrf_frame: Frame, earth: Celestial) -> State: +@pytest.fixture +def object_2_state(instant: Instant, earth: Celestial) -> State:bindings/python/test/conjunction/close_approach/test_generator.py (2)
201-204: Reduce flakiness: don’t assume >1 close approaches in 3 hours.Cardinality depends on orbital geometry and step/tolerance;
> 1is brittle.- assert len(close_approaches) > 1 + assert len(close_approaches) >= 1
210-227: Assert empty result for an effectively empty interval.The test only checks type; also assert the list is empty to validate behavior.
assert close_approaches is not None assert isinstance(close_approaches, list) + assert len(close_approaches) == 0bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/Generator.cpp (1)
18-43: Generator binding surface matches C++ API and looks correct.Constructor defaults, getters/setters, and
compute_close_approachesare wired properly. Nice docstrings.You can drop the unused
using ostk::core::container::Array;to silence potential warnings.Also applies to: 89-141
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.test.cpp (2)
463-470: Strengthen LOF validation by tying back to getMissDistance().Instead of recomputing the expected norm, assert equality with closeApproach.getMissDistance() to validate consistency across APIs and reduce duplication.
Apply this tweak:
- const double expectedMissDistance = std::sqrt(7.0e6 * 7.0e6 + 7.0e6 * 7.0e6); - - EXPECT_NEAR(std::sqrt(normSquared), expectedMissDistance, 1e-3); + const double miss = closeApproach.getMissDistance().inMeters(); + EXPECT_NEAR(std::sqrt(normSquared), miss, 1e-3);
420-438: Optional: add a non-inertial/inertial frame mix case.Add a case computing components in ITRF (or another non-inertial frame) to exercise frame conversions and sign conventions.
Would you like me to draft the extra test case?
test/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.test.cpp (2)
346-361: Also assert monotonic ordering of results.Results should be time-sorted; add an explicit monotonicity check to guard regressions.
Example:
EXPECT_EQ(4, closeApproaches.getSize()); @@ EXPECT_LT((closeApproaches[3].getInstant() - (epoch + 2.0 * orbitalPeriod)).getAbsolute(), tolerance); + EXPECT_TRUE( + closeApproaches[0].getInstant() < closeApproaches[1].getInstant() && + closeApproaches[1].getInstant() < closeApproaches[2].getInstant() && + closeApproaches[2].getInstant() < closeApproaches[3].getInstant() + );
422-444: Edge case: derivative = 0 at TCA.Consider a parametric case where the relative-distance derivative is exactly zero over a small plateau at TCA to ensure the solver still returns one event.
I can add a synthetic COE pair to exercise this if helpful.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (1)
169-178: Add C++-parity alias for constructor: Undefined vs undefined.Expose both "Undefined" (parity with C++) and "undefined" (Pythonic) to avoid surprises.
Apply this diff:
.def_static( - "undefined", + "undefined", &CloseApproach::Undefined, R"doc( Construct an undefined close approach. @@ )doc" ) + .def_static( + "Undefined", + &CloseApproach::Undefined, + R"doc( + Construct an undefined close approach (C++-style alias). + )doc" + )src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (2)
175-195: Prefer using getRelativeState() for LOF components.Current code relies on LOF origin semantics. Using getRelativeState() makes intent explicit and robust if LOF origin behavior changes.
Apply this diff:
- const Shared<const Frame> localOrbitalFrame = aLocalOrbitalFrameFactorySPtr->generateFrame(object1State_); - const State relativeStateInFrame = object2State_.inFrame(localOrbitalFrame); + const Shared<const Frame> localOrbitalFrame = aLocalOrbitalFrameFactorySPtr->generateFrame(object1State_); + const State relativeStateInFrame = this->getRelativeState().inFrame(localOrbitalFrame);Optionally extract a small helper to DRY coordinate extraction across both component methods.
72-91: Stream output: include frame name for clarity.Including the frame of object1/object2 in the stream can aid debugging.
Example addition:
ostk::core::utils::Print::Line(anOutputStream) << "Object 1 State:" << (aCloseApproach.object1State_.isDefined() ? "Defined" : "Undefined"); + if (aCloseApproach.object1State_.isDefined()) { + ostk::core::utils::Print::Line(anOutputStream) + << "Object 1 Frame:" << aCloseApproach.object1State_.accessFrame()->getName(); + }src/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.cpp (2)
157-175: Validate positive durations on setters.Guard against non-positive step size/tolerance to avoid solver misbehavior.
Example:
- if (!aSearchStepSize.isDefined()) + if (!aSearchStepSize.isDefined() || (aSearchStepSize <= Duration::Zero())) ... - if (!aSearchTolerance.isDefined()) + if (!aSearchTolerance.isDefined() || (aSearchTolerance <= Duration::Zero())) ...Please confirm Duration has comparison operators; otherwise I can adjust the check.
144-155: Optional: de-duplicate close approaches if solver returns adjacent intervals.A stable unique pass after sorting can avoid duplicates at the same instant.
Example:
std::sort(...); - return closeApproaches; + closeApproaches.erase( + std::unique( + closeApproaches.begin(), closeApproaches.end(), + [](const CloseApproach& a, const CloseApproach& b) { return a.getInstant() == b.getInstant(); } + ), + closeApproaches.end() + ); + return closeApproaches;include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp (2)
37-38: Prefer constexpr variables over macros for type safety.Using
#definefor constants lacks type information and proper scoping. Modern C++ offersconstexprorinline constexpras safer alternatives.Apply this diff to replace the macros:
-#define DEFAULT_STEP_SIZE Duration::Seconds(15.0) -#define DEFAULT_TOLERANCE Duration::Milliseconds(1.0) +inline constexpr Duration DEFAULT_STEP_SIZE = Duration::Seconds(15.0); +inline constexpr Duration DEFAULT_TOLERANCE = Duration::Milliseconds(1.0);
76-84: Consider returning by const reference for better performance.Returning
Trajectoryby value may incur unnecessary copying overhead. For a getter accessing a member variable, returning by const reference is more efficient.If the implementation allows, update the signature to:
const Trajectory& getReferenceTrajectory() const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/Generator.cpp(1 hunks)bindings/python/test/conjunction/close_approach/test_close_approach.py(1 hunks)bindings/python/test/conjunction/close_approach/test_generator.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.test.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.test.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (2)
CloseApproach(50-50)aFrame(155-155)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (3)
CloseApproach(35-55)Undefined(197-200)Undefined(197-197)
bindings/python/test/conjunction/close_approach/test_generator.py (4)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (1)
CloseApproach(35-55)include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp (1)
Generator(60-64)src/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.cpp (1)
Generator(47-54)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (1)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (6)
aCloseApproach(57-65)aCloseApproach(57-57)aCloseApproach(67-70)aCloseApproach(67-67)anOutputStream(72-91)anOutputStream(72-72)
bindings/python/test/conjunction/close_approach/test_close_approach.py (3)
bindings/python/test/conjunction/close_approach/test_generator.py (3)
environment(24-25)earth(29-30)epoch(34-35)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (1)
CloseApproach(35-55)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp (2)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (2)
OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach(5-181)OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach(5-5)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/Generator.cpp (2)
OpenSpaceToolkitAstrodynamicsPy_Conjunction_Generator(5-142)OpenSpaceToolkitAstrodynamicsPy_Conjunction_Generator(5-5)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (1)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (6)
CloseApproach(50-50)aCloseApproach(61-61)aCloseApproach(72-72)anOutputStream(83-83)aFrame(155-155)aLocalOrbitalFrameFactorySPtr(168-170)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.test.cpp (4)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (2)
CloseApproach(50-50)aFrame(155-155)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (3)
CloseApproach(35-55)Undefined(197-200)Undefined(197-197)include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp (1)
Generator(60-64)src/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.cpp (3)
Generator(47-54)Undefined(177-180)Undefined(177-177)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (19)
CloseApproach(35-55)isDefined(93-96)isDefined(93-93)getObject1State(98-106)getObject1State(98-98)getObject2State(108-116)getObject2State(108-108)getInstant(118-126)getInstant(118-118)getMissDistance(128-140)getMissDistance(128-128)getRelativeState(142-153)getRelativeState(142-142)computeMissDistanceComponentsInFrame(155-173)computeMissDistanceComponentsInFrame(155-156)computeMissDistanceComponentsInLocalOrbitalFrame(175-195)computeMissDistanceComponentsInLocalOrbitalFrame(175-177)Undefined(197-200)Undefined(197-197)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp (2)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (1)
CloseApproach(35-55)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/Generator.cpp (4)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (5)
CloseApproach(35-55)isDefined(93-96)isDefined(93-93)Undefined(197-200)Undefined(197-197)include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp (1)
Generator(60-64)src/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.cpp (17)
Generator(47-54)isDefined(56-59)isDefined(56-56)getReferenceTrajectory(61-69)getReferenceTrajectory(61-61)getSearchStepSize(71-79)getSearchStepSize(71-71)getSearchTolerance(81-89)getSearchTolerance(81-81)computeCloseApproaches(91-155)computeCloseApproaches(91-93)setSearchStepSize(157-165)setSearchStepSize(157-157)setSearchTolerance(167-175)setSearchTolerance(167-167)Undefined(177-180)Undefined(177-177)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp (5)
Generator(60-64)aSearchStepSize(131-131)aSearchTolerance(141-141)anOtherObjectTrajectory(119-121)aState1(162-162)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp (5)
isDefined(93-96)isDefined(93-93)Undefined(197-200)Undefined(197-197)CloseApproach(35-55)
🪛 Clang (14.0.6)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Tuple.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp
[error] 3-3: 'OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp' file not found
(clang-diagnostic-error)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/Generator.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.cpp
[error] 3-3: 'algorithm' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.1)
bindings/python/test/conjunction/close_approach/test_close_approach.py
43-43: Unused function argument: gcrf_frame
(ARG001)
64-64: Unused function argument: gcrf_frame
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Development Image / Merge Image Manifests
🔇 Additional comments (8)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp (2)
13-15: Registration order looks good.Registering CloseApproach before Generator helps ensure converters exist when returning
list[CloseApproach]. LGTM.Note: Tests currently import from
ostk.astrodynamics.conjunction.close_approach. Either change tests to import fromostk.astrodynamics.conjunction, or create a nested submoduleclose_approachand register classes there.
3-4: No action required; include path is correctly configured.The include directories are already properly set in
bindings/python/CMakeLists.txtviaTARGET_INCLUDE_DIRECTORIES (${NAME} PUBLIC "${PROJECT_SOURCE_DIR}/src"), which resolves tobindings/python/src. Both referenced.cppfiles exist at the expected locations. If a Clang file-not-found error was encountered, it was likely a build cache issue; regenerate the build.Likely an incorrect or invalid review comment.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/Generator.cpp (1)
3-3: Manual verification required — unable to locate files in repository.All search attempts returned no results: the header file
Generator.hppcould not be found, CMakeLists.txt include configurations could not be located, and the binding source file itself could not be found at the expected path. This prevents verification of whether the include path issue is real.Please confirm:
- The repository structure matches expectations (bindings/python/src/ directory exists)
- The header file exists in the expected location or document its actual path
- CMake include directories are properly configured
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.hpp (1)
6-13: Ignore this review comment—the include is correct and properly configured.The
#include <OpenSpaceToolkit/Core/Container/Tuple.hpp>include is valid. This file is from the external dependencyOpenSpaceToolkitCore(version 5), which is properly declared in CMakeLists.txt viaFIND_PACKAGE ("OpenSpaceToolkitCore" "5" REQUIRED)and linked withTARGET_LINK_LIBRARIES. The same include path is used consistently throughout the codebase in 16+ files with no issues. The static analysis tool's failure to find it is an environment/build configuration issue (missing include paths), not a code problem.Likely an incorrect or invalid review comment.
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/CloseApproach.test.cpp (1)
343-372: LGTM: relative state checks are precise and meaningful.Good verification of instant, frame, coordinate extraction, and expected relative components.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (1)
139-152: Confirm Tuple<Length,Length,Length> caster is available.The return type uses ostk::core::container::Tuple. Ensure the tuple caster is registered so Python gets a native tuple of Length objects.
If not, I can wire the pybind11 type_caster for ostk::core::container::Tuple.
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp (2)
60-141: Well-designed interface with good const correctness.The class interface follows solid design principles: clear method names, appropriate use of const methods, proper parameter passing by const reference, and comprehensive documentation.
157-162: LGTM: Well-documented helper method.The private static helper has a clear signature and comprehensive documentation. The static nature is appropriate for a pure computation function.
bindings/python/test/conjunction/close_approach/test_close_approach.py
Outdated
Show resolved
Hide resolved
bindings/python/test/conjunction/close_approach/test_generator.py
Outdated
Show resolved
Hide resolved
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp
Show resolved
Hide resolved
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/Generator.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/Generator.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/Generator.cpp
Outdated
Show resolved
Hide resolved
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp
Outdated
Show resolved
Hide resolved
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp
Outdated
Show resolved
Hide resolved
include/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.hpp
Outdated
Show resolved
Hide resolved
src/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.cpp
Outdated
Show resolved
Hide resolved
src/OpenSpaceToolkit/Astrodynamics/Conjunction/Generator/Generator.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp (1)
106-128: Setters policy: confirm exposure vs. immutability style.A prior review suggested dropping setters in Python for trivial types and encouraging reconstruction. Given C++ exposes setStep/setTolerance and tests use them, parity is reasonable; otherwise, consider removing these from Python for a more immutable style. Please confirm project convention and keep both languages consistent.
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
6-16: Include (or ) for std::ostream declarations.This header declares friend operator<< and print(std::ostream&, …) but doesn’t include a forward declaration for std::ostream. Add (minimal) or to avoid order-dependent build breaks.
#ifndef __OpenSpaceToolkit_Astrodynamics_Conjunction_CloseApproach__ #define __OpenSpaceToolkit_Astrodynamics_Conjunction_CloseApproach__ +#include <iosfwd> #include <OpenSpaceToolkit/Core/Container/Tuple.hpp> #include <OpenSpaceToolkit/Core/Type/Shared.hpp>
🧹 Nitpick comments (11)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
115-121: Spell out preconditions and edge‑case behavior for computeCloseApproaches.Please document and enforce:
- Both trajectories and aSearchInterval must be defined.
- aSearchInterval must be finite and non‑empty (start < end).
- Returned Array is time‑sorted and contains unique events (no duplicates on boundary crossings).
bindings/python/test/conjunction/test_close_approach.py (1)
174-190: Optional: Assert component units more tightly.Consider asserting component magnitudes are finite and that sqrt(sum^2) equals get_miss_distance() within tolerance for an end‑to‑end cross‑check.
bindings/python/test/conjunction/close_approach/test_generator.py (1)
196-209: Optional: Validate ordering and bounds of events.Add:
- Strict time ordering (non‑decreasing instants).
- All instants within search_interval (already checked).
Example:
instants = [ca.get_instant() for ca in close_approaches] assert all(instants[i] <= instants[i+1] for i in range(len(instants)-1))test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (1)
317-321: Relax miss‑distance tolerance (reduce flakiness).1e‑6 m is unnecessarily tight for floating‑point ops; 1e‑3 m matches other tolerances used in this file and is safer.
- EXPECT_NEAR(missDistance.inMeters(), expectedMissDistance, 1e-6); + EXPECT_NEAR(missDistance.inMeters(), expectedMissDistance, 1e-3);bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp (1)
30-43: Docstring clarity: name defaults and intent explicitly.Minor wording tweak to reduce ambiguity and match API semantics (search “step size” and “root-finding tolerance”):
- step (Duration): The step to use during the close approach search. Defaults to Duration.seconds(15.0). - tolerance (Duration): The tolerance to use during the close approach search. Defaults to Duration.milliseconds(1.0). + step (Duration): Search step size for the temporal condition solver. Defaults to Duration.seconds(15.0). + tolerance (Duration): Root-finding tolerance for event localization. Defaults to Duration.milliseconds(1.0).test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp (1)
332-365: Edge-case test suggestion: boundary transition at search start.Add a case where objects transition from approaching→moving-apart exactly at search_interval.start to assert the implementation correctly skips that event (and uses tolerance-consistent logic). I can draft this test if helpful.
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (3)
128-141: Use tolerance-based boundary skip, not exact Instant equality.Exact equality risks spurious inclusions/exclusions due to solver discretization. Compare start-times within tolerance_ instead.
- if (interval.getStart() == aSearchInterval.getStart()) + if ((interval.getStart() - aSearchInterval.getStart()).getAbsolute() <= tolerance_) { continue; }
110-122: Be explicit about required headers for TemporalConditionSolver.This translation unit uses TemporalConditionSolver but doesn’t include its header. Please include the defining header here to avoid reliance on transitive includes (path per project layout).
208-214: Numerical robustness near co-location.Optional: treat very small ||r|| as co-location to avoid dot(r,v)/||r|| blowups.
- if (relativePositionNorm == 0.0) + if (relativePositionNorm <= 1e-6) // meters; tune or make configurable { throw ostk::core::error::RuntimeError("Cannot compute relative distance derivative: states are co-located."); }Alternatively, keep strict zero but clamp the denominator when |r|→0.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (1)
141-169: Return-type docstrings: clarify component ordering and frames.Minor doc nits to state explicit component order and reference frame assumptions:
- tuple[Length, Length, Length]: The miss distance components (x, y, z). + tuple[Length, Length, Length]: Miss distance components in the provided frame, ordered (x, y, z).- tuple[Length, Length, Length]: The miss distance components (radial, in-track, cross-track or similar depending on the factory). + tuple[Length, Length, Length]: Miss distance components ordered per the local orbital frame definition (e.g., R/S/W for QSW).src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (1)
183-200: Consider simplifying ternary-with-void pattern.Lines 185 and 199 use
condition ? action : void();which is less readable than a simple if-statement. While functional, this pattern is unconventional.Consider this refactor for improved readability:
- displayDecorator ? ostk::core::utils::Print::Header(anOutputStream, "Close Approach") : void(); + if (displayDecorator) + { + ostk::core::utils::Print::Header(anOutputStream, "Close Approach"); + }And similarly for line 199. However, if this is a project-wide pattern, you may safely ignore this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp(1 hunks)bindings/python/test/conjunction/close_approach/test_generator.py(1 hunks)bindings/python/test/conjunction/test_close_approach.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp (4)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (2)
CloseApproach(50-50)aFrame(155-155)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (3)
CloseApproach(35-55)Undefined(202-205)Undefined(202-202)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
Generator(59-63)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (3)
Generator(49-54)Undefined(175-178)Undefined(175-175)
bindings/python/test/conjunction/close_approach/test_generator.py (5)
bindings/python/test/conjunction/test_close_approach.py (3)
environment(23-24)close_approach(85-92)earth(28-29)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (1)
CloseApproach(35-55)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
Generator(59-63)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (1)
Generator(49-54)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
Generator(59-63)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (17)
Generator(49-54)isDefined(56-59)isDefined(56-56)getReferenceTrajectory(61-69)getReferenceTrajectory(61-61)getStep(71-79)getStep(71-71)getTolerance(81-89)getTolerance(81-81)computeCloseApproaches(91-153)computeCloseApproaches(91-93)setStep(155-163)setStep(155-155)setTolerance(165-173)setTolerance(165-165)Undefined(175-178)Undefined(175-175)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (2)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (1)
CloseApproach(35-55)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (6)
aCloseApproach(57-65)aCloseApproach(57-57)aCloseApproach(67-70)aCloseApproach(67-67)anOutputStream(72-77)anOutputStream(72-72)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (2)
CloseApproach(50-50)aFrame(155-155)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (3)
CloseApproach(35-55)Undefined(202-205)Undefined(202-202)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp (4)
catch(199-202)catch(226-229)catch(253-256)catch(287-290)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (19)
CloseApproach(35-55)isDefined(79-82)isDefined(79-79)getObject1State(84-92)getObject1State(84-84)getObject2State(94-102)getObject2State(94-94)getInstant(104-112)getInstant(104-104)getMissDistance(114-126)getMissDistance(114-114)getRelativeState(128-139)getRelativeState(128-128)computeMissDistanceComponentsInFrame(141-159)computeMissDistanceComponentsInFrame(141-142)computeMissDistanceComponentsInLocalOrbitalFrame(161-181)computeMissDistanceComponentsInLocalOrbitalFrame(161-163)Undefined(202-205)Undefined(202-202)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp (2)
OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach_Generator(5-142)OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach_Generator(5-5)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (5)
Generator(59-63)aStep(130-130)aTolerance(140-140)anOtherObjectTrajectory(118-120)aState1(161-161)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (5)
isDefined(79-82)isDefined(79-79)Undefined(202-205)Undefined(202-202)CloseApproach(35-55)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (2)
anInstant(49-67)anInstant(49-54)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (1)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (7)
CloseApproach(50-50)aCloseApproach(61-61)aCloseApproach(72-72)anOutputStream(83-83)anOutputStream(177-177)aFrame(155-155)aLocalOrbitalFrameFactorySPtr(168-170)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp (1)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (2)
OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach(7-189)OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach(7-7)
bindings/python/test/conjunction/test_close_approach.py (3)
bindings/python/test/conjunction/close_approach/test_generator.py (3)
environment(24-25)earth(29-30)epoch(34-35)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (1)
CloseApproach(35-55)
🪛 Clang (14.0.6)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp' file not found
(clang-diagnostic-error)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Tuple.hpp' file not found
(clang-diagnostic-error)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp
[error] 3-3: 'algorithm' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp
[error] 3-3: 'OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.1)
bindings/python/test/conjunction/test_close_approach.py
43-43: Unused function argument: gcrf_frame
(ARG001)
64-64: Unused function argument: gcrf_frame
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Development Image / Merge Image Manifests
🔇 Additional comments (11)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp (2)
12-13: Registration order looks good.Conjunction submodule is created, then CloseApproach is registered before Message. No issues here.
3-3: No changes needed—file exists and include path is correct.The static analysis warning was a false positive. The file
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cppexists, the include path is valid, and the registration function is properly defined and called. The include pattern matches other similar, working includes likeMessage.cpp. The code is correct.test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (2)
336-372: LGTM: Relative state computation checks are solid.Good validation of frame coercion, coordinate extraction, and expected components.
3-3: No code changes needed—the include path is valid and already in use elsewhere.The include
#include <OpenSpaceToolkit/Core/Container/Array.hpp>is correct and follows the established pattern. OpenSpaceToolkitCore is already configured as a required dependency in CMakeLists.txt, and the same include path is already used throughout the codebase in validation files. The CI warning is a build environment issue, not a code defect.Likely an incorrect or invalid review comment.
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
6-8: The include path is correct and consistent across the codebase—verify CI toolchain configuration.The path
<OpenSpaceToolkit/Core/Container/Array.hpp>is used consistently throughout the codebase (Access.hpp, Dynamics.hpp, CDM.hpp, etc.) and correctly references the external OpenSpaceToolkitCore v5 library (defined in CMakeLists.txt line 319 viaFIND_PACKAGE). The code change itself is not at fault.The Clang warning indicates a toolchain/CI configuration issue where the Core library headers are not found in the compiler's include search path—not an error in this pull request. Verify that your CI environment has the OpenSpaceToolkitCore development headers installed and properly configured.
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (6)
35-55: LGTM: Well-structured constructor with appropriate validation.The constructor properly validates instant consistency between the two states and sets up the StateBuilder with the necessary coordinate subsets for relative state computation.
57-70: LGTM: Comparison operators follow best practices.The equality operator correctly handles undefined states and compares member states appropriately.
72-82: LGTM: Standard implementation patterns.Both the stream output operator and
isDefined()method follow standard conventions correctly.
84-112: LGTM: Consistent accessor implementations.All accessor methods properly guard against undefined states with appropriate error handling.
114-139: LGTM: Correct distance and relative state computations.Both methods correctly compute the miss distance and relative state with proper frame transformations and reduction logic.
202-205: LGTM: Standard undefined factory pattern.The static factory method correctly creates an undefined CloseApproach instance.
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp
Show resolved
Hide resolved
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (1)
49-56: Minor: Constructor assigns before validation.The constructor initializes member variables in the initializer list (lines 50-52), then immediately calls
setStepandsetTolerance(lines 54-55) which reassign the same values after validation. While this ensures validation occurs, it results in redundant assignments.Consider refactoring to validate first, then initialize once:
Generator::Generator(const Trajectory& aReferenceTrajectory, const Duration& aStep, const Duration& aTolerance) - : referenceTrajectory_(aReferenceTrajectory), - step_(aStep), - tolerance_(aTolerance) + : referenceTrajectory_(aReferenceTrajectory) { this->setStep(aStep); this->setTolerance(aTolerance); }This avoids the double assignment while maintaining validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/python/test/conjunction/close_approach/test_generator.py(1 hunks)bindings/python/test/conjunction/test_close_approach.py(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/python/test/conjunction/test_close_approach.py
🧰 Additional context used
🧬 Code graph analysis (4)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (5)
Generator(59-63)aStep(130-130)aTolerance(140-140)anOtherObjectTrajectory(118-120)aState1(161-161)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (5)
isDefined(79-82)isDefined(79-79)Undefined(202-205)Undefined(202-202)CloseApproach(35-55)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (2)
anInstant(49-67)anInstant(49-54)
bindings/python/test/conjunction/close_approach/test_generator.py (5)
bindings/python/test/conjunction/test_close_approach.py (3)
environment(23-24)close_approach(91-98)earth(28-29)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (1)
CloseApproach(35-55)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (1)
Generator(49-56)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
Generator(59-63)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (2)
CloseApproach(50-50)aFrame(155-155)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (3)
CloseApproach(35-55)Undefined(202-205)Undefined(202-202)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp (4)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (2)
CloseApproach(50-50)aFrame(155-155)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (3)
CloseApproach(35-55)Undefined(202-205)Undefined(202-202)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (3)
Generator(49-56)Undefined(187-190)Undefined(187-187)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
Generator(59-63)
🪛 Clang (14.0.6)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp
[error] 3-3: 'algorithm' file not found
(clang-diagnostic-error)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Development Image / Build Development Image (linux/arm64, ubuntu-24.04-arm)
🔇 Additional comments (7)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (2)
93-155: Excellent close-approach detection logic.The implementation correctly:
- Validates all inputs before processing
- Uses TemporalConditionSolver to find transition intervals where objects change from approaching to receding
- Skips the initial instant to avoid false positives when objects start moving apart
- Sorts results to ensure chronological ordering
The algorithm is sound and handles edge cases appropriately.
192-226: Robust relative distance derivative computation.The implementation correctly computes d(||r||)/dt = dot(r,v)/||r|| in the GCRF frame, with proper:
- State reduction to position/velocity components
- Frame transformation handling
- Co-location detection and error handling
The mathematical approach is correct and well-implemented.
bindings/python/test/conjunction/close_approach/test_generator.py (1)
1-228: Comprehensive test coverage for Python bindings.The test suite provides excellent coverage:
- Constructor variants (explicit params and defaults)
- Defined/undefined state checks
- All accessors (reference trajectory, step, tolerance)
- Mutators with validation
- Core functionality (compute_close_approaches)
- Edge cases (empty intervals, no results)
- Type validation and temporal containment checks
The fixtures are well-organized and the assertions are thorough.
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (1)
1-575: Excellent test coverage for CloseApproach class.The test suite is comprehensive and well-structured:
- Validates construction with consistent/inconsistent instants
- Tests all operators (==, !=, stream insertion)
- Covers all accessors with proper undefined handling
- Tests relative state computation with frame transformations (GCRF↔ITRF)
- Validates miss-distance calculations in both arbitrary and local orbital frames
- Includes proper numerical tolerances for floating-point comparisons
- Tests error propagation for undefined inputs
The numerical validation at lines 317-320 correctly computes the expected miss distance and verifies it within appropriate tolerance.
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp (3)
352-383: Excellent multi-approach validation.The test correctly validates the periodic nature of close approaches between orthogonal circular orbits:
- Sets up two circular orbits at 90° inclination
- Expects 4 close approaches over 2 orbital periods (2 per period)
- Validates timing against expected intervals (0.5T, 1.0T, 1.5T, 2.0T)
- Uses appropriate tolerance (1 ms) for temporal comparisons
This provides strong validation of the Generator's ability to detect multiple encounters.
386-479: Strong validation against real CDM data.The parameterized tests use real Conjunction Data Message (CDM) cases to validate:
- TCA (Time of Closest Approach) detection accuracy
- Miss-distance magnitude computation
- Local orbital frame (R, S, W) component decomposition
- Negative cases (no approaches before/after TCA window)
Using three distinct CDM scenarios with varying geometries and miss distances (270m, 280m, 1015m) provides excellent coverage of real-world conjunction scenarios.
481-496: Correct exception handling for co-located states.The test properly validates that co-located states (same trajectory for both objects) throw a RuntimeError with the expected message. The exception type and message verification are now correct.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #593 +/- ##
==========================================
+ Coverage 91.88% 91.98% +0.10%
==========================================
Files 110 113 +3
Lines 10517 10686 +169
==========================================
+ Hits 9664 9830 +166
- Misses 853 856 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
phc1990
force-pushed
the
feat/add-close-approach-module
branch
from
cb41296 to
caf5ac0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
6-11: Missing<iosfwd>include (previously flagged).The friend declaration at line 83 uses
std::ostream, but neither<iosfwd>nor<ostream>is included. This can break compilation if transitive includes change.Apply this diff:
#include <OpenSpaceToolkit/Core/Container/Tuple.hpp> #include <OpenSpaceToolkit/Core/Type/Shared.hpp> +#include <iosfwd> #include <OpenSpaceToolkit/Physics/Coordinate/Frame.hpp>
🧹 Nitpick comments (1)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (1)
49-56: Consider streamlining constructor initialization.The constructor initializes
step_andtolerance_in the member initializer list (lines 50-52), then immediately callssetStep(aStep)andsetTolerance(aTolerance)(lines 54-55), which validate and reassign the same values. While this ensures validation is centralized, it results in redundant assignments.Consider one of these approaches:
Option 1: Initialize with undefined values, then use setters:
Generator::Generator(const Trajectory& aReferenceTrajectory, const Duration& aStep, const Duration& aTolerance) : referenceTrajectory_(aReferenceTrajectory), - step_(aStep), - tolerance_(aTolerance) + step_(Duration::Undefined()), + tolerance_(Duration::Undefined()) { this->setStep(aStep); this->setTolerance(aTolerance); }Option 2: Extract validation logic to a private helper and use it in both constructor and setters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp(1 hunks)bindings/python/test/conjunction/close_approach/test_generator.py(1 hunks)bindings/python/test/conjunction/test_close_approach.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/python/test/conjunction/test_close_approach.py
🧰 Additional context used
🧬 Code graph analysis (10)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (2)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (1)
CloseApproach(35-55)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp (4)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (2)
CloseApproach(50-50)aFrame(155-155)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (3)
CloseApproach(35-55)Undefined(202-205)Undefined(202-202)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
Generator(59-63)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (3)
Generator(49-56)Undefined(186-189)Undefined(186-186)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (19)
CloseApproach(35-55)isDefined(79-82)isDefined(79-79)getObject1State(84-92)getObject1State(84-84)getObject2State(94-102)getObject2State(94-94)getInstant(104-112)getInstant(104-104)getMissDistance(114-126)getMissDistance(114-114)getRelativeState(128-139)getRelativeState(128-128)computeMissDistanceComponentsInFrame(141-159)computeMissDistanceComponentsInFrame(141-142)computeMissDistanceComponentsInLocalOrbitalFrame(161-181)computeMissDistanceComponentsInLocalOrbitalFrame(161-163)Undefined(202-205)Undefined(202-202)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp (2)
OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach_Generator(5-142)OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach_Generator(5-5)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp (4)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (5)
CloseApproach(35-55)isDefined(79-82)isDefined(79-79)Undefined(202-205)Undefined(202-202)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
Generator(59-63)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (17)
Generator(49-56)isDefined(58-61)isDefined(58-58)getReferenceTrajectory(63-71)getReferenceTrajectory(63-63)getStep(73-81)getStep(73-73)getTolerance(83-91)getTolerance(83-83)computeCloseApproaches(93-154)computeCloseApproaches(93-94)setStep(156-169)setStep(156-156)setTolerance(171-184)setTolerance(171-171)Undefined(186-189)Undefined(186-186)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (1)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (7)
CloseApproach(50-50)aCloseApproach(61-61)aCloseApproach(72-72)anOutputStream(83-83)anOutputStream(177-177)aFrame(155-155)aLocalOrbitalFrameFactorySPtr(168-170)
bindings/python/test/conjunction/close_approach/test_generator.py (5)
bindings/python/test/conjunction/test_close_approach.py (6)
environment(23-24)close_approach(91-98)earth(28-29)test_constructor_success(102-113)test_is_defined_success(115-119)test_undefined_success(121-126)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
CloseApproach(50-50)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (1)
CloseApproach(35-55)include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
Generator(59-63)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (1)
Generator(49-56)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (6)
aCloseApproach(57-65)aCloseApproach(57-57)aCloseApproach(67-70)aCloseApproach(67-67)anOutputStream(72-77)anOutputStream(72-72)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (3)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (5)
Generator(59-63)aStep(128-128)aTolerance(138-138)aTrajectory(118-118)aState1(159-159)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (5)
isDefined(79-82)isDefined(79-79)Undefined(202-205)Undefined(202-202)CloseApproach(35-55)test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (2)
anInstant(49-67)anInstant(49-54)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp (1)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (2)
OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach(7-189)OpenSpaceToolkitAstrodynamicsPy_Conjunction_CloseApproach(7-7)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (2)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (2)
CloseApproach(50-50)aFrame(155-155)src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (3)
CloseApproach(35-55)Undefined(202-205)Undefined(202-202)
🪛 Clang (14.0.6)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp
[error] 3-3: 'OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp
[error] 6-6: 'OpenSpaceToolkit/Core/Container/Tuple.hpp' file not found
(clang-diagnostic-error)
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp
[error] 3-3: 'algorithm' file not found
(clang-diagnostic-error)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp
[error] 3-3: 'OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp' file not found
(clang-diagnostic-error)
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp
[error] 3-3: 'OpenSpaceToolkit/Core/Container/Array.hpp' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test / Run C++ Unit Tests
- GitHub Check: Test / Build Python bindings
🔇 Additional comments (20)
include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.hpp (1)
35-192: LGTM!The CloseApproach class is well-designed with comprehensive documentation, proper accessors, computation methods for miss distance components in different frames, and good separation of concerns. The print method has been added as suggested in previous reviews.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction.cpp (1)
3-13: LGTM!The CloseApproach binding is properly integrated into the conjunction submodule, following the existing pattern used for other bindings.
bindings/python/test/conjunction/close_approach/test_generator.py (1)
1-228: LGTM!The test suite is comprehensive and well-structured, covering constructor variants, accessors, mutators, undefined states, and the core
compute_close_approachesfunctionality. Previous review concerns about import paths and empty-interval assertions have been addressed.test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.test.cpp (1)
1-575: LGTM!Comprehensive test suite covering construction, operators, accessors, miss distance calculations, relative state computations, and frame-based transformations. The tests properly validate both success cases and error conditions with appropriate exception message checks.
test/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.test.cpp (1)
1-557: LGTM!Excellent test suite with comprehensive coverage including constructor variants, accessors, mutators, undefined states, and real-world CDM-based validation. The parameterized tests with actual conjunction data add significant confidence. The exception handling test at lines 481-495 correctly catches
RuntimeError.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach/Generator.cpp (1)
1-142: LGTM!The Generator Python bindings are comprehensive and well-documented, exposing all key functionality including construction, accessors, mutators, and the core
compute_close_approachesmethod. The bindings follow pybind11 best practices with clear docstrings.include/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.hpp (1)
1-167: LGTM!The Generator header is well-structured with comprehensive documentation, clear API surface, and appropriate default values for
aStep(15 seconds) andaTolerance(1 millisecond). The class design follows the established patterns in the codebase.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Conjunction/CloseApproach.cpp (1)
1-189: LGTM!The CloseApproach Python bindings are comprehensive and well-documented, exposing all functionality including construction, operators, accessors, and computation methods for miss distance components in different frames. The submodule structure properly organizes the Generator binding under
close_approach.src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach/Generator.cpp (6)
40-47: LGTM! Well-structured static helper configuration.The static state builder for relative distance derivative computation is appropriately scoped and configured with GCRF frame and position/velocity subsets.
58-91: LGTM! Consistent validation pattern.The
isDefined()check and accessor methods follow a consistent defensive pattern with proper undefined-state guards.
93-154: LGTM! Sound close approach detection algorithm.The algorithm correctly identifies close approaches by finding transitions from approaching (derivative < 0) to moving apart (derivative > 0). The logic appropriately:
- Skips intervals starting at the search interval start to avoid false positives
- Creates CloseApproach instances at transition points
- Sorts results chronologically (defensive, even if TemporalConditionSolver returns ordered intervals)
156-184: LGTM! Robust input validation.Both
setStepandsetToleranceproperly validate inputs with appropriate checks for defined state and strictly positive values.
186-189: LGTM! Correct undefined singleton pattern.Returns a Generator with an undefined trajectory, ensuring
isDefined()returns false as expected.
191-225: LGTM! Mathematically correct derivative computation.The relative distance derivative
d/dt(||r||) = dot(r, v) / ||r||is correctly implemented with proper state transformation to GCRF frame and co-located state detection.The exact zero comparison on line 219 is acceptable for detecting the error condition of co-located states.
src/OpenSpaceToolkit/Astrodynamics/Conjunction/CloseApproach.cpp (6)
35-55: LGTM! Proper state validation and builder setup.The constructor correctly validates that both object states share the same instant when defined, and appropriately constructs the state builder using object1's frame with position/velocity subsets.
57-77: LGTM! Standard operator implementations.The equality, inequality, and stream output operators follow consistent patterns with proper undefined-state handling.
79-112: LGTM! Consistent accessor pattern.The
isDefined()and accessor methods follow the established pattern with appropriate guards.
114-139: LGTM! Correct miss distance and relative state computation.Both methods properly use the state builder to reduce states to position/velocity components and compute the relative state in object1's frame before calculating the miss distance.
141-181: LGTM! Both methods correctly compute miss distance components.The implementation correctly handles two different cases:
computeMissDistanceComponentsInFrame: UsesgetRelativeState()for an arbitrary framecomputeMissDistanceComponentsInLocalOrbitalFrame: Directly transformsobject2State_since the local orbital frame is centered at object1's positionThe distinction is intentional and correct, as confirmed in previous review discussions.
183-205: LGTM! Clean output formatting and undefined pattern.The
Undefined()factory correctly returns an undefined instance.
Adds a Close Approach module:
Example:
Summary by CodeRabbit
New Features
Tests