Skip to content

remove cosine_distance/euclidean_distance impl #752

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

Closed
wants to merge 1 commit into from

Conversation

skshetry
Copy link
Member

in favor of usearch's.

Copy link

codecov bot commented Dec 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.37%. Comparing base (195199e) to head (5209fb9).
Report is 88 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #752      +/-   ##
==========================================
- Coverage   87.51%   87.37%   -0.15%     
==========================================
  Files         114      113       -1     
  Lines       10945    10920      -25     
  Branches     1501     1501              
==========================================
- Hits         9579     9541      -38     
- Misses        991     1000       +9     
- Partials      375      379       +4     
Flag Coverage Δ
datachain 87.35% <ø> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -217,23 +217,6 @@ def register_user_defined_sql_functions() -> None:
# Register optional functions if we have the necessary dependencies
# and otherwise register functions that will raise an exception with
# installation instructions
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

from memory: we need this as a fallback because usearch is not available on Windows - unum-cloud/usearch#427

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

description on that PR should shed some light on the approach

@shcheklein
Copy link
Member

@skshetry what is the status of this?

1 similar comment
@shcheklein
Copy link
Member

@skshetry what is the status of this?

@shcheklein
Copy link
Member

@skshetry is it still needed?

@skshetry
Copy link
Member Author

@skshetry is it still needed?

Well, there are no extensions for macOS available atm (see unum-cloud/usearch#557). Linux is not working either for some reason, and usearch does not support Windows. So, CI is not running usearch, and as a result, tests are stable.

@shcheklein
Copy link
Member

Well, there are no extensions for macOS available atm (see unum-cloud/usearch#557). Linux is not working either for some reason, and usearch does not support Windows. So, CI is not running usearch, and as a result, tests are stable.

so, do we need it or not?

@skshetry
Copy link
Member Author

Well, there are no extensions for macOS available atm (see unum-cloud/usearch#557). Linux is not working either for some reason, and usearch does not support Windows. So, CI is not running usearch, and as a result, tests are stable.

so, do we need it or not?

I created this to fix flaky tests. There are no flaky tests atm, we'll need this when unum-cloud/usearch#557 gets fixed.

@shcheklein
Copy link
Member

we'll need this when unum-cloud/usearch#557 gets fixed.

to do what?

@skshetry skshetry closed this Jan 31, 2025
@skshetry skshetry deleted the cleanup-cosine-distance branch January 31, 2025 03:25
@skshetry
Copy link
Member Author

skshetry commented Jan 31, 2025

to do what?

The cosine_distance in usearch is not stable and has floating point errors, especially for smaller numbers. See unum-cloud/usearch#320.
This makes test flaky, which this fixes by adding a tolerance. (This PR also tried to do other things, but that's not neccessary).

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.

3 participants