-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support save contexts for generated views #17
base: cloned_release_v0.25.0_11757
Are you sure you want to change the base?
Support save contexts for generated views #17
Conversation
Clone of the PR voxel51/fiftyone#4636 |
👋 I'm here to help you review your pull request. When you're ready for me to perform a review, you can comment anywhere on this pull request with this command: As a reminder, here are some helpful tips on how we can collaborate together:
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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.
Review Summary by Korbit AI
Code Execution Comments
- Implement error handling to check for
ground_truth
presence before setting attributes to avoid potentialAttributeError
.
Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-review
command in a comment at the root of your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ✅ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Tests ❌ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
for sample in view.iter_samples(autosave=True): | ||
sample.ground_truth.foo = "bar" |
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.
The code iterates through the samples in the view
and sets sample.ground_truth.foo = "bar"
, but it does not handle the case when a sample may not have a ground_truth
field, such as sample2
in this test. This can lead to an AttributeError
if ground_truth
does not exist. Consider adding error handling, such as a try/except block or an if statement, to check for the presence of ground_truth
before accessing and setting the foo
attribute.
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
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.
@korbit-ai How would I fix this issue?
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.
Hi @julianser, it's great that you're working on making the save contexts available for more interfaces. One suggestion to prevent the possible AttributeError
when the ground_truth
field may not exist could be to add an if statement in this manner: if 'ground_truth' in sample:
before accessing sample.ground_truth
and setting the foo
attribute. This would check for the presence of ground_truth
before attempting to access and set the foo
attribute, thereby preventing any AttributeError
from occurring. Keep up the great work!
Resolves #4619
Prior to
fiftyone==0.23.8
, methods likecompute_embeddings()
andapply_model()
were callingsample.save()
in a loop, which is inefficient, so we updated these methods to use save contexts in voxel51/fiftyone#4244.However, save contexts were not yet implemented for generated views, so the above methods stopped working when invoked on generated views such as
to_patches()
😭This PR resolves this by fully implementing save contexts for generated views 🥂
Tested by
Added unit tests 🪄
Example usage
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Description by Korbit AI
What change is being made?
Enable save contexts for generated views in the FiftyOne library, including support for Clips, Patches, and Frames views, and add corresponding unit tests.
Why are these changes being made?
Previously, generated views such as Clips, Patches, and Frames did not support save contexts, which limited their functionality. This change allows these views to save changes back to their source datasets, enhancing the usability and flexibility of the library. The added unit tests ensure that the new functionality works as expected and maintains data integrity.