Skip to content
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

Fixed Typos and Clarifying Concepts on Semantic Cache Notebook #75

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Tuvshno
Copy link

@Tuvshno Tuvshno commented Apr 5, 2024

Hello, first time contributor here!

What does this PR do?

I was working through this great notebook and noticed a typo along with some areas that I felt needed clarifying in the code. The changes were mostly comments and a typo.

I fixed the "thresold" to "threshold" typo inside the semantic_cache class.

In the semantic_cache class, I tried to reduce the ambiguity of the code by adding a few clarifying comments since the class was the central point of the notebook. These additions aim to assist first-time readers in understanding the FAISS implementation.

These changes include adding clarification on:

  • The definitions of the D and I arrays that were returned from index.search()
  • Meaning of nprobe
  • Adding comments on the meaning of the If conditions

I didn't want to overcomplicate the explanations as the contribution guide says, but I did want to give context that I thought could help a reader out.

Who can review?

@MKhalusova I would appreciate the review!

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@merveenoyan merveenoyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very general comment, thanks a lot

" self.index.nprobe = 8\n",
" self.index.nprobe = 8 # Number of nearby cells to search\n",
"\n",
" # D - Array of Distances between the query vector and nearest neighbors\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repetitive comments here breaks readability of the code sometimes, would be nice to instead have a docstring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants