ezag / pyeuclid

2D and 3D maths module for Python
97 stars 41 forks source link

Minor fix to improve compatibility when using numarray value types #10

Closed robclewley closed 7 years ago

robclewley commented 9 years ago

I'm not an expert with this, but I don't think it's numpy's problem. Here's how to reproduce it:

----> 1 LineSegment2(Point2(*np.array([0,1.03])), Point2(*np.array([3.1, 2])))

/sw/lib/python2.7/site-packages/euclid.py in __init__(self, *args)
   1836             raise AttributeError('%r' % (args,))
   1837
-> 1838         if not self.v:
   1839             raise AttributeError('Line has zero-length vector')
   1840

TypeError: __nonzero__ should return bool or int, returned numpy.bool_

Weirdly, I get the same TypeError with LineSegment2(Point2(np.array([0,1.03])), Point2(np.array([3.1, 2]))) even though, by itself, Point2(np.array([0,1.03])) raises

/sw/lib/python2.7/site-packages/euclid.py in __repr__(self)
   1784 class Point2(Vector2, Geometry):
   1785     def __repr__(self):
-> 1786         return 'Point2(%.2f, %.2f)' % (self.x, self.y)
   1787
   1788     def intersect(self, other):

TypeError: float argument required, not numpy.ndarray

In any case, it also occurs with LineSegment2(Point2(np.array([0]), np.array([1.03])), Point2(np.array([3.1]), np.array([0.1])))

I think it's reasonable that this should work given that numpy singleton arrays may be generated in a calculation elsewhere and passed to a function that creates euclid Line2 objects.

bootchk commented 9 years ago

Thanks for clarifying. You call the numpy result a 'singleton array' (I should read about numpy.) Why shouldn't you convert that to float before passing it into pyeuclid? (Its not really my call, whether pyeuclid is supposed to work smoothly with numpy.)

I suppose a singleton array is an array with only one element. My only experience is with python array, and I can't remember whether an array with only one element say of type float can be used where that contained type is expected.

Now I see you use the unpack operator * in some of your problem cases but not all of them. Is that what you expect to unpack a numpy array into a scalar of type float? Maybe numpy implements that operator and is flawed? Again, I'm not an expert.

robclewley commented 9 years ago

Yes, I certainly could make the explicit conversions to floats, and that would solve the problem. I was just assuming (maybe wrongly!) that adding the bool() coercion is a harmless way to avoid user burden for that fussy overhead. numpy singletons often occur as the result of various matrix and vector processing algorithms. Admittedly, they lead to some "gotchas", like this one, but it's a long-standing feature of numpy that I'm sure is well defended somewhere. No biggie either way, it was just a suggestion :)

bootchk commented 9 years ago

I agree, no biggie, no zealotry. Just trying to understand.

If the result of ' someNumpyType != 0' ( the pyeuclid code) returns a type numpy.bool, I would guess that numpy is implementing the != operator and returning their own type numpy.bool, which fits in a byte. And numpy also implements 'or' operator which returns type numpy.bool_ ?

I don't have a problem with your solution, but maybe a comment is needed so someone doesn't remove the bool() later.

I wonder if ' 0 != self.x' would be a fix (would return an int instead of a numpy.bool_). But it would also need a comment.

robclewley commented 9 years ago

Well, I do know that np.array([3.1]) or np.array([0.1]) yields array([ 3.1]) because the first array evaluates to "True" in an if statement, while an empty array does not. So I'm not sure we can say that the or operator necessarily returns a numpy.bool_. That's out of my depth, though. If it would help, I can ask a question on the numpy users forum, where the real numpy experts are. This kind of problem must have come up before.

In any case, switching the order of the comparison still yields the same error. I think the numpy array implements something that overrides the int's comparison method.

If you, or whoever, decide to keep my suggestion, I'll be happy to add a comment to the code as part of the PR. Let me know if/when someone is willing to merge and I'll do that.

Cheers.