feat: lla FromPosition constructor #306
Conversation
WalkthroughThe pull request introduces new methods for converting between Changes
Sequence DiagramsequenceDiagram
participant User
participant Position
participant LLA
participant Celestial
participant Environment
User->>Position: FromLLA(lla, celestial?)
alt celestial not provided
Position->>Environment: getGlobalInstance()
Environment-->>Position: centralBody
end
Position->>Celestial: getEquatorialRadius()
Position->>Celestial: getFlattening()
Position->>Position: Convert LLA to Cartesian
Position-->>User: Return Position
User->>LLA: FromPosition(position, celestial?)
alt celestial not provided
LLA->>Environment: getGlobalInstance()
Environment-->>LLA: centralBody
end
LLA->>Celestial: getEquatorialRadius()
LLA->>Celestial: getFlattening()
LLA->>LLA: Convert Position to LLA
LLA-->>User: Return LLA
Possibly related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
test/Global.test.hpp (1)
16-33: Consolidate common numeric comparison logic.These macros share logic that compares vector sizes and element differences. Consider extracting or refactoring the repeated comparison logic into a shared inline function for DRY (Don't Repeat Yourself) code, though macros can be more convenient in testing contexts.
include/OpenSpaceToolkit/Physics/Coordinate/Position.hpp (1)
166-174: Confirm usage of global environment references.The
FromLLAmethod references the global environment ifaCelestialSPtris null. While convenient, it introduces a hidden dependency that may complicate testing or multi-threaded scenarios if the global environment is modified concurrently.bindings/python/test/coordinate/test_position.py (1)
100-101: Re-evaluate the skipped test.The
test_is_nearmethod is currently skipped. If this test is outdated or incomplete, consider removing the skip or adding the pending scenarios to ensure coverage of thePosition.is_nearmethod.src/OpenSpaceToolkit/Physics/Coordinate/Position.cpp (1)
222-225: Method signature well-intentioned, but please verify parameter naming consistency.
Consider changingaLLAto something likeanLLAorllafor consistent parameter naming across other methods.test/OpenSpaceToolkit/Physics/Coordinate/Position.test.cpp (1)
259-285: Comprehensive coverage of FromLLA.
- Tests undefined LLA → expected Undefined exception.
- Tests missing Celestial → expected runtime error.
- Tests valid Earth pointer → ensures correct Cartesian coords.
- Tests fallback to global environment → ensures correct fallback.
Suggest adding an additional test for lat/long extremes (poles) if feasible.include/OpenSpaceToolkit/Physics/Coordinate/Spherical/LLA.hpp (1)
339-348:FromPositionmethod signature is clear and modular.
- Defaults to global environment’s central celestial if none provided.
- Matches existing pattern in
Position::FromLLA.
Consider clarifying that altitudes can be negative if below reference ellipsoid.bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Spherical/LLA.cpp (1)
417-432: Add a usage note forNoneascelestial.
Everything looks good in this newly introduced static method. However, consider noting in the docstring or code comments that passingNoneforcelestialdefers to the global environment's central celestial object, ensuring future maintainers are crystal-clear about this fallback mechanism.src/OpenSpaceToolkit/Physics/Coordinate/Spherical/LLA.cpp (1)
534-554: Check validity of ellipsoid parameters before converting.
This newFromPositionmethod is well-structured and properly checks for undefined inputs. However, consider verifying that the retrievedequatorialRadiusandflatteningfromcelestialSPtrare themselves defined; while unusual, an undefined radius or flattening might cause downstream runtime issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Position.cpp(9 hunks)bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Spherical/LLA.cpp(1 hunks)bindings/python/test/coordinate/spherical/test_lla.py(2 hunks)bindings/python/test/coordinate/test_position.py(1 hunks)include/OpenSpaceToolkit/Physics/Coordinate/Position.hpp(3 hunks)include/OpenSpaceToolkit/Physics/Coordinate/Spherical/LLA.hpp(3 hunks)src/OpenSpaceToolkit/Physics/Coordinate/Position.cpp(2 hunks)src/OpenSpaceToolkit/Physics/Coordinate/Spherical/LLA.cpp(2 hunks)test/Global.test.hpp(1 hunks)test/OpenSpaceToolkit/Physics/Coordinate/Position.test.cpp(3 hunks)test/OpenSpaceToolkit/Physics/Coordinate/Spherical/LLA.test.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (2)
include/OpenSpaceToolkit/Physics/Coordinate/Position.hpp (1)
Learnt from: vishwa2710
PR: open-space-collective/open-space-toolkit-physics#278
File: src/OpenSpaceToolkit/Physics/Environment.cpp:46-46
Timestamp: 2024-11-12T02:57:39.443Z
Learning: In the `Environment` class, `aCentralCelestialObject` is a shared pointer to a `const Object`, ensuring immutability and preventing shared mutability concerns when assigning to `centralCelestialObject_`.
include/OpenSpaceToolkit/Physics/Coordinate/Spherical/LLA.hpp (1)
Learnt from: vishwa2710
PR: open-space-collective/open-space-toolkit-physics#278
File: src/OpenSpaceToolkit/Physics/Environment.cpp:46-46
Timestamp: 2024-11-12T02:57:39.443Z
Learning: In the `Environment` class, `aCentralCelestialObject` is a shared pointer to a `const Object`, ensuring immutability and preventing shared mutability concerns when assigning to `centralCelestialObject_`.
🔇 Additional comments (20)
test/Global.test.hpp (1)
35-53: Be mindful of test severity differences.Using
FAIL()instead ofADD_FAILURE()inASSERT_VECTORS_ALMOST_EQUALcan halt test execution, which makes sense for critical mismatches. Ensure this approach aligns with your testing philosophy, as it may prevent subsequent checks from running in the same test.bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Position.cpp (1)
202-217: Ensure robust error handling when celestial is undefined.The
from_llamethod uses a defaultNonefor the celestial parameter, relying on the global environment if not provided. Consider explicitly raising an error if no valid celestial can be determined at runtime, to prevent silent failures or unexpected defaults.src/OpenSpaceToolkit/Physics/Coordinate/Position.cpp (5)
10-12: Includes look appropriate.
The newly added headers are necessary for the LLA and Celestial object references.
226-229: Good validation for undefined LLA.
Throwing an exception is consistent with the rest of the codebase’s approach.
231-233: Verify fallback logic for celestial object.
Ensuring we retrieve the global environment's central celestial object is a good fallback.
235-238: Check for potential race conditions in retrieving global environment.
If the global environment could be reset in parallel, consider a locking mechanism or more robust check.Would you like a shell script that searches for concurrent environment resets in the codebase?
240-242: Conversion logic appears correct and consistent with ITRF usage.
Given that altitudes can be negative, confirm that Earth flattening is accurate across negative altitudes as well.test/OpenSpaceToolkit/Physics/Coordinate/Position.test.cpp (3)
5-6: New includes for Environment and Earth recognized.
They are essential for the new tests targeting the global environment usage.
17-19: Updated using statements look fine.
BringingLLA,Environment, andEarthinto scope is aligned with your new tests.
257-258: Test name is well chosen.
“FromLLA” indicates coverage of the new functionality.bindings/python/test/coordinate/spherical/test_lla.py (5)
11-13: Imports are consistent and necessary.
They align with the new test methods referencing Position and Earth.
494-498: Setup for test_from_position is clear.
These lines define the Earth model, frame, and a sample equatorial position.
499-507: Validation of resulting LLA correctness is thorough.
- Latitude near zero → equator.
- Longitude near zero → prime meridian.
- Altitude near zero → up close to Earth's surface.
508-511: Undefined position test is valuable.
It confirms the method raises a runtime error whenPositionlacks definition.
512-514: Global environment test is well-structured.
This ensures coverage of the optional celestial pointer fallback.include/OpenSpaceToolkit/Physics/Coordinate/Spherical/LLA.hpp (4)
9-9: Added<OpenSpaceToolkit/Core/Type/Shared.hpp>is appropriate.
This is essential for the newSharedpointer usage.
23-31: New forward declarations for Celestial object.
This reduces overall compile times and is consistent with the environment folder structure.
34-36: Forward declaration ofPositionfosters loose coupling.
Avoiding direct includes helps keep compile units small.
43-43: AddedSharedalias similarly used across codebase.
This matches code style in other classes.test/OpenSpaceToolkit/Physics/Coordinate/Spherical/LLA.test.cpp (1)
1107-1139: Thorough test coverage forFromPosition.
Great job covering all relevant scenarios: valid conversion, undefined position, and null celestial with or without a global environment. Tests appear robust and validate the new method extensively.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/position-from-lla #306 +/- ##
==========================================================
+ Coverage 82.87% 82.89% +0.01%
==========================================================
Files 102 102
Lines 8019 8031 +12
==========================================================
+ Hits 6646 6657 +11
- Misses 1373 1374 +1 ☔ View full report in Codecov by Sentry. |
* feat: Add FromPosition static constructor to LLA class * feat: Add FromPosition test case for LLA coordinate conversion * feat: Add FromPosition method tests for LLA Python bindings * test: Add FromPosition test for LLA coordinate conversion * feat: add tests for from position
* feat: FromLLA constructor for position * chore: style * feat: lla FromPosition constructor (#306) * feat: Add FromPosition static constructor to LLA class * feat: Add FromPosition test case for LLA coordinate conversion * feat: Add FromPosition method tests for LLA Python bindings * test: Add FromPosition test for LLA coordinate conversion * feat: add tests for from position
Summary by CodeRabbit
New Features
from_llamethod toPositionclass for creating position from latitude, longitude, and altitudefrom_positionmethod toLLAclass for converting position to latitude, longitude, and altitudeTests
Documentation