Merge release/v1.0.0 to develop
#4849
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/packages/operators/src/built-in-operators.ts (1)
1293-1314: LGTM! Consider clarifying UI visibility.The implementation of the
SetGroupSliceclass looks good and follows the established pattern for operators. The logic is sound, and all necessary methods are implemented correctly.Consider uncommenting the
unlisted: trueproperty in theconfigmethod if this operator is intended to be hidden from the UI. If it should be visible, you can remove the commented line entirely for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/packages/operators/src/built-in-operators.ts (2 hunks)
- fiftyone/core/dataset.py (1 hunks)
- fiftyone/operators/builtin.py (2 hunks)
- fiftyone/operators/operations.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/operators/src/built-in-operators.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (3)
fiftyone/operators/operations.py (1)
637-643: LGTM: New methodset_group_sliceis well-implemented.The new
set_group_slicemethod is correctly implemented and follows the existing patterns in theOperationsclass. The docstring provides clear information about the method's purpose and parameters.app/packages/operators/src/built-in-operators.ts (1)
1362-1363: LGTM! New operators registered correctly.The
SetGroupSlice,SetPlayheadState, andSetFrameNumberoperators have been correctly registered as built-in operators. This addition follows the established pattern and integrates well with the existing code.fiftyone/core/dataset.py (1)
576-579: Improved error handling for group slice settingThis change enhances the
group_slicesetter by only raising aValueErrorwhenslice_nameis notNoneand not present inself._doc.group_media_types. This allows settinggroup_slicetoNonewithout raising an error, which is useful for resetting to the default group slice.
|
|
||
| # @todo ideally we would only set this if the group slice we renamed is | ||
| # currently loaded in the App | ||
| ctx.ops.set_group_slice(ctx.dataset.default_group_slice) | ||
|
|
||
| ctx.ops.reload_dataset() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the group slice update logic.
The added code sets the default group slice and reloads the dataset after renaming a group slice. This ensures that the application state is updated, which is good. However, there's room for improvement:
-
The TODO comment suggests that this operation should ideally be conditional, only updating if the renamed group slice is currently loaded in the App. Implementing this condition could optimize performance by avoiding unnecessary updates.
-
The use of
ctx.ops.set_group_slice()followed byctx.ops.reload_dataset()might cause two separate update operations. Consider if these can be combined into a single, more efficient update.
Consider implementing the conditional logic mentioned in the TODO comment to optimize performance. You might want to add a check like this:
if ctx.current_group_slice == name:
ctx.ops.set_group_slice(new_name)
ctx.ops.reload_dataset()This would ensure that the update only occurs when necessary, potentially improving the efficiency of the operation.
|
|
||
| # @todo ideally we would only set this if the group slice we deleted is | ||
| # currently loaded in the App | ||
| ctx.ops.set_group_slice(ctx.dataset.default_group_slice) | ||
|
|
||
| ctx.ops.reload_dataset() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize group slice update after deletion.
The added code sets the default group slice and reloads the dataset after deleting a group slice. While this ensures that the application state is updated, there are some considerations:
-
The TODO comment suggests that this operation should ideally be conditional, only updating if the deleted group slice was currently loaded in the App. Implementing this condition could optimize performance by avoiding unnecessary updates.
-
The use of
ctx.ops.set_group_slice()followed byctx.ops.reload_dataset()might cause two separate update operations. Consider if these can be combined into a single, more efficient update. -
The logic here is very similar to the one in the
RenameGroupSliceclass. Consider refactoring this common functionality into a separate method to improve code reusability and maintainability.
Consider implementing the conditional logic mentioned in the TODO comment and refactoring the common functionality. Here's a suggestion:
def _update_group_slice_if_needed(ctx, deleted_slice_name):
if ctx.current_group_slice == deleted_slice_name:
ctx.ops.set_group_slice(ctx.dataset.default_group_slice)
ctx.ops.reload_dataset()
class DeleteGroupSlice(foo.Operator):
# ... existing code ...
def execute(self, ctx):
name = ctx.params["name"]
ctx.dataset.delete_group_slice(name)
_update_group_slice_if_needed(ctx, name)
class RenameGroupSlice(foo.Operator):
# ... existing code ...
def execute(self, ctx):
name = ctx.params["name"]
new_name = ctx.params["new_name"]
ctx.dataset.rename_group_slice(name, new_name)
_update_group_slice_if_needed(ctx, name)This refactoring would improve code reusability and ensure consistent behavior between renaming and deleting operations.
Merge
release/v1.0.0todevelopSummary by CodeRabbit
New Features
Bug Fixes
Improvements