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

Fix build, tests, and clean up int types #62

Merged
merged 13 commits into from
Sep 9, 2023

Conversation

alecjacobson
Copy link
Contributor

@alecjacobson alecjacobson commented Sep 3, 2023

  • The build was failing. Appears to be fixed by using C++14.
  • A few tests were also failing. I've fixed most of them by using correct/modern numpy types.
  • Fixes Does dense_longlong mean the same thing on linux and windows? #59 by replacing non-standard int, long and long long in C++ code with int32 and int64 and (omitted) and on the python/codegen side connects intc/int32 and int/int64 appropriately
  • fixed broken github ci workflow
  • modified cmake build to work on local windows machine

Since 128 integers are not supported in a standard way in C++ or numpy, it doesn't make sense to me to try to support them. (I didn't purge the complex256 related code, but along these lines it should also go).

When writing bindings, code will need to change according to:

Before After
npe_arg(x, dense_int, dense_long, dense_longlong) npe_arg(X, dense_int32, dense_int64)

I don't understand the remaining failing test. I'm not sure what the correct behavior is supposed to be and I can't make sense of the macros defining the npe_default_arg

An upshot of this, is that a bunch of the #ifdef WIN stuff can be dropped. By insisting on explicitly declaring the int sizes, we get the benefits of using std::.

While it would have been easier to just keep a transform_type_char for windows to filter 'Q','q' into 'L','l', the change I made uses isinstance checks to truly verify that the type is the correct it. Ideally the chars could be eliminated entirely for compile-time type checks but I leave that for another day.

I tried to fix the failing test (already failing before this PR) npe_default_arg for sparse types. It seems npe_default_arg is not working at all for sparse types (also not working with npe_matches / not clear how to pass a default arg [as Eigen::SparseMatrix?] ). Ideally, npe_default_arg with sparse types should just not compile rather than silently building and getting a suprise at runtime.

@alecjacobson
Copy link
Contributor Author

(Sorry this snowballed into a larger change and I didn't break it up. If this has any chance of getting merged and you'd prefer it in smaller PRs just let me know.)

@fwilliams
Copy link
Owner

Hey Alec, thanks a lot for this change! I agree with your points about types and this is a much cleaner API. I'm a bit busy this week but I'll review this and try to get it merged asap.

I need to update point-cloud-utiles and an internal library at NVIDIA to match your changes. This should give some extra test coverage.

Also, I have an open PR to integrate the very small change (added a public method to a bumpy class) from my fork, so we should soon be able to depend on mainline pybind.

@alecjacobson
Copy link
Contributor Author

Sounds good. FWIW libigl-python-bindings main is currently ✅ building and ✅ passing tests with this branch.

Copy link
Owner

@fwilliams fwilliams left a comment

Choose a reason for hiding this comment

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

There's one broken test that checks if you can pass in a None. This actually isn't supported behavior because it turns out to be quite hard to handle this case and there's an easier way to do it.

If you could comment out that line and make sure the tests pass, I'm ready to merge.

@@ -104,20 +104,26 @@ def test_passing_0d_arrays(self):
def test_passing_0d_arrays_1(self):
dim = np.random.randint(5)
# (np.zeros([0, dim]), np.zeros([dim, 0]), np.zeros([0]), np.zeros([0, 0])):
dim = 2
Copy link
Owner

Choose a reason for hiding this comment

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

maybe separate out a dim=1 case

self.assertEqual(len(a), 0)
# Alec: I'm not sure if it's expected behavior in numpy or a bug in
# numpyegien, but if dim == 1 then retp.shape = (0,) rather than (0,1)
if dim == 1:
Copy link
Owner

Choose a reason for hiding this comment

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

NumpyEigen squeezes 1 dimensions by default when returning values, but there's an argument to npe::move (maybe it's hidden -- I don't recall atm) that changes this behavior.

//
// But in the end is this an elaborate way to change dtype().type() 'Q','q' -> 'L','l' ?
//
// Yes, but I guess the point is that these checks are what the code should be doing instead of using the chars to begin with.
Copy link
Owner

Choose a reason for hiding this comment

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

These characters are unique identifiers for the internal numpy types (https://numpy.org/doc/stable/reference/generated/numpy.dtype.char.html#numpy.dtype.char).

If I remember correctly, we need a way of translating from the pybind array type into an identifier, and also being able to return something in the case where the array is something weird (like Object) which will throw an error.

I suppose you're right though, this could all be inlined into the generated code.

@fwilliams fwilliams merged commit e9e01eb into fwilliams:master Sep 9, 2023
0 of 3 checks passed
@fwilliams
Copy link
Owner

Found a small bug that fixes the failing test. Merging your PR and will send a fix.

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 this pull request may close these issues.

Does dense_longlong mean the same thing on linux and windows?
2 participants