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

Change points_in_polygon to return boolean array #238

Merged
merged 1 commit into from Aug 17, 2016

Conversation

corranwebster
Copy link
Contributor

Fixes #237.

Using unsigned char for boolean array rather than int.

@jwiggins
Copy link
Member

Why use unsigned char and not bool?

@corranwebster
Copy link
Contributor Author

Are we guaranteed bool* is an array of bytes? My knowledge of C++ isn't deep enough to know if that's certain.

@jvkersch
Copy link
Contributor

jvkersch commented Aug 17, 2016

Are we guaranteed bool* is an array of bytes?

I heard you guys talking about this at lunch and I was curious myself, so I looked it up in the C++ standard, and it's not guaranteed. Edit: cf. section 5.3.3 of this, in particular footnote 75.

@jwiggins
Copy link
Member

Right... I can't find a link to the relevant part of the C++ standard, but I did find this: http://en.cppreference.com/w/cpp/language/types

So no, it's not necessarily an array of bytes. Is this then dictated by what the Numpy C-API expects (even if obscured by SWIG)?

@jwiggins
Copy link
Member

I say use whatever Numpy expects. That's probably unsigned char. If so, LGTM.

@corranwebster
Copy link
Contributor Author

Yep - it's because numpy expects a bool array to be an array of bytes in memory with 0 and 1 values. So signed vs. unsigned shouldn't matter, I guess, but unsigned makes it clearer that we're just getting a chunk of memory.

@corranwebster corranwebster merged commit 168e222 into master Aug 17, 2016
@corranwebster corranwebster deleted the fix/points-in-polygon-dtype branch August 17, 2016 15:12
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.

None yet

3 participants