-
Notifications
You must be signed in to change notification settings - Fork 563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix target view for clear sample/frame field operators #5122
Conversation
WalkthroughThe changes in this pull request modify the Changes
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
fiftyone/operators/builtin.py (2)
625-629
: Consider refactoring duplicate code inexecute
methodsBoth
ClearSampleField.execute
andClearFrameField.execute
contain similar code for handling thetarget
parameter and obtainingtarget_view
. To adhere to the DRY principle, consider extracting this common logic into a shared method or utility function to improve maintainability.Also applies to: 719-723
Line range hint
635-686
: Refactor duplicated input resolution logic in_clear_sample_field_inputs
and_clear_frame_field_inputs
The functions
_clear_sample_field_inputs
and_clear_frame_field_inputs
share similar logic for constructingtarget_choices
and handling thetarget
parameter. Refactoring this shared code into a common utility function would reduce duplication and enhance readability.Also applies to: 729-780
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
fiftyone/operators/builtin.py
(2 hunks)
🔇 Additional comments (2)
fiftyone/operators/builtin.py (2)
625-629
: Implemented target
parameter handling in ClearSampleField.execute
The execute
method now retrieves the target
parameter and obtains the corresponding target_view
using _get_target_view
. This change allows for clearing sample fields on specific views, enhancing the operator's flexibility.
719-723
: Added target
parameter support in ClearFrameField.execute
The execute
method in ClearFrameField
now correctly handles the target
parameter and obtains the appropriate target_view
, enabling the clearing of frame fields on specified views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary by CodeRabbit
New Features
ClearSampleField
andClearFrameField
operators to allow clearing fields in specific views or the entire dataset.Bug Fixes
target
parameter.