Behavior of is_collinear() in geometry/point.py

[Amit Saha]

Hello,

Consider the following:

Python 3:

>>> from sympy import Line, Point
>>> line = Line(Point(1,0), slope = 1)
>>> Point.is_collinear(line)
True

Python 2:

>>> from sympy import Line
>>> line = Line(Point(1,0), slope = 1)
>>> Point.is_collinear(line)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unbound method is_collinear() must be called with Point
instance as first argument (got Line instance instead)

In the second case (because of Python 2's concept of unbound methods),
the user at least knows that something wrong has happened. In the
first case, however, since Python 3 allows class methods to be called
with any object instance, it is simply not a problem.

I propose the following fix:

diff --git a/sympy/geometry/point.py b/sympy/geometry/point.py
index 4e941a1..7f359f5 100644
--- a/sympy/geometry/point.py
+++ b/sympy/geometry/point.py
@@ -151,6 +151,7 @@ def length(self):
"""
return S.Zero

+ @classmethod
def is_collinear(*points):
"""Is a sequence of points collinear?

@@ -209,6 +210,10 @@ def is_collinear(*points):
# Coincident points are irrelevant and can confuse this algorithm.
# Use only unique points.
points = list(set(points))
+
+ if not any([isinstance(p, Point) for p in points]):
+ raise GeometryError('Must pass only point objects')
+
if len(points) == 0:
return False
if len(points) <= 2:


The conversion to @classmethod was only done so that we are able to
raise the exception in case of Python 2 as well.

With this fix in place, for both Python 2 and Python 3, the user will
get a GeometryError exception raised:

sympy.geometry.exceptions.GeometryError: Must pass only point objects


Here is the test script I used:

from sympy import Point, Line

line = Line(Point(1,0), slope = 1)
Point.is_collinear(line)


If this looks good, I will submit a pull request with updated tests.


Thanks,
Amit.

[Amit Saha]

That should be all() instead.

-Amit.

--
http://echorand.me

[Amit Saha]

Realized that I had missed something else as well. Anyway, here is the
proposed fix:

diff --git a/sympy/geometry/point.py b/sympy/geometry/point.py
index 4e941a1..c524c61 100644

--- a/sympy/geometry/point.py
+++ b/sympy/geometry/point.py
@@ -151,6 +151,7 @@ def length(self):
"""
return S.Zero

+ @classmethod
def is_collinear(*points):
"""Is a sequence of points collinear?

@@ -208,7 +209,11 @@ def is_collinear(*points):

"""
# Coincident points are irrelevant and can confuse this algorithm.
# Use only unique points.

- points = list(set(points))
+ points = list(set(points[1:]))
+
+ if not all([isinstance(p, Point) for p in points]):

+ raise GeometryError('Must pass only point objects')
+
if len(points) == 0:
return False
if len(points) <= 2:

Best,
Amit.


--
http://echorand.me

[Aaron Meurer]

Sounds good to me. Can you submit a pull request?

Aaron Meurer

> --
> You received this message because you are subscribed to the Google Groups "sympy" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to sympy+un...@googlegroups.com.
> To post to this group, send email to sy...@googlegroups.com.
> Visit this group at http://groups.google.com/group/sympy.
> For more options, visit https://groups.google.com/groups/opt_out.

[Amit Saha]

On Sat, Dec 14, 2013 at 6:53 AM, Aaron Meurer <asme...@gmail.com> wrote:
> Sounds good to me. Can you submit a pull request?

Thanks, Aaron. Done finally: https://github.com/sympy/sympy/pull/7327

Best,
Amit.

--
http://echorand.me