-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ENH]: Add install dependencies section in multimodal_retrieval.ipynb #4491
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This PR adds an 'Install Dependencies' section to the multimodal_retrieval.ipynb notebook, fixes typographical errors, and resolves a bug where the most relevant images weren't showing properly when querying with new URIs or images. This summary was automatically generated by @propel-code-bot |
@@ -414,7 +431,7 @@ | |||
"\n", | |||
"print(\"Results\")\n", | |||
"retrieved = collection.query(query_images=[query_image], include=['data'], n_results=5)\n", | |||
"for img in retrieved['data'][0][1:]:\n", | |||
"for img in retrieved['data'][0][:]:\n", |
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.
[BestPractice]
In both result loops, you're now including the query image itself in the display loop by using [:]
instead of [1:]
. This means the first displayed image will be the query image itself, which may be redundant and confusing to users.
Consider keeping the original slicing to exclude the query image from results:
"for img in retrieved['data'][0][:]:\n", | |
"for img in retrieved['data'][0][1:]:\n", |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
@@ -477,7 +494,7 @@ | |||
"query_uri = image_uris[1]\n", | |||
"\n", | |||
"query_result = collection.query(query_uris=query_uri, include=['data'], n_results=5)\n", | |||
"for img in query_result['data'][0][1:]:\n", | |||
"for img in query_result['data'][0][:]:\n", |
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.
[BestPractice]
Same issue here - this loop now includes the query image in the display results. Unless this is intentional, you should keep the original slicing to exclude the query image:
"for img in query_result['data'][0][:]:\n", | |
"for img in query_result['data'][0][1:]:\n", |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Description of changes
Improvements and Bug fixes:
multimodal_retrieval.ipynb
to ensure that the notebook works when "Run All" is used without throwing an error. This reduces the friction for beginners who want to try out the multimodal retrieval functionality of ChromaDB.Test plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?