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

RFC: replace {Long,LongLong,...}Vector with {Int32,Int64,...}Vector (with deprecation) #1741

Closed
h-vetinari opened this issue Mar 5, 2021 · 2 comments

Comments

@h-vetinari
Copy link
Contributor

In trying to get faiss-gpu to work with windows (cf. #1586), I'm hitting a couple of points where the different architectures collide. One of the main points about this revolves about what types int, long and long long actually correspond to on the respective platforms.

Since these types are - sadly - fundamentally not cross-platform compatible, it is a bit unfortunate that the SWIG interface currently defines IntVector, LongVector, LongLongVector.

For reasons of portability, I therefore wanted to suggest to deprecate these names and replace them with something unambiguous, e.g:

  • CharVector -> Int8Vector
  • IntVector -> Int32Vector
  • LongVector -> Int32Vector if sizeof_long == 4 else Int64Vector
  • LongLongVector -> Int64Vector
  • ByteVector -> UInt8Vector
  • FloatVector -> Float32Vector
  • DoubleVector -> Float64Vector

This is the maximal reasonable set of renames/deprecations (Int16Vector, UInt16Vector, UInt32Vector, UInt64Vector exist already), and arguably not all of the names on the left need to be deprecated. However, the uniform naming structure and intrinsic match with the underlying bit-width (as well as the numpy-types) is something that would IMO also substantially reduce the cognitive burden of figuring out what object one is working with ("what was long again on this system?"; or not knowing that char or byte correspond to integer types).

I have a solution for doing exactly that which I believe is fairly elegant and unobtrusive - essentially generating wrapper classes around the equivalent non-deprecated versions in __init__.py, with the only difference that they raise a DeprecationWarning once per session (if called). The advantage of this is that this can be done in pure python, and doesn't need any surgery on the SWIG interface.

If desired, I can post the patches directly, but wanted to first get a feel if this would be welcome.

WDYT @wickedfoo @beauby @mdouze?

@mdouze
Copy link
Contributor

mdouze commented Mar 5, 2021

I agree about the mess with XXXVectors, let's change those. They are an inheritance from Lua...

Personally, I hate deprecation warnings. Let's just rename the vectors to make sure that code that uses the old names crashes. I don't think there is a lot of external code that uses these names anyways, because the array_to_vector interface is more convenient.

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Mar 5, 2021

Cool, good to hear!

The final decision is ultimately yours, but I would still give users at least one release advance notice before removing them. I'll post the patches I currently have, but of course, we can still adapt to your proposal - changing names is of course a bit easier than providing a deprecation period (but I still think that this would be kinder for the users).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants