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

Support reset & dispose #42

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

Conversation

asilvas
Copy link
Contributor

@asilvas asilvas commented Sep 21, 2023

No description provided.

METRIC_L1, ///< L1 (aka cityblock)
METRIC_Linf, ///< infinity distance
METRIC_Lp, ///< L_p distance, p is given by a faiss::Index
METRIC_L1 = 2, ///< L1 (aka cityblock)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the docs weren't smart enough to get the incremental values.

@ewfian
Copy link
Owner

ewfian commented Sep 21, 2023

Thanks for your new PR, Noticed that dispose method was added. Could you please clarify the indentation? I see that it's being checked in every method, and I'm trying to understand its necessity.

Also, I noticed that the underlying implementation of the dispose method is actually calling index_.release();. If we keep this method, would it be more appropriate to rename it to release to reflect its actual functionality?

@asilvas
Copy link
Contributor Author

asilvas commented Sep 21, 2023

Thanks for your new PR, Noticed that dispose method was added. Could you please clarify the indentation? I see that it's being checked in every method, and I'm trying to understand its necessity.

Also, I noticed that the underlying implementation of the dispose method is actually calling index_.release();. If we keep this method, would it be more appropriate to rename it to release to reflect its actual functionality?

Dispose is used to free all memory associated with an index. If you're OK with a segfault if calling a method on a released index, then I'm fine reverting the checks from every method.

dispose is consistent naming across ML frameworks for freeing memory. But up to you if you want to change it.

@ewfian
Copy link
Owner

ewfian commented Oct 2, 2023

@asilvas In the world of javascript, memory is rarely dealt with, the reset and dispose have similar uses. I want to remove the dispose method first, and merge this pr. Is this okay?

@asilvas
Copy link
Contributor Author

asilvas commented Oct 2, 2023

@asilvas In the world of javascript, memory is rarely dealt with, the reset and dispose have similar uses. I want to remove the dispose method first, and merge this pr. Is this okay?

I'd prefer to keep both as there is no other way to free all memory, and have need for recreating a lot of indexes.

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