Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

upgrade faiss #7 #8

Merged
merged 2 commits into from
Sep 13, 2020
Merged

upgrade faiss #7 #8

merged 2 commits into from
Sep 13, 2020

Conversation

gensmusic
Copy link
Contributor

@gensmusic gensmusic commented Sep 2, 2020

@Enet4
This is the first basic patch.
some changes

  1. upgrade to faiss: 2ac91ad79d9b82800804e073b13a64223cdd6727
  2. bindgen add --size_t-is-usize, since bindgen change the size_t convertion. This leads a minor change of Index trait. see remove_ids()
  3. adapt to the Clustering class changes.

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, but so far I haven't managed to build the respective Faiss version on my machine (probably a CUDA or GCC mismatch, will continue trying). In the meantime, I took the liberty of reviewing this code in terms of API design.

src/cluster.rs Outdated Show resolved Hide resolved
src/cluster.rs Outdated Show resolved Hide resolved
@gensmusic
Copy link
Contributor Author

I prefer the repr(transparent) way, but FaissClusteringIterationStats(_H) is an opaque type which size is 0.

#[repr(C)]
pub struct FaissClusteringIterationStats_H {
    _unused: [u8; 0],
}

repr(transparent) require non-zero-size T,therefore I cannot use a tuple struct wrapper.

#[repr(transprent)]
pub struct ClusteringIterStats(FaissClusteringIterationStats)
// got error[E0690]: transparent struct needs exactly one non-zero-sized field, but has 0

Since FaissClusteringIterationStats(_H) is an opaque type, I mannually create a ClusteringIterationStats same as FaissClusteringIterationStats(_H).

@gensmusic gensmusic requested a review from Enet4 September 13, 2020 03:37
@gensmusic
Copy link
Contributor Author

you can review the changes in 843c1a4

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Right, those changes are OK! I somehow assumed that FaissClusteringIterationStats was a record type rather than an opaque type.

This is good to merge. I'll try to throw in a new crate release (0.9.0) within the next few days. Thank you very much!

@Enet4 Enet4 merged commit 30da2c4 into Enet4:master Sep 13, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants