ENH: __contains__ can check multiple points

[carterbox]

If you want to check whether many points are inside a polytope, looping over each point and using in is very slow compared to testing all the points simultaneously. This commit allows the user to specify an np.ndarray of column vectors to check simultaneously. The return value is then an array of bools. In order to use this feature, you have to call __contains__ directly instead of using in.

[johnyf]

Thanks, this addition addresses a canonical task. Comments:

1. This implementation of __contains__ does not satisfy the specification of __contains__, which requires that it always return a Boolean value (my understanding is that this is also why one needs to call __contains__ directly). So this implementation does not comply to Python's data model.

2. Following the previous observation, (and also previous experience with overloading __str__ in an unanticipated way and later trying to figure out why certain behavior was observed--after having forgotten the details), and since the user will anyway have to call some method directly, I recommend implementing this option as a method named contains. This will also make the method visible, thus making users aware of its availability, via introspection. Regarding usage, writing contains instead of __contains__ is shorter and doesn't need pressing any modifier keys, and also more readable.

3. PEP8 recommends having few to no empty lines. I agree with that recommendation, even though I once used to leave blank spaces. Thanks to Python's meaningful indentation, removing all blank spaces emphasizes the control flow, and so aids in reading code. At places where a blank line feels mandatory, a suitable brief comment usually exists that would fill that empty line, thus both not interrupting the vertical indentation line, and utilizing the otherwise empty space.

The tests fail for another reason, see #46.

[carterbox]

1,2. OK. That seems reasonable. I will split multiple points out into a separate contains function.

3. Are you referring to blank lines between successive if control statements? I will also remove those.

[johnyf]

Yes, I mean the (few) blank lines between if, for, return, and assert statements.

[carterbox]

I just realized that there is a method polytope.are_inside() which already does the contains operation for polytope.polytope. However, this method does not exist for polytope.region.

I added a deprecation warning for are_inside because contains can be used for all polyregs.

[johnyf]

@carterbox should I proceed with merging?

[carterbox]

@johnyf I'm not sure. I didn't make any changes to the documentation for contains because I was confused by whether you wanted me to use "strictly contains" instead of just "contains" to express <.

[johnyf]

We could use "contains", and maybe add "(<)" somewhere in the docstring, which is unambiguous.

[johnyf]

Also, please add commits to the pull request without rebasing, as recommended for CPython, to keep the reviewing linear, letting the rebasing happen once upon merging. Quoting:

"Your pull request may involve several commits as a result of addressing code review comments. Please keep the commit history in the pull request intact by not squashing, amending, or anything that would require a force push to GitHub. A detailed commit history allows reviewers to view the diff of one commit to another so they can easily verify whether their comments have been addressed. The commits will be squashed when the pull request is merged."

[carterbox]

I've realized that with this pull, I have complicated the API by adding contains. There are now 4 methods which do similar things: in (__contains__), is_inside, contains, and are_inside.

Instead of making it more complicated or breaking the API by replacing is_inside and are_inside with contains, my goals are now the following:

1. Extend are_inside to work with both regions and polytopes.
2. Add doc strings all of these functions, so they are easier to find.

[carterbox]

@johnyf OK. I think it's ready for a final review and merge.

[johnyf]

Merged as of 0e6120aa73bf364568be4c3214b5f6e4acea4bb7, but with changes that prefer contains and __contains__. Indeed, simple is better than complex. I think that the method contains is a good addition, so I kept contains, __contains__, and deprecated the rest. There should be preferably one obvious way to do things [PEP 20]. Writing poly.contains(points) is much more readable than pc.are_inside(points, poly).

Correction to some previous remarks: the strict inequality test is with respect to the tolerance bound (self.A.dot(point.flatten()) - self.b < ABS_TOL), not with respect to the boundary (so the boundary is included). Apologies for the confusion. Of course, passing abs_tol=0 results to a strict containment test, excluding the boundary, so in that sense the discussion was relevant. I noted this option in the docstring of the method Polytope.contains.

The method Region.__contains__ was using a loop before this PR. Using any over a generator is less code and more Pythonic.

Regarding absolute tolerance, it was impractical that I had added this argument to __contains__, no one will pass it there. Instead, it suffices to have it as an argument of the method contains. If one really wants to pass a tolerance, they can call poly.contains([point], abs_tol=smaller).

About whether point can be flattened, I just mentioned it has to be a vector, leaving it to the user to explore corner cases that may want to provide as input.

I removed the reshaping, because it is the user's responsibility to make sure they pass reasonable input. Also, it modified the argument, whenever the argument was an instance of numpy.ndarray (so there were side effects).

I expanded the tests to exercise regions too.