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

Make normalizeVector(v) in place #81

Conversation

erikdubbelboer
Copy link
Contributor

All usage of normalizeVector was in the form of

    v = normalizeVector(v)

In that case it's better to just do the replacement in place and not having to allocate a new slice.

@erikdubbelboer erikdubbelboer force-pushed the normalizeVector-in-place branch from 095f876 to fb1beb6 Compare May 19, 2024 13:09
Copy link
Owner

@philippgille philippgille left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestion!

I'm worried that for the places where the user passes an existing embedding (e.g. as Add() parameter, Document.Embedding field in AddDocument() or QueryEmbedding() parameter), they could be surprised to have it changed and they might want to continue using their slice in their code, where it might need to be the unnormalized form.

Do you have a use case where the allocation is an issue (noticeable/measurable memory usage performance difference)?

Could it be an option to have both normalize functions, and call the in-place one only when the embedding is not user-supplied?

All usage of normalizeVector was in the form of

    v = normalizeVector(v)

In that case it's better to just do the replacement in place and not
having to allocate a new slice.
Except for in AddDocument as the caller hands of control of the Document
to us there.
@erikdubbelboer erikdubbelboer force-pushed the normalizeVector-in-place branch from fb1beb6 to 43e1a07 Compare May 26, 2024 06:31
@erikdubbelboer
Copy link
Contributor Author

I agree that with a library like this the performance overhead of some extra garbage collection probably isn't going to be measurable.

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