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

Does dense_longlong mean the same thing on linux and windows? #59

Closed
alecjacobson opened this issue Feb 11, 2023 · 1 comment · Fixed by #62
Closed

Does dense_longlong mean the same thing on linux and windows? #59

alecjacobson opened this issue Feb 11, 2023 · 1 comment · Fixed by #62

Comments

@alecjacobson
Copy link
Contributor

alecjacobson commented Feb 11, 2023

I'm sorry I don't have a minimal example. I'm trying to determine whether dense_longlong is ever needed in my use case (libigl python bindings). After looking at

'dense_int': ('npy_int', 'int', 'int32'),
'dense_long': ('npy_long', 'long', 'int64'),
'dense_longlong': ('npy_longlong', 'longlong', 'int128'),
my understanding is that dense_longlong would correspond to int128 which for my case is way too big.

However, when I left just dense_int and dense_long (which I thought corresponded to int32 and int64), my windows tests are failing

    b = igl.boundary_loop(f)
  ValueError: Invalid scalar type (longlong, Row Major) for argument 'f'. Expected one of ['int32', 'int64'].

Here my understanding is that f is read from a .npy file into python as a int64 numpy array. So, I'm confused by this ValueError: message (which I believe is coming from numpyeigen). Is this error message accurate?

I think maybe there's a confusion happening here due to how ambiguous C++ is for the sizes of int, long, longlong etc. But I wonder if there's a numpy_eigen way to avoid this:

For example, in that boundary_loop python binding I have code that currently looks like:

npe_arg(f, dense_int, dense_long ) 

but what I'd much rather write is:

npe_arg(f, dense_int32, dense_int64)

since I'm defining an input argument, it's convenient if these type names match the python types which are thankfully non-ambiguous.

Adding dense_longlong to the current code seems to get around this issue on windows but inadvertently build int128 bindings on linux/mac?

Is there a current way to guarantee that I build exactly int32 and int64 bindings in numpyeigen?

If not, how hard would it be to support the proposed dense_int32 and dense_int64 type names above?

@fwilliams
Copy link
Owner

You are correct that dense_longlong does not mean the same thing on MSVC and most non-MSVC compilers.

NumpyEigen follows the naming convention here: https://numpy.org/doc/stable/user/basics.types.html

You can see that longlong is platform specific. On MSVC, long and int are the same types (32 bit signed integer) and long long is a 64 bit signed integer. On other platforms long is a 64 bit signed integer and long long is either 64 or 128 bits.

In general, I agree that int128 is way too big for most applications. One possible solution is to add types in NumpyEigen dense_int32 dense_int64, dense_int128 that are properly ifdef'd and result in the right thing on the right platform. The reason I didn't do this is that it's quite tedious to test (need to try 32 and 64 bit platforms and MSVC/Clang/GCC accross multiple platforms).

If you want to give it a try,you have to add types in this file: https://github.com/fwilliams/numpyeigen/blob/master/src/npe_typedefs.cpp (note the idfdefs for WIN64)

and add new tokens to the compiler here:

'dense_longlong': ('npy_longlong', 'longlong', 'int128'),

I would actually really appreciate a PR to fix this since I think think your use case is pretty common.

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 a pull request may close this issue.

2 participants