feat!: vectorize access generation #461
Conversation
WalkthroughThe pull request introduces a comprehensive enhancement to the access and visibility criteria functionality in the Open Space Toolkit's Astrodynamics module. The changes primarily focus on creating a more flexible and robust system for defining access targets and visibility conditions. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant Generator
participant AccessTarget
participant VisibilityCriterion
User->>Generator: Create with Environment
User->>AccessTarget: Create from LLA/Position/Trajectory
User->>VisibilityCriterion: Define Visibility Criteria
AccessTarget->>VisibilityCriterion: Attach Visibility Criterion
User->>Generator: Compute Accesses
Generator->>AccessTarget: Check Visibility
Generator-->>User: Return Access Intervals
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
vishwa2710
force-pushed
the
feat/compute-accesses-multiple-targets
branch
from
9af2651 to
d4cf3f9
Compare
vishwa2710
force-pushed
the
feat/add-constraint
branch
from
8402e27 to
dc571ca
Compare
vishwa2710
changed the base branch from
feat/compute-accesses-multiple-targets
to
main
vishwa2710
force-pushed
the
feat/add-constraint
branch
from
dc571ca to
8d1b417
Compare
include/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.hpp
Outdated
Show resolved
Hide resolved
vishwa2710
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
==========================================
+ Coverage 91.21% 91.27% +0.05%
==========================================
Files 86 92 +6
Lines 8720 9088 +368
==========================================
+ Hits 7954 8295 +341
- Misses 766 793 +27 ☔ View full report in Codecov by Sentry. |
![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 (26)
src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERInterval.cpp (5)
12-20: Clarify degrees-to-radians assumption in constructor
The constructor ultimately converts azimuth and elevation from degrees to radians. It would be beneficial to document clearly that the input intervals are expected in degrees (0–360 for azimuth and -90–90 for elevation) before converting. This helps avoid confusion for those passing pre-converted radian values.
26-33: Consider handling wrap-around intervals for azimuth
Azimuth can be cyclical, sometimes wrapping around 360°. Users might require an interval like [350, 10] that spans the 0° boundary. Consider adding logic (or at least documentation) for handling intervals that cross the 360°→0° boundary.
35-42: Elevation interval bounds
Checking the elevation interval within -90° to 90° is appropriate, but confirm if there is any margin needed to accommodate floating-point rounding (e.g., 90.0000001). If strict bounds are intended, the current approach is fine.
51-61: Conversion from degrees to radians
Converting each interval from degrees to radians in place is correct. As a slight improvement, reuse the conversion factor (M_PI / 180.0) in a named constant to eliminate repetition and reduce the risk of typos.
78-81: Equality operator
The equality comparison is standard. Consider documenting floating-point precision constraints if intervals have small tolerances.src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERMask.cpp (6)
27-31: Azimuth bounds covering 0–360
The check ensures the mask’s smallest azimuth is ≥ 0° and largest is ≤ 360°. This is correct. However, be mindful of cases where the user might need to wrap around 360° → 0° or expect a partial coverage.
33-39: Validate elevation angles
Allowing ±90° is correct, but consider clarifying if ±90° is inclusive or whether any margin is needed.
41-43: Ensuring mask endpoints
Inserting 0° and 360° in the mask if missing helps define a closed loop. The approach is clear and practical, but watch for potential duplication if the mask already has 0° or 360° inserted.Also applies to: 45-51
53-60: Degree-to-radian conversion
This loop-based approach is straightforward. Similar to AERInterval, consider extracting a reusable conversion constant or function to avoid repeating the factor.
72-93: Mask interpolation logic
By checking the sign of the determinant (line 91), we effectively see if the tested point is above or below the line connecting two mask data points. This is a neat geometric approach. Consider clarifying in doc comments that the mask is interpreted as the min elevation boundary.
95-98: Equality operator
The code is consistent. Keep in mind floating-point comparisons if you expect closely spaced keys inazimuthElevationMask.include/OpenSpaceToolkit/Astrodynamics/Access/Generator.hpp (1)
250-255: Compute for multiple fixed targets
This grouped approach for multipleAccessTargets is a logical extension of single-target logic. Look for any possible concurrency optimization if large arrays are used, though that might be an over-optimization for now.include/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.hpp (1)
121-152: LineOfSight struct
Storing theEnvironmentby value might be somewhat heavy if the environment is large or frequently updated. However, if immutability suits your use case, this is fine.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/Generator.cpp (1)
22-24: Clarity in aliasing
AliasingAccessTargetandGeneratorfrom theostk::astrodynamics::accessnamespace aligns well with the subsequent class definitions.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/VisibilityCriterion.cpp (1)
119-187: Potential caution with large masks
TheAERMaskis exposed with aMapof azimuth-to-elevation pairs. If the map is large, there could be performance overhead when frequently callingis_satisfied. Consider documenting or caching strategies for large masks.src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp (6)
79-94: Matrix creation for SEZ rotation
ThecomputeR_SEZ_ECEFmethod uses explicit trigonometry to form the rotation matrix. This code is clear and well-documented. Consider referencing a standard utility method if used frequently.
147-149: New constructor signature for Generator
Adding custom filters is a neat extension. This flexibility lets advanced users shape the access logic. Document expected lifetimes of the functional arguments if references are used.
408-419: No stateFilter support for multiple fixed targets
This restriction is clearly enforced. The thrown error is self-explanatory. If future needs arise, consider partial or specialized support for multi-target state filtering.
422-432: Stacked SEZ rotation
This block effectively precomputes each target’s SEZ matrix. For large target counts, watch out for memory usage or consider lazy evaluation if performance becomes an issue.
561-589: Filtering with AER masks
Cycles over each target in a loop for mask checking. This is correct, but might become a performance bottleneck with many targets. Consider a vectorized approach if needed.
590-614: Line-of-sight checks
The loop over each target individually callsisSatisfied. This is consistent with the current design. Consider parallelization if you have large sets of ground stations.test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp (1)
22-23: Added Tabulated State
This includes theTabulatedorbit model. Ensure comprehensive tests cover tabulated data edge cases (e.g., short data sets).src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/LineOfSight.cpp (2)
25-28: Constructor sets environment but no validation.
While the constructor is concise and initializes the environment, consider validating that the provided environment is well-defined (e.g., not default or null) to avoid potential runtime issues.
30-51: Ensure robust intersection checks.
The core logic for line-of-sight evaluation looks correct. However, consider these suggestions for a more robust implementation:
- Verify that transforming from ITRF to other frames (if needed) has no side effects.
- Provide a fallback in case environment objects are not up-to-date or cannot be intersected for some reason.
bindings/python/test/access/test_visibility_criterion.py (1)
156-165: Test: test_line_of_sight_is_satisfied.
Checks a representative scenario. Consider expanding to edge/corner cases, e.g., overlap with environment objects.test/Global.test.hpp (1)
66-87: Duplication in macros is acceptable but can be consolidated.
ASSERT_MATRICES_ALMOST_EQUALandEXPECT_MATRICES_ALMOST_EQUALshare much of the same code. While duplication is acceptable for clarity, a reusable helper function could reduce code repetition, making the macros more maintainable.+#ifdef USE_HELPER +inline void compareMatrices(const auto& m1, const auto& m2, double tolerance, bool assertOnFailure) +{ + if ((m1).rows() != (m2).rows() || (m1).cols() != (m2).cols()) + { + if (assertOnFailure) { FAIL() << "Matrices have different sizes."; } + else { ADD_FAILURE() << "Matrices have different sizes."; } + return; + } + for (int i = 0; i < (m1).rows(); ++i) + { + for (int j = 0; j < (m1).cols(); ++j) + { + if (std::abs((m1)(i, j) - (m2)(i, j)) > (tolerance)) + { + if (assertOnFailure) + { + FAIL() << "Matrices differ at (" << i << ", " << j << ")."; + } + else + { + ADD_FAILURE() << "Matrices differ at (" << i << ", " << j << ")."; + } + } + } + } +} +#endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
benchmark/OpenSpaceToolkit/Astrodynamics/Access.benchmark.cpp(5 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/Generator.cpp(3 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/VisibilityCriterion.cpp(1 hunks)bindings/python/test/access/test_generator.py(7 hunks)bindings/python/test/access/test_visibility_criterion.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Access/Generator.hpp(5 hunks)include/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp(8 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERInterval.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERMask.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/ElevationInterval.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/LineOfSight.cpp(1 hunks)test/Global.test.hpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp(27 hunks)test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Transform.test.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Transform.test.cpp
🧰 Additional context used
📓 Learnings (2)
src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp (1)
Learnt from: vishwa2710
PR: open-space-collective/open-space-toolkit-astrodynamics#454
File: src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp:365-369
Timestamp: 2024-11-12T02:57:39.443Z
Learning: In `Generator.cpp`, within the `computeAccessesWithGroundTargets` method, vectorizing the loop for computing `dx_SEZ` is not feasible due to how the matrices `SEZRotations` and `dx` are stacked.
include/OpenSpaceToolkit/Astrodynamics/Access/Generator.hpp (1)
Learnt from: vishwa2710
PR: open-space-collective/open-space-toolkit-astrodynamics#454
File: src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp:365-369
Timestamp: 2024-11-12T02:57:39.443Z
Learning: In `Generator.cpp`, within the `computeAccessesWithGroundTargets` method, vectorizing the loop for computing `dx_SEZ` is not feasible due to how the matrices `SEZRotations` and `dx` are stacked.
🪛 cppcheck (2.10-2)
test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp
[error] 152-152: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp
[error] 45-45: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (174)
src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERInterval.cpp (5)
21-25: Good undefined-interval check
The early exit on undefined intervals is a best practice. The code properly throws an exception if any interval is undefined to prevent erroneous usage.
44-49: Ensure range intervals align with domain
It is good to see an exception thrown if the lower bound is negative. For completeness, consider also rejecting intervals that start at 0.0 if you never expect zero range.
63-68: Accurate check inisSatisfied(const AER&)
The method correctly transforms the input AER to radians and meters. The code is straightforward and properly delegates to the overloaded method.
70-75: Efficient containment check
The method’s logic for checking containment within the three intervals is clear and efficient.
84-87: Inequality operator
Delegating tooperator==is a neat approach. No changes needed here.src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERMask.cpp (4)
14-19: Constructor handles mask and range
The constructor structure is clear. It associates an azimuth-elevation mask with a range interval. Good approach for modularizing the logic.
20-25: Range interval positivity
The range must be positive, which makes sense. You might want to confirm zero-range edge cases if relevant for direct contacts (e.g., altitude exactly overhead).
63-70:isSatisfied(const AER&)
This pass-through approach is consistent and mirrors the logic used inAERInterval. Well-defined usage.
100-103: Inequality operator
Again, straightforward usage of negating the equality operator. Nicely done.include/OpenSpaceToolkit/Astrodynamics/Access/Generator.hpp (6)
61-66: Helpful class-level documentation
IntroducingAccessTargetclarifies how position-based vs. trajectory-based targets are handled. Good overall description.
69-73: Enum classType
A simple but effective distinction betweenFixedandTrajectory. No issues here; usage is straightforward.
180-186: Well-structured private constructor
The private constructor consolidates thetype_,visibilityCriterion_, andtrajectory_. This design encourages using static factories. Good approach to prevent misuse.
210-218: Condition function param
getConditionFunctionnow takes anAccessTargetrather than a raw trajectory, aligning with the new design. Make sure all references to the older approach are updated or deprecated.
214-218: computeAccesses over interval
Good pattern to allow a coarse mode. The doc comment specifically states it's only available for fixed targets. Make sure calls using a trajectory target handlecoarse=truegracefully (e.g., ignoring coarse logic or throwing).
246-248: Separation ofcomputeAccessesForTrajectoryTarget
This helper clearly separates logic for trajectory-based computations from fixed targets. Enhances readability.include/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.hpp (9)
37-80: Modular AERInterval structure
TheAERIntervalstruct is well defined with separate checks for AER. The code remains consistent with the.cppimplementation. Good separation of concerns.
82-119: AERMask structure
Similarly,AERMaskprovides an alternative approach to intervals, using a map-based mask. Clear naming and well-structured interface.
154-181: ElevationInterval
Focused on a single elevation range check. Straightforward design covers simpler restricted-elevation use cases.
195-206: FromAERInterval
Exposes a factory method for a combined AER interval. This is consistent with usage patterns in the.cppfile.
207-215: FromAERMask
Same pattern for mask-based criteria. The naming is clear and unambiguous.
216-221: FromLineOfSight
Constructing from an environment is intuitive. Confirm that environment usage remains thread-safe if concurrency arises.
222-226: FromElevationInterval
Simple helper for an elevation-only criterion. Great for quick usage in common scenarios.
228-237:is<T>()template
Useful for differentiating criterion type. This is idiomatic forstd::variant.
238-251:as<T>()template
Provides a convenient cast to retrieve the underlying criterion. Good addition for flexible usage.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/Generator.cpp (14)
11-19: Use statements
The set ofusingdirectives neatly organizes frequently used types. Nothing to adjust here.
27-34: Class docstrings
Clear docstring explainingAccessTarget. This helps Python users quickly grasp its purpose.
36-42: Enum Exposure
Exporting the enum to Python is straightforward. The docstring clarifies the meaning of each variant.
44-54:get_typebinding
Neatly exposesaccessType(). The docstring is thorough, matching C++ side doc.
55-63:get_visibility_criterion
Ensures transparency for the Python side. The docstring informs how to retrieve the underlyingVisibilityCriterion.
64-73:get_trajectory
Provides direct trajectory access. This is essential for further manipulations in Python.
74-83:get_position
Straightforward binding for fixed positions. No further improvements needed.
84-98:get_lla
Good to see the docstring clarifying that aCelestialis needed for the conversion. The signature is intuitive.
99-112:compute_r_sez_ecef
Exposes the rotation matrix. The docstring properly warns about shape (3x3).
113-161: Static factories
from_lla,from_position,from_trajectoryare nicely consolidated. Documentation is thorough, aligning well with the C++ side.
Line range hint
162-178:Generatorconstructor
Removes the AER filter, which is now replaced by the broader visibility-criterion approach. The defaults are consistent with the C++ side.
258-268:get_condition_function
Exposes the internal logic for partial functional composition in Python. This is advanced but helpful.
273-293:compute_accesses(single target)
Matches the newAccessTargetapproach. The coarse-mode doc is consistent with the C++ doc. Good job.
295-315:compute_accesses(multiple targets)
Provides the Python side with a variety of targets in a single call. Ensures feature parity with the C++ API.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/VisibilityCriterion.cpp (6)
17-35: Good structure for top-level pybind11 class definition
The wrapper forVisibilityCriterionis properly set up, enabling Python classes to inherit the docstrings from the underlying C++ classes. The docstring provides a concise overview of the class purpose.
36-117: AERInterval Python binding looks complete
All constructors, read-only members, andis_satisfiedoverloads are exposed. The docstrings clearly explain how to use the intervals and the arguments. This is helpful for Python users.
189-223: LineOfSight constructor and environment usage
The constructor forLineOfSightrelies on a valid environment. Ensure that any Python-side code checks or documents that the passed environment is defined and not None before usage, so you avoid runtime issues.
225-264: ElevationInterval angle conversion
The docstrings clearly mention degrees vs. radians, which can be confusing. Your Python docstrings handle both carefully. This is helpful to reduce user error.
266-368: Static factory methods
Providing short, descriptive names (from_aer_interval,from_aer_mask, etc.) is a great way to keep the Python API readable. The docstrings are also well-structured.
370-448: Casting methods throwvalue_error
These bindings properly raise Python exceptions on incorrect criterion casting. This is good for aligning with Pythonic error handling.src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp (24)
47-50: accessType() method returning a reference
Returning a const reference is fine for performance; just confirm that the enumeration is stable or trivially destructible in all usage scenarios.
[approve]
52-60: VisibilityCriterion & Trajectory references
Both methods return a const reference to members. This is a good design for preventing accidental modification, but ensure that the objects outlive all references.
[approve]
62-70: Handling of fixed-position type
Throwing an error if theAccessTargetis notFixedis appropriate. This is consistent with preventing undefined states.
96-107: Creation via FromLLA
This factory constructor is valuable for typical ground station definitions. Including the environment’s flattening and radius ensures geographic correctness.
109-117: Position-based creation
Nicely parallels the LLA-based constructor. The check onisDefined()is essential.
131-141: AccessTarget constructor error on undefined trajectory
This guard ensures that we never create an invalidAccessTarget. Good defensive coding.
Line range hint
205-240: Generating condition function
This lambda captures references to relevant objects and performs environment checks. Code clarity is good. You handle state filtering and multiple visibility criterion checks.
[approve]
262-291: computeAccesses single target
The logic checks if the target is trajectory or fixed, then delegates. The coarse mode exception for trajectory targets is an appropriate guard. Good maintainability.
293-355: computeAccesses multiple targets
Ensures that all targets share the sameAccessTarget::Type. This is an important design constraint. The error messages are descriptive.
394-405: UsingtemporalConditionSolver.solve
This is well-structured and consistent with the rest of the code. Confirm that the solver handles corner cases such as tiny intervals or instant-based edges.
476-507: Lambda for computing AER
The triple of(azimuth_rad, elevation_rad, range_m)is intuitive. The loop carefully indexes the 3×3 block. All steps are correct.
509-518: Separately computing elevations
Splitting the logic into a dedicated lambda is neat. Minimizes duplication. The code remains readable.
542-559: Filtering with AER intervals
Vectorized approach ensures efficient checks for each target in one pass. The boolean array usage is a clean solution.
615-644: Elevation intervals
Again, consistent approach to building a filter function. The final array of booleans for each target is well-managed. Code is readable.
646-649: Error for mixing different criterion types
Forbidding mixing in the same call keeps the logic simpler. This aligns with the design constraints.
651-667: Building inAccessPerTarget
The iterative approach is straightforward. The row assignment from the casted array is well done.
678-689: Refining intervals if not coarse
The call tocomputePreciseCrossingsfor each target provides better sub-interval accuracy. This is crucial for finer results.
703-729: generateAccessesFromIntervals
Maps intervals toAccessobjects and applies an optional filter. This is a neat separation of concerns.
731-876:computePreciseCrossingswith rootSolver
The approach to refine the intervals using the root solver is well-structured. The handling of partial intervals and properly bridging the bounding points is robust.
878-918: ComputeIntervals
Properly identifies transitions from no-access to in-access. The padded vector approach is a standard technique. Looks correct.
940-944: Extending zero-duration interval
This block artificially widens the interval to handle single-instant phenomena. This is a practical solution.
Line range hint
990-999: Squared norm usage for objective
MinimizingdeltaPosition.squaredNorm()is typical for range-based optimization. Implementation is correct.
1062-1078: Elevation calculation
Computes the localdx_Zvia dot product with the rotation vector. This method is straightforward and likely correct for Earth references.
1081-1091: CalculateAer
Switches into NED frame to computeAER::FromPositionToPosition. This is a known approach. The code is well-structured.test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp (34)
10-11: Added Interpolator header
Bringing inInterpolator.hppsuggests new test coverage that might rely on orbit or trajectory interpolation. Make sure to confirm test usage is relevant.
69-77: Test fixtures
Using dedicated test fixtures is a good practice for shared setup. This improves readability and maintainability.
99-147: AccessTarget constructor tests
Checks the factory methods thoroughly (FromLLA,FromPosition,FromTrajectory). This is great coverage for each creation path. TheEXPECT_THROWtests for undefined inputs are also correct.
Line range hint
149-180: Generator constructor tests
Multiple constructor overloads tested with varying filters. Good to see you verifying the internal state of theGenerator.🧰 Tools
🪛 cppcheck (2.10-2)
[error] 152-152: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
181-184: AccessType test
Simple, direct check. This ensuresAccessTarget::Type::Fixedis returned properly.
186-189: AccessVisibilityCriterion test
Straightforward equality check ensures that the stored criterion matches the reference.
191-194: AccessTrajectory test
Verifies no exception thrown onaccessTrajectory(). Good basic coverage.
196-215: GetPosition test
Covers both the valid (fixed) and invalid (trajectory) usage. Thorough approach verifying the thrown exception.
217-222: GetLLA test
Ensures numerical consistency by comparing the result to the original LLA, which is a sensible approach.
224-232: ComputeR_SEZ_ECEF test
Compares the matrix to a known reference. This is a good approach to verifying rotation correctness.
234-242: FromLLA test
Basic checks confirm the correct type assignment and the sameVisibilityCriterion.
243-250: FromPosition test
Similarly straightforward. Good coverage.
252-259: FromTrajectory test
EnsuresTrajectorytargets are properly stored. Good test.
Line range hint
261-289: isDefined tests
Multiple checks verifying the default generator plus the undefined generator. This is important for robust usage.
290-299: GetStep tests
Confirms the returned step for the default generator or throws for undefined.
301-310: GetTolerance tests
Same pattern asGetStep. Good consistency in how the library handles undefined states.
312-322: GetAccessFilter tests
Confirms the default is null, ensures undefined generator usage fails.
323-333: GetStateFilter tests
Same pattern asGetAccessFilter. All good.
Line range hint
334-408: GetConditionFunction tests
Detailed coverage of valid or invalid orbits, plus verifying the function pointer. The usage of the generated orbits is thorough.
Line range hint
414-484: ComputeAccesses_1
Compares the results against a reference CSV with a tolerance. Checking each instant ensures stable regression tests.
Line range hint
524-589: ComputeAccesses_2
Similarly uses a reference file for comparison. This is consistent with the approach in_1. Good approach for real data.
Line range hint
610-683: ComputeAccesses_3
Tests TLE-based orbit with a ground station. Great coverage to ensure line-of-sight computations function.
Line range hint
683-768: ComputeAccesses_4
Focuses onstateFilterusage with a custom condition. The test is well structured and checks partial vs. full intervals.
774-848: ComputeAccesses
Throws for various undefined states or disallowed configurations. This ensures robust error handling. The multiple target checks confirm uniform type usage.
870-896: Mixed LLA array
Testing many ground stations with the new methods ensures thorough coverage. The large array should confirm performance and correctness.
[approve]
898-954: Multiple AERInterval targets
Computes combined accesses and compares them to the single-target approach. This is a superb cross-verification technique.
956-1008: Multiple ElevationInterval targets
Follows the same pattern as AERInterval. This consistent testing approach fosters confidence in code correctness.
1012-1019: SetStep
Check that updating the generator’s step works or throws for undefined. Very direct.
1023-1031: SetTolerance
Same structure asSetStep. Good code parity.
1034-1044: SetAccessFilter
Ensures we can set or reset the filter with no issues.
1048-1058: SetStateFilter
Same thoroughness asSetAccessFilter.
Line range hint
1062-1068: Undefined
Checks forGenerator::Undefined()usage. Clear coverage.
Line range hint
1071-1157: AerRanges_1
Again references CSV data for validation. The test scaffolding is consistent with the other scenarios.
Line range hint
1168-1253: AerMask_1
Similar structure toAerRanges_1. Thorough test coverage for AER masks.src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.cpp (6)
12-15: Equality operator
Compares the internal variant. This is straightforward and ensures equality checks reflect the underlying data.
17-20: Inequality operator
Correctly defers to the equality operator. Perfect approach to keep logic centralized.
22-28: FromAERInterval
Creates anAERIntervalinternally. No issues spotted; code is straightforward.
29-34: FromAERMask
Similarly straightforward. Good constructor usage.
36-39: FromLineOfSight
Simple environment-based constructor, consistent with the rest.
41-44: FromElevationInterval
Keeps consistent style with other static creation methods.src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/ElevationInterval.cpp (3)
12-29: ElevationInterval constructor
Bounds enforced at [-90, 90] deg. The error message is descriptive. Converting to radians at construction is convenient. Watch for possible floating precision issues on boundary extremes (e.g., exactly -90.0 or 90.0).
31-34: isSatisfied
UsesInterval::containsto check. Straightforward.
36-44: Equality and inequality operators
Implementation is consistent across allVisibilityCriterionclasses. This ensures uniform usage.src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/LineOfSight.cpp (3)
1-2: License header check.
No issues identified with the license header.
53-56: Equality operator only checks access objects.
Currently,operator==compares only the environment’s access objects. Consider whether other environment properties (e.g., gravitational models, time scale) should also be included to ensure complete equivalence.
58-61: Inequality operator is consistent with equality operator.
Implementation simply invertsoperator==, which is correct and consistent.benchmark/OpenSpaceToolkit/Astrodynamics/Access.benchmark.cpp (7)
17-17: New include for VisibilityCriterion.
The addition of<OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.hpp>is appropriate given the usage below.
40-40: Using statement for AccessTarget.
Neat usage alias, ensures code clarity.
42-42: Using statement for VisibilityCriterion.
Similarly, clear usage for the newly introduced class.
56-58: Updated function signature to use AccessTarget.
This is a significant change that improves clarity by explicitly referencing an AccessTarget instead of using a Trajectory. Be sure to review the entire codebase (including external calls) to confirm they are updated accordingly.
64-64: Generator call used purely for performance measurement.
WrapscomputeAccesses()inbenchmark::DoNotOptimize, which is correct for benchmarking.
78-80: Creation of theVisibilityCriterionandAccessTarget.
This approach is aligned with the new design of usingAccessTarget.
90-90: Calling the updatedcomputeAccessesfunction.
Integrated seamlessly with the new method signature.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access.cpp (2)
6-6: Include Python bindings for VisibilityCriterion.
ImportingVisibilityCriterion.cppensures the new classes are exposed to Python.
203-203: Integration of VisibilityCriterion in Python submodule.
OpenSpaceToolkitAstrodynamicsPy_Access_VisibilityCriterioncall properly wires up the new functionality in Python.bindings/python/test/access/test_visibility_criterion.py (23)
1-2: License header check.
No issues identified with the added license.
3-5: Standard imports for testing.
Using pytest and numpy is standard practice. Nothing unusual here.
6-7: Import RealInterval.
This is central to testing AER intervals. The import is correct.
8-11: Environment and time utilities.
Necessary imports for environment/time-based tests.
14-15: VisibilityCriterion import.
Ensures direct usage of the new functionality in tests.
17-20: Fixture: instant.
Provides a consistent test instant. Using a fixed time is fine for unit tests.
22-25: Fixture: environment.
Uses the default environment. This is appropriate for basic line-of-sight tests.
27-30: Fixture: azimuth_interval.
Simple RealInterval for AER testing. No concerns.
32-35: Fixture: elevation_interval.
Same as above but for elevation. Straightforward usage.
37-40: Fixture: range_interval.
Covers typical orbital altitudes up to 1e7.
42-48: Fixture: aer_mask.
Defines a dict for an azimuth-elevation mask. Straightforward structure.
50-65: Test: test_aer_interval_constructor.
Verifies thatAERIntervalis constructed properly. This is good coverage.
65-73: Test: test_aer_mask_constructor.
EnsuresAERMaskcan handle dict inputs as expected.
74-81: Test: test_line_of_sight_constructor.
Checks that a valid environment-based criterion is constructed.
82-89: Test: test_elevation_interval_constructor.
Simple coverage forElevationInterval. No obvious issues.
90-102: Test: test_visibility_criterion_from_aer_interval.
Covers the static creation method and type check. Thorough.
103-112: Test: test_visibility_criterion_from_aer_mask.
Tests creation from an AER mask criterion. Straightforward.
113-121: Test: test_visibility_criterion_from_line_of_sight.
Covers the line-of-sight type creation. Good usage ofis_line_of_sight().
122-129: Test: test_visibility_criterion_from_elevation_interval.
Similarly confirms the type as elevation interval.
130-144: Test: test_aer_interval_is_satisfied.
Validates that out-of-range AER fails. Thorough coverage.
145-155: Test: test_elevation_interval_is_satisfied.
Verifies correct inclusion/exclusion in the interval. No issues.
166-175: Test: test_visibility_criterion_type_checks.
Confirms correct boolean checks for each type. Straightforward.
179-193: Test: test_visibility_criterion_cast.
Tests the cast toAERIntervaland ensuresValueErrorfor invalid cast. Comprehensive.test/Global.test.hpp (1)
43-64: Macros for matrix comparison correctly implemented.They closely mirror the vector comparison macros. The nested loops and tolerance checks are straightforward and well-structured. This is a valuable utility for ensuring matrix equality in tests.
bindings/python/test/access/test_generator.py (14)
5-6: Using numpy is good for array manipulations and matrix operations.If matrix or vector assertions are done in Python tests, it may be beneficial to align with the new matrix macros in C++ using consistent tolerance values.
17-19: Import statements offer better test coverage.Imports for
Position,Frame, andLLAexpand test coverage to coordinate transformations, aligning with the newAccessTargettests for position-based checks.
21-21: Import of Earth object introduces real scenario testing.This is a good approach to ensure that environment-based tests are validated against realistic celestial bodies.
29-30: Imports for new access functionality.
AccessTargetandVisibilityCriterionare integral to the new approach to target definition. Their usage ensures the test suite covers the newly introduced classes thoroughly.
48-48: Use ofNoneforstate_filteris valid.This ensures that state transitions are not filtered, providing a baseline scenario. Confirm that any specialized filtering is tested elsewhere.
92-95: Fixture naming aligns with standard test patterns.
visibility_criterionfixture is self-descriptive. However, be sure to test different criteria in separate fixtures or parameterized tests to cover edge cases.
97-100: Use ofLLAfixture is appropriate.Ensures repeated creation of location-based test data remains consistent and easy to maintain.
102-109:AccessTarget.from_llafixture clarifies test intentions.It ensures reproducible setup for tests requiring a fixed LLA-based target. This pattern keeps test code clean and comprehensible.
111-117:AccessTarget.from_trajectoryfixture covers dynamic scenarios.This ensures coverage for time-dependent or orbital-based targets.
119-188: Comprehensive unit tests forAccessTarget.All critical methods are tested:
- Constructors
get_type- Trajectory checks
- Position and LLA retrieval
- Derivative computations (
compute_r_sez_ecef)
This thorough coverage strengthens confidence in the new feature set.
Line range hint
188-240: Extensive constructor tests forGenerator.Ensures valid initialization across different combinations (environment, access/state filters, step, tolerance). Good test diversity.
245-251: Condition function test ensures correct hooking of target logic.It confirms that a trajectory-based
AccessTargetis properly recognized by theGenerator.
Line range hint
264-281: Basic coverage ofcompute_accessessingle-target usage.Ensures that computed intervals are valid. Consider testing corner cases like zero-length intervals or extremely short durations.
281-301: Multiple targets handling.This test ensures logic can handle
access_targetsas a list. The result being a list of lists ofAccessobjects is well-covered. Overall approach is correct.test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp (13)
1-2: License header present, no issues.
3-8: Includes are well-organized and minimal.They bring in relevant classes, bridging to newly introduced functionalities.
9-9: Integration with global testing utilities.
#include <Global.test.hpp>ensures quick access to shared macros or global test resources.
15-26: Clean namespace usage.Logical usage of
usingstatements clarifies code references. This fosters readability, especially for test code.
29-34: Fixture approach sets up the environment for testing.Leverages
Environment::Default()for consistent test conditions. Good pattern for repeated usage.
36-104:Constructortest thoroughly checks valid and invalid creation paths.
- Covers valid intervals.
- Validates undefined intervals.
- Exercises out-of-bound intervals.
Good coverage.🧰 Tools
🪛 cppcheck (2.10-2)
[error] 45-45: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
105-127:FromAERMask,FromLineOfSight, andFromElevationIntervalvalidations.Robust checks with
EXPECT_NO_THROWconfirm that these constructors handle normal usage.
129-169:Istest ensures runtime type identification.It verifies each variant (AERInterval, AERMask, LineOfSight, ElevationInterval). This is essential to multi-variant classes.
171-232:Astest ensures correct downcasting.Testing each variant’s content, including correct radian conversions, helps prevent subtle numeric errors.
234-254:AERIntervalIsSatisfiedtest covers basic inclusion/exclusion.Verifies that within-bounds AER is recognized, while out-of-bounds values are rejected.
257-277:AERMaskIsSatisfiedtest ensures multi-segment coverage.Checks valid interpolation between mask points and out-of-bounds rejection. Good for verifying complex mask logic.
279-293:LineOfSightIsSatisfiedtest highlights realistic geometry check.Confirms that
LineOfSightis properly recognized as blocked. Could be extended to test an unobstructed scenario.
295-313:ElevationIntervalIsSatisfiedconfirms angle constraints.Confirms that valid angles pass, while too-low or too-high angles fail. Adequate functional coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bindings/python/test/access/test_visibility_criterion.py(1 hunks)bindings/python/test/solvers/test_temporal_condition_solver.py(2 hunks)bindings/python/test/test_display.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
bindings/python/test/access/test_visibility_criterion.py
12-12: ostk.physics.coordinate.spherical.AER imported but unused
Remove unused import: ostk.physics.coordinate.spherical.AER
(F401)
🔇 Additional comments (8)
bindings/python/test/test_display.py (4)
23-24: Imports look fine.
It’s great to seeAccessTargetandVisibilityCriterionimported for more flexibility in computing satellite access.
56-61: Appropriate usage ofAccessTargetandVisibilityCriterion.
Creating a line-of-sight visibility criterion from the environment and then deriving anAccessTargetwith the position-based approach is a good design decision. This clarifies how the ground station’s location and environment-based constraints come into play.
85-85: Consistent usage ofaccess_target.
Ensures that the new visibility approach is being applied.
93-93: Good approach to reuseaccess_target.
Maintains consistency in how access is computed across different trajectories.bindings/python/test/solvers/test_temporal_condition_solver.py (3)
19-20: Imports look good.
These imports introduce the new criterion- and target-based access approach in the solver test.
146-150: Adoption of criterion-based access.
The creation of aVisibilityCriterionfrom line-of-sight and then constructing theAccessTargetfrom a trajectory aligns well with the new design for access generation.
154-154: Seamless integration with generator condition function.
Passingaccess_targetintogenerator.get_condition_functionhighlights the new architectural approach, combining environment, target, and trajectory for robust computations.bindings/python/test/access/test_visibility_criterion.py (1)
1-199: Comprehensive test coverage forVisibilityCriterion.
These tests provide a robust set of checks for AER intervals, masks, line of sight, and elevation-based visibility. Great job ensuring each constructor and satisfaction method is validated.🧰 Tools
🪛 Ruff (0.8.2)
12-12:
ostk.physics.coordinate.spherical.AERimported but unusedRemove unused import:
ostk.physics.coordinate.spherical.AER(F401)
vishwa2710
force-pushed
the
feat/add-constraint
branch
2 times, most recently
from
fc20d83 to
c1ece1b
Compare
vishwa2710
force-pushed
the
feat/add-constraint
branch
from
4a87f77 to
5867a96
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp (1)
Line range hint
1126-1129: Use relative paths for test data files.Using absolute paths like
/app/test/...makes the tests less portable. Consider using relative paths or environment variables.- const File referenceDataFile = - File::Path(Path::Parse("/app/test/OpenSpaceToolkit/Astrodynamics/Access/Generator/AerRanges/Scenario 1.csv") - ); + const File referenceDataFile = + File::Path(Path::Parse("test/OpenSpaceToolkit/Astrodynamics/Access/Generator/AerRanges/Scenario 1.csv") + );
🧹 Nitpick comments (10)
bindings/python/test/access/test_visibility_criterion.py (2)
42-46: Improve Documentation foraer_maskFixtureIt would be helpful to add a short docstring or comment indicating the intended meaning of the dictionary key-value pairs (e.g., azimuth as key, elevation limit as value).
129-147: Consider Additional Boundary TestingWhen validating
aer_interval.is_satisfied, you might also test boundary conditions like exactly 0°, 90°, or outside intervals (e.g., just above 90°). This would ensure robust coverage of edge cases.bindings/python/test/test_display.py (1)
60-61: Position-based AccessTarget for Ground StationUsing
from_positionclarifies that you’re referencing a fixed point on Earth for access checks. Keep an eye on coordinate frame transformations when dealing with altitudes or non-ITRF frames.include/OpenSpaceToolkit/Astrodynamics/Access/Generator.hpp (1)
246-270: Consider adding documentation for private methods.While the implementation looks correct, adding documentation for private methods would improve maintainability.
include/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.hpp (2)
124-124: Consider making the Environment member non-mutable.The
mutablekeyword on the Environment member might not be necessary. Consider if there's a way to restructure the class to avoid mutable state.
254-254: Consider adding a default constructor.The class might benefit from a default constructor that creates a reasonable default visibility criterion.
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/Generator.cpp (2)
47-53: Enhance docstring with return type details.The docstring for
get_typecould be more specific about the return type by listing the possible enum values.R"doc( Get the type of the access target. Returns: - AccessTarget.Type: The type of the access target. + AccessTarget.Type: The type of the access target (Fixed or Trajectory). )doc"
279-283: Enhance docstring for the coarse parameter.The docstring should explain why coarse mode is not supported for trajectory targets and what are the implications of using it.
Args: interval (Interval): The time interval over which to compute accesses. access_target (AccessTarget): The access target to compute the accesses with. to_trajectory (Trajectory): The trajectory to co compute the accesses with. - coarse (bool): True to use coarse mode. Defaults to False. Only available for fixed targets. + coarse (bool): True to use coarse mode for faster but less accurate results. Defaults to False. + Only available for fixed targets as trajectory targets require precise calculations.test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp (1)
39-62: Add edge cases to FromAERInterval constructor tests.The test cases should include additional edge cases such as:
- Azimuth interval crossing 360 degrees
- Elevation interval at exact boundaries (-90, 90)
- Range interval with very large values
{ const Interval<Real> azimuthInterval = Interval<Real>::Closed(0.0, 360.0); const Interval<Real> elevationInterval = Interval<Real>::Closed(0.0, 90.0); const Interval<Real> rangeInterval = Interval<Real>::Closed(0.0, 1e6); EXPECT_NO_THROW(VisibilityCriterion visibilityCriterion = VisibilityCriterion::FromAERInterval(azimuthInterval, elevationInterval, rangeInterval);); + // Test azimuth interval crossing 360 degrees + const Interval<Real> crossingAzimuthInterval = Interval<Real>::Closed(350.0, 10.0); + EXPECT_NO_THROW(VisibilityCriterion visibilityCriterion = + VisibilityCriterion::FromAERInterval(crossingAzimuthInterval, elevationInterval, rangeInterval);); + + // Test elevation interval at exact boundaries + const Interval<Real> fullElevationInterval = Interval<Real>::Closed(-90.0, 90.0); + EXPECT_NO_THROW(VisibilityCriterion visibilityCriterion = + VisibilityCriterion::FromAERInterval(azimuthInterval, fullElevationInterval, rangeInterval);); + + // Test range interval with very large values + const Interval<Real> largeRangeInterval = Interval<Real>::Closed(0.0, 1e12); + EXPECT_NO_THROW(VisibilityCriterion visibilityCriterion = + VisibilityCriterion::FromAERInterval(azimuthInterval, elevationInterval, largeRangeInterval););🧰 Tools
🪛 cppcheck (2.10-2)
[error] 45-45: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp (1)
477-482: Consider vectorization opportunities for SEZ computation.The comment "TBI: See if this can be vectorized" suggests potential performance improvements. Based on the retrieved learning from PR #454, vectorizing this loop is not feasible due to how the matrices are stacked. Consider documenting this limitation in the code.
- // TBI: See if this can be vectorized + // Note: Vectorization is not feasible here due to the structure of SEZRotations matrix, + // where each 3x3 block needs to be applied independently to the corresponding column of dx. MatrixXd dx_SEZ = MatrixXd::Zero(3, dx.cols()); for (Eigen::Index i = 0; i < dx.cols(); ++i) { dx_SEZ.col(i) = SEZRotations.block<3, 3>(0, 3 * i) * dx.col(i); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
benchmark/OpenSpaceToolkit/Astrodynamics/Access.benchmark.cpp(5 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access.cpp(2 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/Generator.cpp(3 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/VisibilityCriterion.cpp(1 hunks)bindings/python/test/access/test_generator.py(7 hunks)bindings/python/test/access/test_visibility_criterion.py(1 hunks)bindings/python/test/solvers/test_temporal_condition_solver.py(2 hunks)bindings/python/test/test_display.py(3 hunks)include/OpenSpaceToolkit/Astrodynamics/Access/Generator.hpp(5 hunks)include/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp(8 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERInterval.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERMask.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/ElevationInterval.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/LineOfSight.cpp(1 hunks)test/Global.test.hpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp(27 hunks)test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Transform.test.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Transform.test.cpp
- bindings/python/test/solvers/test_temporal_condition_solver.py
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access.cpp
- src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/LineOfSight.cpp
- src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/ElevationInterval.cpp
- src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERMask.cpp
- bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Access/VisibilityCriterion.cpp
- src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.cpp
🧰 Additional context used
📓 Learnings (2)
include/OpenSpaceToolkit/Astrodynamics/Access/Generator.hpp (1)
Learnt from: vishwa2710
PR: open-space-collective/open-space-toolkit-astrodynamics#454
File: src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp:365-369
Timestamp: 2024-11-12T02:57:39.443Z
Learning: In `Generator.cpp`, within the `computeAccessesWithGroundTargets` method, vectorizing the loop for computing `dx_SEZ` is not feasible due to how the matrices `SEZRotations` and `dx` are stacked.
src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp (1)
Learnt from: vishwa2710
PR: open-space-collective/open-space-toolkit-astrodynamics#454
File: src/OpenSpaceToolkit/Astrodynamics/Access/Generator.cpp:365-369
Timestamp: 2024-11-12T02:57:39.443Z
Learning: In `Generator.cpp`, within the `computeAccessesWithGroundTargets` method, vectorizing the loop for computing `dx_SEZ` is not feasible due to how the matrices `SEZRotations` and `dx` are stacked.
🪛 cppcheck (2.10-2)
test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp
[error] 152-152: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp
[error] 45-45: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Run Python Unit Tests
- GitHub Check: Test / Run C++ Unit Tests
🔇 Additional comments (21)
src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERInterval.cpp (3)
21-24: Throwing for Undefined Intervals is Good PracticeGood job ensuring that the constructor throws a runtime error if the intervals are undefined. This helps avoid silently propagating invalid states in the codebase.
63-76: Satisfaction Logic Looks GoodThe
isSatisfiedmethods correctly check whether a given AER state lies within all three intervals. This is straightforward, maintains readability, and should be efficient for typical usage.
78-87: Operators Provide Clear ComparisonsOverloading
operator==andoperator!=to compare all three intervals (azimuth, elevation, range) is appropriate and enhances readability.bindings/python/test/access/test_visibility_criterion.py (1)
49-63: Constructor Test Coverage is ThoroughThe
test_aer_interval_constructormethod ensures that constructing anAERIntervalis straightforward and returns the correct type. Good job verifying object creation.bindings/python/test/test_display.py (2)
23-24: Congratulations on Switching to the New Access APIImporting
AccessTargetandVisibilityCriterionaligns with the new vectorized access approach. This should make the display logic more flexible and cohesive with current architecture trends.
56-59: Line-of-Sight Criterion UsageCreating the
VisibilityCriterionfromline_of_sightis straightforward. Verify that this environment-based approach suits all operational scenarios (e.g., scenes with additional or custom celestial objects).benchmark/OpenSpaceToolkit/Astrodynamics/Access.benchmark.cpp (4)
17-17: LGTM! Include added for new VisibilityCriterion functionality.The include is correctly placed and aligns with the new architecture.
40-42: LGTM! Using declarations added for new types.The declarations are properly ordered and align with the new architecture.
56-58: LGTM! Function signature updated to use AccessTarget.The change from
TrajectorytoAccessTargetaligns with the new architecture for access computations.Also applies to: 64-64
78-80: LGTM! Ground station setup updated to use VisibilityCriterion and AccessTarget.The changes correctly demonstrate the new pattern for creating access targets with visibility criteria.
Also applies to: 90-90
test/Global.test.hpp (2)
43-64: LGTM! EXPECT_MATRICES_ALMOST_EQUAL macro added.The macro follows best practices:
- Proper use of do-while(0) for macro safety
- Comprehensive dimension checks
- Element-wise comparison with tolerance
- Clear failure messages
66-87: LGTM! ASSERT_MATRICES_ALMOST_EQUAL macro added.The macro follows best practices:
- Proper use of do-while(0) for macro safety
- Uses FAIL() for immediate test termination
- Matches the structure of EXPECT variant
bindings/python/test/access/test_generator.py (6)
5-6: LGTM! New imports added.The imports are properly organized and include all necessary types for the new functionality.
Also applies to: 17-19, 21-21, 29-30
48-48: LGTM! Generator fixture updated.Removing the state filter default aligns with the new architecture.
92-109: LGTM! New fixtures added for visibility criterion and access target creation.The fixtures provide reusable test setup for the new functionality.
111-116: LGTM! Trajectory target fixture added.The fixture correctly creates an AccessTarget from a trajectory and visibility criterion.
119-185: LGTM! TestAccessTarget class added with comprehensive test coverage.The test class thoroughly validates:
- Constructor
- Type retrieval
- Visibility criterion access
- Trajectory access
- Position access
- LLA access
- Matrix computations
281-301: LGTM! Test added for multiple targets support.The test properly validates the new capability to compute accesses for multiple targets.
include/OpenSpaceToolkit/Astrodynamics/Access/Generator.hpp (1)
61-186: LGTM! AccessTarget class added with comprehensive functionality.The class is well-designed with:
- Clear documentation
- Type-safe enumeration
- Proper encapsulation
- Factory methods for different creation patterns
include/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.hpp (1)
232-251: LGTM! Type-safe variant access with proper error handling.The template methods
is()andas()provide a safe way to check and access the underlying visibility criterion type.test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cpp (1)
1156-1159: Uncomment and fix timeOfClosestApproach assertions.The commented-out assertions for timeOfClosestApproach should either be fixed and uncommented or removed with an explanation.
✅ Verification successful
Remove commented timeOfClosestApproach code for better readability
The commented-out assertions appear to be intentionally disabled for this specific test case, while timeOfClosestApproach is properly tested in other test cases within the same file. For better code readability, these commented sections should be removed rather than uncommented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if timeOfClosestApproach assertions are commented out in other test files rg -l "timeOfClosestApproach.*commented" test/Length of output: 48
Script:
#!/bin/bash # Search for timeOfClosestApproach occurrences in test files rg "timeOfClosestApproach" test/ -B 2 -A 2 # Look for commented EXPECT assertions related to timeOfClosestApproach rg "//.*EXPECT.*timeOfClosestApproach" test/ # Find all EXPECT assertions with timeOfClosestApproach (not commented) rg "EXPECT.*timeOfClosestApproach" test/Length of output: 37909
Script:
#!/bin/bash # Get context around the commented assertions rg -B 10 -A 10 "// EXPECT_TRUE.*timeOfClosestApproach" test/OpenSpaceToolkit/Astrodynamics/Access/Generator.test.cppLength of output: 2955
src/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion/AERInterval.cpp
Outdated
Show resolved
Hide resolved
vishwa2710
force-pushed
the
feat/add-constraint
branch
from
5867a96 to
f2b7ff6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp (5)
29-34: Consider adding documentation to the test fixture class.Adding documentation to explain the purpose of the test fixture and its members would improve maintainability.
+/// Test fixture for VisibilityCriterion tests +/// Provides default environment and Earth celestial object class OpenSpaceToolkit_Astrodynamics_Access_VisibilityCriterion : public ::testing::Test { protected: + /// Default environment instance Environment defaultEnvironment_ = Environment::Default(); + /// Shared pointer to Earth celestial object const Shared<const Celestial> defaultEarthSPtr_ = defaultEnvironment_.accessCelestialObjectWithName("Earth"); };
36-194: Enhance test case descriptions for better clarity.While the constructor tests are comprehensive, adding descriptive comments for each test case would make the test intentions clearer.
Example for AERInterval tests:
// FromAERInterval { + // Test case: Valid intervals within bounds { const Interval<Real> azimuthInterval = Interval<Real>::Closed(0.0, 360.0); const Interval<Real> elevationInterval = Interval<Real>::Closed(0.0, 90.0); const Interval<Real> rangeInterval = Interval<Real>::Closed(0.0, 1e6);🧰 Tools
🪛 cppcheck (2.10-2)
[error] 45-45: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
196-298: Consider adding edge cases to operator tests.While the current tests cover basic equality and inequality, consider adding edge cases such as:
- Comparing different types of visibility criteria
- Comparing with slightly different values
- Comparing with empty/undefined values
300-340: Add negative test cases for type checking.Consider adding test cases to verify that
is<T>()returnsfalsewhen checking for incorrect types.Example:
EXPECT_FALSE(visibilityCriterion.is<VisibilityCriterion::AERMask>()); // when type is AERInterval EXPECT_FALSE(visibilityCriterion.is<VisibilityCriterion::LineOfSight>()); // when type is AERInterval
405-484: Consider adding more edge cases to satisfaction tests.While the current tests cover basic satisfaction checks, consider adding:
- Edge cases at interval boundaries
- Cases with NaN or infinite values
- Cases with very small or very large values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp
[error] 45-45: There is an unknown macro here somewhere. Configuration is required. If ; is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test / Check C++ Format
- GitHub Check: Test / Run Python Unit Tests
- GitHub Check: Test / Run C++ Unit Tests
- GitHub Check: Test / Check Python Format
🔇 Additional comments (2)
test/OpenSpaceToolkit/Astrodynamics/Access/VisibilityCriterion.test.cpp (2)
1-28: LGTM!The file structure is well-organized with appropriate includes and using statements.
342-403: LGTM!The type conversion tests are comprehensive and properly verify the converted values.
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
AccessTargetclass to enhance access computation flexibility.VisibilityCriterionwith multiple visibility assessment methods.Improvements
Bug Fixes