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

C API : ClusteringIterationStats integration #1140

Merged

Conversation

glutamatt
Copy link
Contributor

C Api : update Clustering iterations stats

fix #1139

based on #1139 (comment)

Copy link
Contributor

@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.

Thank you for working on this! There is one major blocker and a minor concern that should be fixed before merging, please see the comments below.

  1. Missing definitions for the ClusteringIterationStats C getters
  2. rename obj to iteration_stats for consistency

@@ -75,14 +75,20 @@ FAISS_DECLARE_GETTER(Clustering, size_t, d)
/// getter for k
FAISS_DECLARE_GETTER(Clustering, size_t, k)

FAISS_DECLARE_CLASS(ClusteringIterationStats)
Copy link
Contributor

Choose a reason for hiding this comment

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

While these lines will correctly declare the class and its getters, they won't provide an implementation. Attempting to call any of these methods would fail at link time. Please head over to Clustering_c.cpp and add them through the DEFINE_GETTER macro (should be easy to follow by example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done thanks !!
3f9b817

c_api/Clustering_c.cpp Outdated Show resolved Hide resolved
c_api/Clustering_c.cpp Outdated Show resolved Hide resolved
c_api/Clustering_c.cpp Outdated Show resolved Hide resolved
c_api/Clustering_c.h Outdated Show resolved Hide resolved
glutamatt and others added 5 commits March 12, 2020 18:57
Co-Authored-By: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-Authored-By: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-Authored-By: Eduardo Pinho <enet4mikeenet@gmail.com>
Co-Authored-By: Eduardo Pinho <enet4mikeenet@gmail.com>
@mdouze mdouze merged commit ddd9c52 into facebookresearch:master Mar 12, 2020
@glutamatt glutamatt deleted the fix-c_api-clusteringiterationstats branch March 13, 2020 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v1.6.2 : Compilation of C API Failed
4 participants