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

Allow empty searches #47

Merged
merged 2 commits into from
May 20, 2024
Merged

Allow empty searches #47

merged 2 commits into from
May 20, 2024

Conversation

granawkins
Copy link
Member

No description provided.

Copy link

@mentatai mentatai bot left a comment

Choose a reason for hiding this comment

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

The PR includes changes necessary for handling empty searches, and the code works as intended. However, the hardcoded distance value and a spelling mistake in the comments need attention to improve readability and potential functionality adjustments.

Butler is in closed beta. Reply with feedback or to ask Butler to review other parts of the PR. Please give feedback with emoji reacts.

Comment on lines +29 to +31
# If query is empty, searching DB will raise "RuntimeError('Cannot return the
# results in a contigious 2D array. Probably ef or M is too small')"
if not query:
Copy link

Choose a reason for hiding this comment

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

This comment in the code gives an insight into a potential runtime error when the query is empty. However, the correct word here should be 'contiguous' instead of 'contigious.'

Suggested change
# If query is empty, searching DB will raise "RuntimeError('Cannot return the
# results in a contigious 2D array. Probably ef or M is too small')"
if not query:
# results in a contiguous 2D array. Probably ef or M is too small')

Correcting the typo enhances code readability and professionalism.

# results in a contigious 2D array. Probably ef or M is too small')"
if not query:
results = [
{**data, "distance": 1}
Copy link

Choose a reason for hiding this comment

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

Defining a default distance of 1 for all results may not be intuitive for users, especially if they expect a more dynamically calculated distance based on some metric. It might be beneficial to document this choice or revise the logic.

Consider providing a more context-aware calculation for distance when the query is empty, or at least clarify why a distance of 1 is appropriate in this context.

@granawkins granawkins merged commit 524dcca into main May 20, 2024
3 checks passed
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.

1 participant