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

Segment: also use dot product test for contains #13314

Closed
wants to merge 4 commits into from

Conversation

cbm755
Copy link
Contributor

@cbm755 cbm755 commented Sep 14, 2017

This is coming from discussion in #13302 .

This currently fails at least one slow test. So just RFC for now. CC @jksuom, @siefkenj, for discussion.

This should be more robust especially if Floats are supported in
the future.  Check dot products before collinearity for possible
efficiency improvement (not quanitified).  The triangle inequality
approach (added in a previous commit) is still here, but commented
out until we find an example where it is needed.
@cbm755
Copy link
Contributor Author

cbm755 commented Sep 14, 2017

According to the tests,

p = Point(x,y)
L = Segment2D(Point2D(0, 0), Point2D(4, 4))
p in L

should be False. Whereas I might've thought None in the usual SymPy way. I don't know enough about the geometry module to know if this is deliberate. It happens because rank is used in is_collinear and it behaves that way:

>>> Matrix([[1,0],[1,x]]).rank()
2

If this is how it is supposed to work, I cannot swap the order of the collinearity test and the dot prod tests...

@jksuom
Copy link
Member

jksuom commented Sep 14, 2017

I would also have expected that is_collinear(p, L.p1, L.p2) return None in this case but that may be hard to arrange. Apparently Point regards p as a generic point, i.e., a point whose coordinates are algebraically independent. Maybe we'll have to accept this.

I think this was what the change in a previous commit [1] had in
mind when changing from dot prod test to triangle ineq.

[1] c5f0b00
@cbm755
Copy link
Contributor Author

cbm755 commented Sep 14, 2017

I reverted my swap. And I found a new test for the triangle inequality stuff.

So I figure we can either:

(a) cherry-pick the test only to master: 0da2db5

(b) discuss whether adding back the dot prod test is desirable (this PR).

Thoughts?

@cbm755 cbm755 changed the title [WIP] Segment: use dot product test for contains Segment: also use dot product test for contains Sep 14, 2017
@siefkenj
Copy link
Contributor

Unfortunately sympy matrices assume symbols are different form other values in a matrix, so Matrix([[1,0],[1,x]]).rank() returns 2, even though it made an assumption that x != 0. The rref routine, with a small modification, could be made to inform the user if any non-zero assumptions needed to be made (_find_reasonable_pivots reports its assumptions).

I knew there was some reason I used the triangle inequality, and it looks like complex symbols were it! The relationship between complex numbers and the geometry module seems weird at the moment. If a point is initialized with symbols, should those symbols be recast with the real=True assumption? Or should we allow complex numbers as entries and make all routines complex-friendly?

@cbm755
Copy link
Contributor Author

cbm755 commented Sep 14, 2017

I don't know enough about the geometry module to offer an informed opinion. Numerically stable algorithms with Floats would be nice to have too.

@jksuom
Copy link
Member

jksuom commented Sep 15, 2017

To say that a point p is in the segment p1p2 means that p = t*p1 + (1 - t)*p2 where t is in the interval [0, 1]. If the points are collinear then that is true for some t which can be explicitly computed from the coordinates (unless p1 == p2). Then there only remains to test whether that t is real and in the interval [0, 1]. That way there would be no need to declare everything real.

@czgdp1807
Copy link
Member

@cbm755 @jksuom Please provide updates on this PR.

@czgdp1807
Copy link
Member

@cbm755 Any updates on this?

@czgdp1807
Copy link
Member

>>> u, v, w, z = symbols('u, v, w, z')
>>> l = Segment(Point(u, w), Point(v, z))
>>> p = Point(2*u/3 + v/3, 2*w/3 + z/3)
>>> l.contains(p)
True

The above depicts that the tests in this PR pass on master. So, closing this for now. However, if there is a need please re-open this. Thanks.

@czgdp1807 czgdp1807 closed this Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants