Skip to content

Fix cosine similarity when missing simd alignment#808

Merged
changhiskhan merged 3 commits intomainfrom
changhiskhan/cosine-bug
Apr 25, 2023
Merged

Fix cosine similarity when missing simd alignment#808
changhiskhan merged 3 commits intomainfrom
changhiskhan/cosine-bug

Conversation

@changhiskhan
Copy link
Copy Markdown
Contributor

also a bunch of formatting fixes

@changhiskhan changhiskhan requested a review from eddyxu April 25, 2023 08:02
@changhiskhan changhiskhan force-pushed the changhiskhan/cosine-bug branch 3 times, most recently from 4a3b347 to 95ea462 Compare April 25, 2023 08:28
Copy link
Copy Markdown
Contributor

@gsilvestrin gsilvestrin left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread rust/src/linalg/cosine.rs Outdated
}

#[inline]
fn dotprod_scalar<T: Real + Sum>(x: &[T], y: &[T]) -> T {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lets call https://github.com/eto-ai/lance/blob/main/rust/src/linalg/dot.rs ?

Will add SIMD for dot product later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment thread rust/src/linalg/cosine.rs
mod x86_64 {
use super::dotprod_scalar;
use super::sq_scalar;
use std::arch::x86_64::*;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sort import by sections? std / third party / lance with a blank line in between.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment thread rust/src/linalg/cosine.rs Outdated
}

#[inline]
fn sq_scalar<T: Real + Sum>(y: &[T]) -> T {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Extract this to a new routine? or just use normalize().pow(2)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@changhiskhan changhiskhan force-pushed the changhiskhan/cosine-bug branch from 95ea462 to f107814 Compare April 25, 2023 19:15
Copy link
Copy Markdown
Member

@eddyxu eddyxu left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@changhiskhan changhiskhan merged commit 173c805 into main Apr 25, 2023
@changhiskhan changhiskhan deleted the changhiskhan/cosine-bug branch April 25, 2023 20:39
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