refactor!: rename nadir_pointing profile to local_orbital_frame #484
Conversation
|
Warning Rate limit exceeded@vishwa2710 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 30 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (10)
WalkthroughThe pull request introduces a comprehensive renaming of the Changes
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: 0
🧹 Nitpick comments (7)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile/Model/Transform.cpp (1)
130-131: Update docstring references to reflect 'local orbital frame' instead of 'nadir pointing'.The new method name is
local_orbital_frame, but the docstring still describes "nadir pointing." Consider updating it to match the new concept:R"doc( - Create a transform for nadir pointing. + Create a transform for a local orbital frame. )doc"src/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Model/Transform.cpp (1)
167-167: Consider clarifying the reasoning and usage of the local orbital frame.The rename from
NadirPointingtoLocalOrbitalFramemakes the intent clearer, but it may be beneficial to provide in-code comments or docstrings explaining how the frames are derived and how this differs from a strict nadir orientation.test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Transform.test.cpp (2)
104-104: Avoid shadowing variable names.Here,
transform_is declared in the parent scope and shadowed inside this function’s scope. Consider using a different local variable name to improve clarity, e.g.localTransform.- EXPECT_NO_THROW(Transform transform_(DynamicProvider(dynamicProviderGenerator), Frame::GCRF())); + EXPECT_NO_THROW(Transform localTransform(DynamicProvider(dynamicProviderGenerator), Frame::GCRF()));
212-215: Confirm correct initialization of LocalOrbitalFrame in tests.The test successfully checks
LocalOrbitalFramecreation. If additional orbital frame types are supported, consider adding a parameterized test case for each type to strengthen coverage.bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile.cpp (1)
324-325: Update docstring to reflect LocalOrbitalFrame naming.The docstring still mentions “nadir pointing.” Please rename it to reflect the local orbital frame context.
"local_orbital_frame", &Profile::LocalOrbitalFrame, - R"doc( - Create a nadir pointing profile. + R"doc( + Create a local orbital frame profile. ... )doc",src/OpenSpaceToolkit/Astrodynamics/Flight/Profile.cpp (1)
219-223: Consider validating theOrbitparameter before returning aProfile.
IfanOrbitis undefined at runtime,TransformModel::LocalOrbitalFramemay raise an exception or produce an invalidProfile. A basic check here, similar to other constructors in this file, would enhance reliability.test/OpenSpaceToolkit/Astrodynamics/Flight/Profile.test.cpp (1)
413-413: Confirm coverage of eccentricities & inclinations.
Although this test uses a circular (0.0 eccentricity), low-inclination orbit, consider adding more diverse scenarios to ensureLocalOrbitalFramehandles varied orbital parameters reliably.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile/Model/Transform.cpp(1 hunks)bindings/python/test/flight/test_profile.py(1 hunks)bindings/python/test/test_viewer.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Profile.hpp(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Model/Transform.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Profile.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Model/Transform.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Profile.test.cpp(8 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Transform.test.cpp(3 hunks)
🔇 Additional comments (12)
include/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Model/Transform.hpp (1)
79-79: Name change aligns with the PR's objective.The rename from
NadirPointingtoLocalOrbitalFrameimproves semantic clarity. Ensure that the documentation (comments and user guides) is updated to reflect the new naming convention everywhere in the codebase.bindings/python/test/test_viewer.py (1)
41-41: Local orbital frame usage is correctly updated.The fixture now aligns with the new naming scheme. This looks good. For extra robustness, consider adding tests for additional
Orbit.FrameTypevariants to ensure broader coverage of local orbital frame usage.bindings/python/test/flight/test_profile.py (1)
195-199: Ensure method renaming is fully reflected in docstrings and references.The test method now appropriately references
local_orbital_frameinstead ofnadir_pointing. However, please ensure that any related docstrings and references throughout the file and project are fully updated to avoid confusion.test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Transform.test.cpp (1)
79-79: Confirm consistent usage of LocalOrbitalFrame.Renaming the transform to
LocalOrbitalFrameis consistent with the broader refactoring. Verify that references toNadirPointingare fully removed across the project to prevent confusion.include/OpenSpaceToolkit/Astrodynamics/Flight/Profile.hpp (1)
295-295: Confirm the rename aligns with codebase references.Renaming
NadirPointingtoLocalOrbitalFramereflects the PR objective. Ensure all references, including documentation and call sites, are updated.✅ Verification successful
Let me search for any implementation or usage of this method to ensure we catch all references.
Let me check the test file specifically for NadirPointing references.
Based on the search results, I can see that:
The test file still contains references to "NadirPointing" in test names and scenario paths, but these appear to be test data files and not actual implementation code.
The implementation has been updated to use "LocalOrbitalFrame" consistently in the actual code.
The test file shows that the functionality previously tested under "NadirPointing" is now being tested under "LocalOrbitalFrame" with the same test cases and validation data.
Let me verify if there are any other implementation files that need to be checked.
Based on the comprehensive search results, I can now provide a final response:
The rename from
NadirPointingtoLocalOrbitalFramehas been properly implemented in the codebaseThe only remaining references to "NadirPointing" are in:
- Test data files and paths under
test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/NadirPointing/- A single test case name
TEST_F(OpenSpaceToolkit_Astrodynamics_Flight_Profile, NadirPointing_VVLH)These references are in test artifacts and don't affect the actual implementation. The core functionality has been properly renamed to
LocalOrbitalFramein:
- Method declarations and definitions
- Documentation
- Python bindings
- All implementation files
The test data can remain as-is since it's historical validation data, and the test name could be updated in a future cleanup but doesn't impact functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searches for stale references to 'NadirPointing' in the codebase. rg "NadirPointing" -A 5Length of output: 4671
Script:
#!/bin/bash # Search for implementation and usage of LocalOrbitalFrame rg "LocalOrbitalFrame" -A 5 # Search for test files that might need updates fd "Profile.test" --exec cat {}Length of output: 90434
Script:
#!/bin/bash # Search for implementation and test files related to Profile fd "Profile\.(hpp|cpp|test\.cpp)$" --exec cat {}Length of output: 70653
Script:
#!/bin/bash # Search for implementation files fd "Profile\.(cpp|hpp)$" --type f --exec cat {}Length of output: 43609
test/OpenSpaceToolkit/Astrodynamics/Flight/Profile.test.cpp (7)
106-106: Great update to use the renamed method.
Switching fromNadirPointingtoLocalOrbitalFramelooks correct. The orbit is defined at this point, so this usage should be valid.
423-423: Kudos for updating reference data path.
Ensuring that the path now matches the new method nameLocalOrbitalFrameis consistent with the rename.
523-523: Reiterating coverage for inclination changes.
This test method already showcases an inclined orbit (45°). This helps confirm thatLocalOrbitalFrameis tested with non-zero inclination.
533-533: Keeping reference data updated is crucial.
This line properly updates the expected CSV reference dataset. Good job in aligning test artifacts with the rename.
633-633: Confirm coverage for elliptical orbits.
This test uses non-zero eccentricity (0.05), verifying the new method with an elliptical orbit scenario, which is beneficial.
643-643: Updated reference data path for elliptical orbit.
The CSV file path now reflects the rename, ensuring consistency in test artifacts.
991-991: Nice consistency check withVNCframe compare.
ComparingLocalOrbitalFramevs. a knownVNCorientation ensures correctness. These lines are particularly insightful to validate alignment.
Yeah we could provide a warning in this MR and then deprecate it in an upcoming patch. That's best. Will do! |
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile/Model/Transform.cpp
Outdated
Show resolved
Hide resolved
Actually, upon further thought, since there are other breaking changes in this upcoming release, I would like to just club this together to simplify propagating the changes across downstream dependencies. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile/Model/Transform.cpp (1)
130-140: LGTM! Consider enhancing the documentation.The renaming from
nadir_pointingtolocal_orbital_frame_pointingbetter reflects the functionality.Consider enhancing the documentation by:
- Adding examples of available orbital frame types
- Explaining the differences between frame types
- Adding a code example showing typical usage
R"doc( Create a profile aligned with the provided local orbital frame type. Args: orbit (Orbit): The orbit of the satellite. orbital_frame_type (OrbitalFrameType): The type of the orbital frame. + Available types: + - LVLH (Local Vertical Local Horizontal) + - QSW (Quasi-Satellite Wing) + - TNW (Tangential Normal Wing) + For details on frame differences, see: <link_to_documentation> + + Example: + >>> profile = Transform.local_orbital_frame_pointing( + ... orbit=satellite_orbit, + ... orbital_frame_type=OrbitalFrameType.LVLH + ... ) Returns: Transform: The transform for the local orbital frame pointing. )doc",bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile.cpp (1)
324-334: LGTM! Consider enhancing the documentation.The renaming is consistent with the changes in other files and maintains API coherence.
Consider enhancing the documentation by adding a usage example:
R"doc( Create a profile aligned with the provided local orbital frame type. Args: orbit (Orbit): The orbit. orbital_frame_type (OrbitalFrameType): The type of the orbital frame. + + Example: + >>> # Create a profile using LVLH frame + >>> profile = Profile.local_orbital_frame_pointing( + ... orbit=satellite_orbit, + ... orbital_frame_type=OrbitalFrameType.LVLH + ... ) + >>> # Get the state at a specific instant + >>> state = profile.get_state_at(instant) Returns: Profile: The profile aligned with the local orbital frame. )doc",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile.cpp(1 hunks)bindings/python/src/OpenSpaceToolkitAstrodynamicsPy/Flight/Profile/Model/Transform.cpp(2 hunks)bindings/python/test/test_viewer.py(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Profile.hpp(1 hunks)include/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Model/Transform.hpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Profile.cpp(1 hunks)src/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Model/Transform.cpp(1 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Profile.test.cpp(9 hunks)test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Transform.test.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- bindings/python/test/test_viewer.py
- include/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Model/Transform.hpp
- include/OpenSpaceToolkit/Astrodynamics/Flight/Profile.hpp
- src/OpenSpaceToolkit/Astrodynamics/Flight/Profile.cpp
- test/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Transform.test.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test / Run C++ Unit Tests
🔇 Additional comments (6)
src/OpenSpaceToolkit/Astrodynamics/Flight/Profile/Model/Transform.cpp (1)
Line range hint
167-193: LGTM! Implementation is robust and consistent.The implementation correctly handles error cases and maintains proper functionality while being renamed to better reflect its purpose.
test/OpenSpaceToolkit/Astrodynamics/Flight/Profile.test.cpp (5)
106-106: LGTM! Setup correctly updated to use LocalOrbitalFramePointing.The initialization of
profile_has been properly updated to use the new naming while maintaining the same functionality.
385-385: LGTM! Test case name updated to reflect new terminology.The test case has been appropriately renamed to
LocalOrbitalFramePointing_VVLH, which better describes the functionality being tested.
Line range hint
413-425: LGTM! Reference data paths updated consistently.The reference data file paths have been updated to reflect the new naming scheme while maintaining the same directory structure and naming convention.
Also applies to: 524-536, 635-647
994-994: LGTM! Test comparison updated to use LocalOrbitalFramePointing.The expected profile creation has been properly updated to use the new naming while maintaining the same functionality.
Line range hint
1-1200: Verify complete removal of old terminology.Let's verify that all instances of the old terminology have been replaced.
✅ Verification successful
All instances of
NadirPointinghave been successfully replacedThe codebase has been thoroughly checked and no instances of the old terminology remain. The new term
LocalOrbitalFramePointingis consistently used across source files, headers, tests, and bindings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of "NadirPointing" in the codebase # that might have been missed during the rename. echo "Searching for any remaining instances of 'NadirPointing'..." rg --type cpp "NadirPointing" || true echo "Verifying all instances have been replaced with 'LocalOrbitalFramePointing'..." rg --type cpp "LocalOrbitalFramePointing"Length of output: 3209
vishwa2710
force-pushed
the
480-refactor-rename-the-profilenadirpointing-to-profilelocalorbitalframe
branch
from
d4e3e62 to
63ce283
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #484 +/- ##
=======================================
Coverage 91.21% 91.21%
=======================================
Files 86 86
Lines 8720 8720
=======================================
Hits 7954 7954
Misses 766 766 ☔ View full report in Codecov by Sentry. |
vishwa2710
deleted the
480-refactor-rename-the-profilenadirpointing-to-profilelocalorbitalframe
branch
Summary by CodeRabbit
Release Notes
Refactor
NadirPointingmethod toLocalOrbitalFramePointingacross multiple components of the OpenSpaceToolkit Astrodynamics library.Tests
LocalOrbitalFramePointingmethod.