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

Python IndexBinaryFlat.reconstruct_n() broken #2751

Closed
2 of 4 tasks
Dobatymo opened this issue Mar 9, 2023 · 2 comments
Closed
2 of 4 tasks

Python IndexBinaryFlat.reconstruct_n() broken #2751

Dobatymo opened this issue Mar 9, 2023 · 2 comments

Comments

@Dobatymo
Copy link

Dobatymo commented Mar 9, 2023

Summary

reconstruct_n on a IndexBinaryFlat throws a TypeError about a additional argument for the reconstructed vectors. Other Indexes simply return them. Also providing a None value simply crashes Python.

>>> import faiss
>>> index = faiss.IndexBinaryFlat(64)
>>> index.reconstruct_n(0, 4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: reconstruct_n() missing 1 required positional argument: 'recons'
>>> index.reconstruct_n(0, 4, None)  # crash!

EDIT: It looks like the reconstruct_n definition is missing for handle_IndexBinary (?)

def handle_IndexBinary(the_class):

Platform

OS: Windows 10
Python version: 3.8.10
Faiss version: faiss-cpu==1.7.3
Installed from: pip

Running on:

  • CPU
  • GPU

Interface:

  • C++
  • Python

Reproduction instructions

See summary.

@mdouze
Copy link
Contributor

mdouze commented Mar 10, 2023

This is because the reconstruct_n function is not wrapped in python. The proper way of calling it is with:

x  = np.empty((4, index.d // 8), dtype='uint8')
index.reconstruct_n(0, 4, faiss.swig_ptr(x)) 

But it would be better if the function was wrapped properly, hence the enhancement.

@Dobatymo
Copy link
Author

Dobatymo commented Mar 10, 2023

@mdouze Thank you! But there is still something weird going on. Running this with index.d // 8 crashes Python also. Using simply index.d works, but then of course the array is to large for searches.

import numpy as np
import faiss
arr = np.random.randint(0, 256, size=(100, 10), dtype=np.uint8)
index = faiss.IndexBinaryFlat(arr.shape[1] * 8)
index.add(arr)
verify = np.empty((index.ntotal, index.d), dtype=np.uint8)
index.reconstruct_n(0, index.ntotal, faiss.swig_ptr(verify))
verify = verify[:,:index.d // 8]
assert np.array_equal(arr, verify)

verify[:,:index.d // 8] seems to do the trick...

kuarora added a commit to kuarora/faiss that referenced this issue Mar 22, 2024
Summary:
**Context**
[Issue 2751](facebookresearch#2751) is already fixed as class wrappers has replacement definition of reconstruct_n in handle_IndexBinary.

**In this diff**,
Writing test test_reconstruct for binary index to validate fix for above issue.

Differential Revision: D55168600
facebook-github-bot pushed a commit that referenced this issue Mar 22, 2024
Summary:
Pull Request resolved: #3310

**Context**
[Issue 2751](#2751) is already fixed as class wrappers has replacement definition of reconstruct_n in handle_IndexBinary.

**In this diff**,
Writing test test_reconstruct for binary index to validate fix for above issue.

Reviewed By: junjieqi

Differential Revision: D55168600

fbshipit-source-id: b62dc5fa89d65b843c52faa7456f046142e34421
@kuarora kuarora closed this as completed Mar 25, 2024
abhinavdangeti pushed a commit to blevesearch/faiss that referenced this issue Jul 12, 2024
Summary:
Pull Request resolved: facebookresearch#3310

**Context**
[Issue 2751](facebookresearch#2751) is already fixed as class wrappers has replacement definition of reconstruct_n in handle_IndexBinary.

**In this diff**,
Writing test test_reconstruct for binary index to validate fix for above issue.

Reviewed By: junjieqi

Differential Revision: D55168600

fbshipit-source-id: b62dc5fa89d65b843c52faa7456f046142e34421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants