add set_group_slice operator #4844
Conversation
|
Warning Rate limit exceeded@brimoor has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 41 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. WalkthroughThe pull request introduces a new Changes
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (3)
fiftyone/operators/operations.py (1)
632-638: LGTM! Consider enhancing the docstring.The implementation of
set_group_slicelooks good and aligns with the PR objectives. It follows the pattern of other methods in the class for triggering events.Consider enhancing the docstring to provide more context about what a "group slice" is and how it relates to group operations. This would improve the method's self-documentation. For example:
def set_group_slice(self, slice): """Set the group slice in the App. This method is used to update the current group slice, which is crucial for operations like deleting or renaming groups. Args: slice: the group slice to set, typically a dict containing group information """ return self._ctx.trigger("set_group_slice", {"slice": slice})app/packages/operators/src/built-in-operators.ts (1)
1228-1249: LGTM! Consider adding error handling and documentation.The
SetGroupSliceclass implementation looks good and follows the pattern of other operators. However, consider the following improvements:
- Add error handling in the
executemethod to catch and handle potential exceptions.- Add JSDoc comments to explain the purpose of the class and its methods, especially the concept of a "group slice".
- Decide whether this operator should be listed or unlisted in the UI (the commented out
unlisted: truein the config method).Here's a suggested improvement for the
executemethod:async execute(ctx: ExecutionContext): Promise<void> { try { const { slice } = ctx.params; if (typeof slice !== 'string' || slice.trim() === '') { throw new Error('Invalid slice parameter'); } ctx.hooks.setSlice(slice); } catch (error) { console.error('Error in SetGroupSlice execute:', error); throw error; // or handle it as appropriate for your application } }fiftyone/operators/builtin.py (1)
1568-1570: Enhanced group slice handling after deletion with a suggestionThe changes in the
executemethod ofDeleteGroupSliceclass significantly improve the handling of group slices after deletion. By selecting the first available group slice and setting it as the current one before reloading the dataset, it ensures that the UI always has a valid group slice selected. This change aligns well with the PR objectives of enhancing group operations functionality.However, consider handling the edge case where all group slices are deleted. You might want to add a check to ensure
ctx.dataset.group_slicesis not empty before accessing its first element.Consider adding a check for empty group slices:
-first_group_slice = ctx.dataset.group_slices[0] -ctx.ops.set_group_slice(first_group_slice) -ctx.ops.reload_dataset() +if ctx.dataset.group_slices: + first_group_slice = ctx.dataset.group_slices[0] + ctx.ops.set_group_slice(first_group_slice) +else: + # Handle the case when all group slices are deleted + # For example, you might want to set a default view or clear the current slice + ctx.ops.set_group_slice(None) +ctx.ops.reload_dataset()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/packages/operators/src/built-in-operators.ts (2 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 not posted (2)
app/packages/operators/src/built-in-operators.ts (1)
1297-1297: LGTM! SetGroupSlice operator successfully registered.The addition of
_registerBuiltInOperator(SetGroupSlice);to theregisterBuiltInOperatorsfunction is correct and follows the established pattern for registering new operators. This ensures that the newSetGroupSliceoperator will be available for use in the application.fiftyone/operators/builtin.py (1)
1521-1522: Improved group slice handling after renamingThe changes in the
executemethod ofRenameGroupSliceclass enhance the handling of group slices after renaming. By directly setting the group slice to the new name and then reloading the dataset, it ensures that the UI is updated with the correct group slice selection. This change provides more precise control and aligns well with the PR objectives of improving group operations functionality.
imanjra
force-pushed
the
bugfix/grp-slice-op
branch
from
a0c4d01
to
fc22e3a
Compare
What changes are proposed in this pull request?
add set_group_slice operator and use it in delete/rename group operation
How is this patch tested? If it is not, please explain why.
Using delete/rename group operator
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Improvements