feat!: add overwrite option to Frame construction method#349
Open
loft-nicolas-sanchez wants to merge 5 commits intomainfrom
Open
feat!: add overwrite option to Frame construction method#349loft-nicolas-sanchez wants to merge 5 commits intomainfrom
loft-nicolas-sanchez wants to merge 5 commits intomainfrom
Conversation
WalkthroughAdds an optional bool parameter Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller (C++ / Python)
participant FrameClass as Frame::Construct
participant Registry as Frame Registry
participant Emplace as Emplace()
Caller->>FrameClass: Construct(name, is_quasi_inertial, parent, provider, overwrite)
FrameClass->>Registry: Lookup(name)
alt name exists
alt overwrite == true
FrameClass->>Registry: Remove(existing_frame)
FrameClass->>Emplace: Emplace(name, ..., provider, overwrite)
Emplace-->>Registry: Insert(new_frame)
FrameClass-->>Caller: Return new_frame
else overwrite == false
FrameClass-->>Caller: Throw runtime_error
end
else name not found
FrameClass->>Emplace: Emplace(name, ..., provider, overwrite)
Emplace-->>Registry: Insert(new_frame)
FrameClass-->>Caller: Return new_frame
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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 |
![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 (4)
test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp (1)
745-767: Strengthen overwrite test: also assert name lookup and manager stateGood coverage. Add a couple of cheap checks to tighten guarantees:
- WithName should resolve to the replaced instance.
- Exists should remain true after replacement.
Example patch inside this block:
// Verify the new frame has the expected parent (ITRF instead of GCRF) EXPECT_EQ(Frame::ITRF(), secondFrameSPtr->accessParent()); + // Manager lookup should resolve to the replaced instance + EXPECT_EQ(secondFrameSPtr, Frame::WithName(name)); + EXPECT_TRUE(Frame::Exists(name)); + + // Optional: warm a cache before overwrite and ensure subsequent lookups use the new instance. + // (Consider adding a separate test that computes a transform with the first instance, + // then overwrites and recomputes to guard against stale-cache issues.) Frame::Destruct(name);src/OpenSpaceToolkit/Physics/Coordinate/Frame.cpp (1)
429-436: Minor: fix error message grammar“already exist.” → “already exists.”
- throw ostk::core::error::RuntimeError("Frame with name [{}] already exist.", aName); + throw ostk::core::error::RuntimeError("Frame with name [{}] already exists.", aName);bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Frame.cpp (1)
345-365: Expose a short caution in the Python docstring about active referencesOverwriting while holding references to the previous instance can lead to two live frames with the same name (the old one unregistered). Add a brief note so users know to reacquire via Frame.with_name(name) after overwrite.
"construct", &Frame::Construct, arg("name"), arg("is_quasi_inertial"), arg("parent_frame"), arg("provider"), arg("overwrite") = false, R"doc( Construct a frame. Args: name (String): Name. is_quasi_inertial (bool): True if quasi-inertial. parent_frame (Frame): Parent frame. provider (Provider): Provider. - overwrite (bool): True to overwrite existing frame with same name (default: False). + overwrite (bool): True to overwrite existing frame with same name (default: False). + + Note: + If you pass overwrite=True, any existing references to the previous frame object + remain valid but are no longer registered in the manager. Reacquire the frame via + Frame.with_name(name) to get the replaced instance. )doc"include/OpenSpaceToolkit/Physics/Coordinate/Frame.hpp (1)
111-116: Fix param doc typos and clarify semantics
- “isQuasiInertialT True is frame is quasi-inertial” → “isQuasiInertial True if frame is quasi-inertial”.
- Mention overwrite default and behavior.
- /// @param [in] isQuasiInertialT True is frame is quasi-inertial + /// @param [in] isQuasiInertial True if frame is quasi-inertial /// @param [in] aParentFrame A shared pointer to the parent frame /// @param [in] aProvider A shared pointer to the transform provider - /// @param [in] overwrite True to overwrite existing frame with same name + /// @param [in] overwrite True to overwrite existing frame with same name (default: false). + /// Note: existing references to the previous instance remain valid but unregistered; + /// callers should reacquire the frame via WithName(aName) after overwriting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Frame.cpp(2 hunks)include/OpenSpaceToolkit/Physics/Coordinate/Frame.hpp(1 hunks)src/OpenSpaceToolkit/Physics/Coordinate/Frame.cpp(1 hunks)test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp (1)
src/OpenSpaceToolkit/Physics/Coordinate/Frame.cpp (8)
Construct(419-440)Construct(419-425)GCRF(298-305)GCRF(298-298)ITRF(385-392)ITRF(385-385)Destruct(442-452)Destruct(442-442)
src/OpenSpaceToolkit/Physics/Coordinate/Frame.cpp (1)
include/OpenSpaceToolkit/Physics/Coordinate/Frame.hpp (5)
aName(105-105)aName(107-107)aName(117-123)aName(125-125)aName(147-152)
🔇 Additional comments (1)
src/OpenSpaceToolkit/Physics/Coordinate/Frame.cpp (1)
419-440: Cache invalidation verified—review concern already addressed in codeThe
FrameManager::removeFrameWithName()implementation (lines 81–118 of Manager.cpp) properly purges all cached state:
- Bidirectional transform cache cleanup: Entries where the removed frame is the source (line 97) and all entries where it is the destination (lines 100–108) are erased.
- Name→frame mapping removal:
frameMap_.erase()prevents name collisions (line 112).- Mutex protection: All operations are guarded by
std::lock_guard.- Only cache structures:
frameMap_andtransformCache_are the sole caches; both are fully invalidated.The
Construct()method correctly invokes this cleanup whenoverwrite=true(line 431). No stale entries or dangling references will persist.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 83.29% 83.31% +0.02%
==========================================
Files 101 101
Lines 8199 8201 +2
==========================================
+ Hits 6829 6833 +4
+ Misses 1370 1368 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
apaletta3
approved these changes
Oct 28, 2025
bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Frame.cpp
Outdated
Show resolved
Hide resolved
bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Frame.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Antoine Paletta <98616558+apaletta3@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp (1)
746-767: Verify that the parent frame was actually replaced.The test confirms the frame exists by name after overwrite, but doesn't verify that the parent changed from GCRF to ITRF. Add an assertion to confirm the replacement worked correctly.
Apply this diff to verify the parent frame:
// Verify the new frame has the expected parent (ITRF instead of GCRF) EXPECT_TRUE(Manager::Get().hasFrameWithName(name)); + EXPECT_EQ(Frame::ITRF(), Frame::WithName(name)->accessParent()); Frame::Destruct(name);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Frame.cpp(2 hunks)include/OpenSpaceToolkit/Physics/Coordinate/Frame.hpp(1 hunks)test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/OpenSpaceToolkit/Physics/Coordinate/Frame.hpp
🧰 Additional context used
🧬 Code graph analysis (1)
test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp (2)
src/OpenSpaceToolkit/Physics/Coordinate/Frame.cpp (8)
Construct(419-440)Construct(419-425)GCRF(298-305)GCRF(298-298)ITRF(385-392)ITRF(385-385)Destruct(442-452)Destruct(442-442)src/OpenSpaceToolkit/Physics/Coordinate/Frame/Manager.cpp (2)
Get(143-170)Get(143-143)
bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Frame.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
vishwa2710
approved these changes
Oct 28, 2025
Contributor
vishwa2710
left a comment
vishwa2710
left a comment
There was a problem hiding this comment.
Looks great, I believe this should be a breaking change, as it changes the ABI. We are planning on doing a breaking release for OSTk soon so we can bundle it in with that. Let's update the description to be feat!
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 (1)
test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-03T20:32:59.766Z
Learnt from: vishwa2710
PR: open-space-collective/open-space-toolkit-physics#289
File: src/OpenSpaceToolkit/Physics/Coordinate/Frame/Provider/IERS/Manager.cpp:256-256
Timestamp: 2024-11-03T20:32:59.766Z
Learning: In the `Manager` class (`src/OpenSpaceToolkit/Physics/Coordinate/Frame/Provider/IERS/Manager.cpp`), the `BaseManager` handles mutex locking, so additional mutex locks in overridden methods like `reset` are not necessary.
Applied to files:
test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp
🧬 Code graph analysis (1)
test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp (2)
src/OpenSpaceToolkit/Physics/Coordinate/Frame/Manager.cpp (3)
Manager(172-175)Get(143-170)Get(143-143)src/OpenSpaceToolkit/Physics/Coordinate/Frame.cpp (4)
Construct(419-440)Construct(419-425)Destruct(442-452)Destruct(442-442)
⏰ 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). (4)
- GitHub Check: Test / Build Python bindings
- GitHub Check: Test / Check Python Format
- GitHub Check: Test / Check C++ Format
- GitHub Check: Test / Run C++ Unit Tests
🔇 Additional comments (2)
test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp (2)
7-7: LGTM!The Manager header include is necessary for the new test case that verifies frame overwrite behavior.
26-26: LGTM!The using declaration follows the existing pattern and is necessary for the test case.
Comment on lines
+747
to
+769
|
|
||
| // Test overwrite functionality | ||
| { | ||
| const String name = "Custom B"; | ||
|
|
||
| // Create first frame | ||
| const Shared<const Frame> firstFrameSPtr = | ||
| Frame::Construct(name, isQuasiInertial_, Frame::GCRF(), providerSPtr_); | ||
| EXPECT_TRUE(firstFrameSPtr->isDefined()); | ||
|
|
||
| // Attempt to create another frame with same name without overwrite - should throw | ||
| EXPECT_ANY_THROW(Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, false)); | ||
|
|
||
| // Create another frame with same name with overwrite - should succeed | ||
| const Shared<const Frame> secondFrameSPtr = | ||
| Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, true); | ||
| EXPECT_TRUE(secondFrameSPtr->isDefined()); | ||
|
|
||
| // Verify the new frame has the expected parent (ITRF instead of GCRF) | ||
| EXPECT_TRUE(Manager::Get().hasFrameWithName(name)); | ||
|
|
||
| Frame::Destruct(name); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Verify the frame was actually replaced with the new parent.
The test correctly validates that overwrite throws when false and succeeds when true, but line 766 only checks that a frame with the name exists—it doesn't confirm the frame was replaced with the new properties (ITRF parent instead of GCRF).
Add verification that the parent frame changed:
// Verify the new frame has the expected parent (ITRF instead of GCRF)
EXPECT_TRUE(Manager::Get().hasFrameWithName(name));
+ EXPECT_EQ(Frame::ITRF(), Frame::WithName(name)->accessParent());
Frame::Destruct(name);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Test overwrite functionality | |
| { | |
| const String name = "Custom B"; | |
| // Create first frame | |
| const Shared<const Frame> firstFrameSPtr = | |
| Frame::Construct(name, isQuasiInertial_, Frame::GCRF(), providerSPtr_); | |
| EXPECT_TRUE(firstFrameSPtr->isDefined()); | |
| // Attempt to create another frame with same name without overwrite - should throw | |
| EXPECT_ANY_THROW(Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, false)); | |
| // Create another frame with same name with overwrite - should succeed | |
| const Shared<const Frame> secondFrameSPtr = | |
| Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, true); | |
| EXPECT_TRUE(secondFrameSPtr->isDefined()); | |
| // Verify the new frame has the expected parent (ITRF instead of GCRF) | |
| EXPECT_TRUE(Manager::Get().hasFrameWithName(name)); | |
| Frame::Destruct(name); | |
| } | |
| // Test overwrite functionality | |
| { | |
| const String name = "Custom B"; | |
| // Create first frame | |
| const Shared<const Frame> firstFrameSPtr = | |
| Frame::Construct(name, isQuasiInertial_, Frame::GCRF(), providerSPtr_); | |
| EXPECT_TRUE(firstFrameSPtr->isDefined()); | |
| // Attempt to create another frame with same name without overwrite - should throw | |
| EXPECT_ANY_THROW(Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, false)); | |
| // Create another frame with same name with overwrite - should succeed | |
| const Shared<const Frame> secondFrameSPtr = | |
| Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, true); | |
| EXPECT_TRUE(secondFrameSPtr->isDefined()); | |
| // Verify the new frame has the expected parent (ITRF instead of GCRF) | |
| EXPECT_TRUE(Manager::Get().hasFrameWithName(name)); | |
| EXPECT_EQ(Frame::ITRF(), Frame::WithName(name)->accessParent()); | |
| Frame::Destruct(name); | |
| } |
🤖 Prompt for AI Agents
In test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp around lines 747 to
769, after creating secondFrameSPtr with overwrite=true you need to verify the
replacement actually changed the parent: obtain the frame instance from the
manager (or use secondFrameSPtr) and assert its parent is ITRF (not GCRF);
replace the loose Manager::Get().hasFrameWithName(name) check with or add an
EXPECT_TRUE/EXPECT_EQ that the retrieved frame's parent equals Frame::ITRF() to
confirm the overwrite updated the parent frame.
loft-nicolas-sanchez
changed the title
Contributor
|
Waiting to be merged due to ABI break |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior
Tests