Reflect method extended for all lines. #2896

akshayah3 · 2014-02-09T14:39:43Z

delete unused code
remove quote from docstring and wrap long line
keep deleted test
change str(result) to "%s == 0" % str(result)

smichr · 2014-02-09T18:06:01Z

Perhaps this could be a utility method but the intent of reflect of a GeometricEntity is to return the reflected entity, not an equation. A utility function, however, could be written to reflect an arbitrary expression across a given line.

akshayah3 · 2014-02-09T18:14:57Z

@smichr Reflected entity is possible but many a times we get an ellipse whose axes are not parallel to x and y axes so it is not possible to represent such an ellipse in the standard ellipse object form.

akshayah3 · 2014-02-09T18:30:41Z

@smichr

a=Ellipse(Point(3,4),1,3)

   >>> a.reflect(Line(Point(0,-4),Point(5,0)))

   (-40_x/41 + 9_y/41 + 364/41)**2/9 + (27_x/41 + 120_y/41 + 111/41)**2/9

The above expression is an ellipse which cant be represented in a standard ellipse object form like ,Ellipse(Point(x,y),a,b)

smichr · 2014-02-09T21:44:33Z

Yes...until the general ellipse is supported, this should just raise "NotImplementedError".

akshayah3 · 2014-02-09T21:54:45Z

@smichr So as most of the reflected entities(Ellipses) are not gonna be a standard ellipse.I think we should implement this as a utility function as you said which returns an equation than raising an error.

smichr · 2014-02-10T20:41:06Z

Can you return a Curve instead?

akshayah3 · 2014-02-10T21:40:05Z

@smichr Can you return a Curve instead?
No ,there is no way other way to represent a non standard ellipse than an expression.

smichr · 2014-02-11T14:09:37Z

It can be represented in polar form; that could be converted to x-y-coordinate form, couldn't it?

http://en.wikipedia.org/wiki/Ellipse#General_parametric_form

akshayah3 · 2014-02-11T14:54:32Z

@smichr Yes it can be so should I convert it to polar form?

akshayah3 · 2014-02-11T17:37:39Z

@smichr I just checked the polar equation and its even more complicated than the general expression when the center is not at the origin.The general expression looks much better than the polar form.

smichr · 2014-02-11T21:12:11Z

I'm just thinking how a GeometricEntity can be returned. So I guess a reflect function that takes an Expr and Line is the next best thing.

smichr · 2014-02-11T21:14:24Z

sympy/geometry/ellipse.py

+            b = a.reflect(line)
+            p = b.x
+            q = b.y
+            a1, a2, a3 = (p.coeff(sin(t)), p.coeff(cos(t)), p.as_independent(sin(t), cos(t))[0])


if a3 is an additive constant then you should use as_independent(sin(t), cos(t), as_Add=True)[0]

akshayah3 · 2014-02-11T21:16:09Z

@smichr I have also been thinking on those lines but an expression looks like a better solution .

akshayah3 · 2014-02-11T22:48:43Z

@smichr I've added the changes.

smichr · 2014-02-12T05:03:41Z

sympy/geometry/ellipse.py

@@ -539,12 +540,26 @@ def reflect(self, line):
        >>> from sympy import Circle, Line
        >>> Circle((0, 1), 1).reflect(Line((0, 0), (1, 1)))
        Circle(Point(1, 0), -1)
+        >>> Ellipse(Point(3,4),1,3).reflect(Line(Point(0,-4),Point(5,0)))


space after commas

And add a note indicating that until the general Ellipse is supported, an Expr is returned for the Ellipse with a non-horizontal axis.

And there must be something wrong with the calculation:

>>> e1,e2=solve(eq,y) >>> eq (-40*x/41 + 9*y/41 + 364/41)**2/9 + (27*x/41 + 120*y/41 + 111/41)**2/9 >>> e1 -320*x/1609 - 41*sqrt(-(41*x - 347)**2)/4827 - 1844/1609

e1 will always be imaginary (which means there is no value of x that will have a corresponding real value of y...so the answer must have gone wrong somewhere.

akshayah3 · 2014-02-12T05:09:09Z

@smichr I made some changes in the last commit. I added a -1 term .So that would solve this problem I guess

smichr · 2014-02-12T06:13:48Z

sympy/geometry/ellipse.py

@@ -539,12 +540,32 @@ def reflect(self, line):
        >>> from sympy import Circle, Line
        >>> Circle((0, 1), 1).reflect(Line((0, 0), (1, 1)))
        Circle(Point(1, 0), -1)
+        >>> Ellipse(Point(3,4), 1, 3).reflect(Line(Point(0,-4), Point(5,0)))
+        ((-40*x/41 + 9*y/41 + 364/41)**2/9 + (27*x/41 + 120*y/41 + 111/41)**2/9 - 1)


I believe the answer should be

1600*(9*x/40 + y + 37/40)**2/1681 + 1600*(x - 9*y/40 - 91/10)**2/15129 - 1

You can confirm by plotting the equations (in implicit form) at desmos.com/calculator. I used the equation I derived and posted at SO (referenced in issue #2815)

>>> ge -1 + (-s*(x - xc) + y - yc)**2/(m**2*(s**2 + 1)) + (s*(y - yc) + x - xc)**2/(M**2*(s**2 + 1)) >>> e=Ellipse((3,4),1,3) >>> line = Line(Point(0,-4), Point(5,0)) >>> ge.subs(zip((xc,yc), e.center.reflect(line).args)).subs(s, ... Line(e.center,slope=0 if (e.hradius==e.major) else oo).reflect(line).slope ... ).subs(m,e.minor).subs(M,e.major) 1600*(9*x/40 + y + 37/40)**2/1681 + 1600*(x - 9*y/40 - 91/10)**2/15129 - 1

akshayah3 · 2014-02-12T06:41:29Z

@smichr Our solutions are deferring a bit.I'll explain what I did:
a = self.arbitrary_point
b = a.reflect(line)
Above I took the arbitrary point of the given ellipse and reflected it over the given line.So the reflected point lies on the new Ellipse.
p = b.x
q = b.y
Seperated the x and y co-ordinates.
a1, a2, a3 = (p.coeff(sin(t)), p.coeff(cos(t)), p.as_independent(sin(t), cos(t), as_Add=True)[0])
b1, b2, b3 = (q.coeff(sin(t)), q.coeff(cos(t)), q.as_independent(sin(t), cos(t), as_Add=True)[0])
Now here p & q will always be of the form a_sin(t) +b_cos(t) + constant (because arbitrary point on the given Ellipse will be of the form Point(a_cos(t) + constant, b_sin(t) + constant) and reflecting this point on any line will be of the form of Point(a1_sin(t) +b1_cos(t) + constant , a2_sin(t) +b2_cos(t) + constant)
As p and q contain sin and cos I eliminated them by using sint + cost =1 and got this as the general solution for the ellipse.
denominator = (a1_b2 - a2_b1)**2
numerator = (y_a1 - x_b1 + a3_b1 - b3_a1)**2 + (y_a2 - x_b2 + a3_b2 - b3_a2)**2

smichr · 2014-02-12T12:21:39Z

sympy/geometry/util.py

@@ -424,4 +424,4 @@ def centroid(*args):
                A += a
            den = A
        c /= den
-        return c.func(*[i.simplify() for i in c.args])
+        return c.func(*[i.simplify() for i in c.args])


newline needed after end of this line

smichr · 2014-02-12T13:21:56Z

got this as the general

What do you mean "you eliminated cos(t) and sin(t)"; is your answer consistent with

>>> solve((p,q),cos(t),sin(t))
{cos(t): 37/41, sin(t): -364/123}

And how are you deriving the final equations?

akshayah3 · 2014-02-12T14:15:57Z

Solving both the equations (p q) for sin(t) and cos(t) and squared them and added them which results to one.

akshayah3 · 2014-02-12T14:24:24Z

@smichr I got equations following the above procedure.

akshayah3 · 2014-02-12T15:08:08Z

@smichr I'll explain it in little more detail.
p=a1_sin(t) +a2_cos(t) + a3
q=b1_sin(t) +b2_cos(t) + b3
multiplying p with b1 and q with a1 and subtracting both of them will give
p_b2 - q_a2 - a3_b2 + b3_a2 = sin(t) _(a1_b2 - b1_a2) (equation 1)
respectively
p_b1 - q_a1 - a3_b1 + b3_a1 = cos(t) (a1_b2 - b1a2)(equation 2)

Now squaring and adding equations 1 and 2 will give the result which I obtained.(and replacing p and q with x and y respectively)
Note that (a1_b2 - b1_a2)**2 is the denominator as present in the code.

smichr · 2014-02-13T23:00:40Z

sympy/geometry/ellipse.py

+            denominator = (a1*b2 - a2*b1)**2
+            numerator = (y*a1 - x*b1 + a3*b1 - b3*a1)**2 + (y*a2 - x*b2 + a3*b2 - b3*a2)**2
+            result = (numerator/denominator - 1)
+            raise NotImplementedError('General Ellipse is not supported', result)


('General Ellipse is not supported but the equation of the reflected Ellipse is:\n', result)

akshayah3 · 2014-02-14T02:47:27Z

@smichr
(x + z_(x + x_y)).expand()
x_y_z + x_z + x
.coeff(x)
y_z + z + 1
(x + z(x + x_y)).coeff(x)
1
This would not be a problem as I'm applying the _coeff to a term which is produced by the
function arbitrary_points which will always be in an expanded form.

smichr · 2014-02-14T14:15:22Z

sympy/geometry/ellipse.py

+            p = b.x
+            q = b.y
+            x, y = [_uniquely_named_symbol(name, p) for name in 'xy']
+            a1, a2, a3 = (p.coeff(sin(t)), p.coeff(cos(t)), p.as_independent(sin(t), cos(t), as_Add=True)[0])


Is coeff even necessary given the simplicity of the arbitrary point?

>>> var('a:d t');Ellipse((a,b),c,d).arbitrary_point(t) (a, b, c, d, t) Point(a + c*cos(t), b + d*sin(t)) a1,a2,a3 = 0,self.hradius,self.center.x b1,b2,b3 = self.vradius,0,self.center.y

akshayah3 · 2014-02-14T14:25:34Z

@smichr No I'm not applying coeff to the arbitrary point but to the new point which is the result of reflecting the arbitrary point through the given line.So depending on the line coeff is necessary I would say.
a=Ellipse(Point(3,4),1,2)
b=a.arbitrary_point() (Point(cos(t) + 3, 2_sin(t) + 4))
c=b.reflect(Line(Point(0,-4),Point(6,0)))
Point(24_sin(t)/13 + 5_cos(t)/13 + 111/13, -10_sin(t)/13 + 12*cos(t)/13 - 56/13)

smichr · 2014-02-14T22:16:11Z

Sorry about that. After posting I thought I might have made that mistake but couldn't get back to correct it until now.

Consider the following, though, to see what I mean about fragility:

>>> e
Ellipse(Point(c, d), 3, 2)
>>> l
Line(Point(a, b), Point(3, 4))
>>> e.reflect(line)
NotImplementedError: General Ellipse is not supported but the equation of the re
flected Ellipse is:
nan

>>> expr=e.equation()
>>> expr.subs(zip((x, y), Point(x, y).reflect(line).args), simultaneous=True)
(-c/3 + (x*(a - 3)**2 + (b - 4)*(-4*a + 3*b + y*(a - 3)) - (b - 4)*(4*a - 3*b +
x*(b - 4) - y*(a - 3)))/(3*((a - 3)**2 + (b - 4)**2)))**2 + (-d/2 + (-4*a + 3*b)
/(2*(-a + 3)) + ((a - 3)**2*(4*a - 3*b + x*(b - 4) - y*(a - 3)) + (b - 4)*(x*(a
- 3)**2 + (b - 4)*(-4*a + 3*b + y*(a - 3))))/(2*(a - 3)*((a - 3)**2 + (b - 4)**2
)))**2 - 1

akshayah3 · 2014-02-14T22:29:14Z

@smichr Thank you for pointing out.It fails in the cases like above.So then i'll use your method and commit?

smichr · 2014-02-15T03:20:14Z

I made a checklist at the top of this page. Please just check off the items as you are done and then I will know this is ready to look at again. It's close!

smichr · 2014-02-15T14:38:15Z

sympy/geometry/ellipse.py

        if line.slope in (0, oo):
            c = self.center
            c = c.reflect(line)
            return self.func(c, -self.hradius, self.vradius)
-        raise NotImplementedError('reflection line not horizontal | vertical.')
+        else:
+            a = self.arbitrary_point()


delete these 3 lines (a, b, p)

akshayah3 · 2014-02-15T15:29:02Z

@smichr I have made the changes.

akshayah3 · 2014-02-15T15:30:52Z

@smichr Duplicate symbols are only being checked for the Ellipse's equation but what about the given line .What if it contains some duplicate symbols?

smichr · 2014-02-15T15:35:47Z

sympy/geometry/ellipse.py

        """
+        def _uniquely_named_symbol(xname, expr):


let's change expr -> syms

akshayah3 · 2014-02-15T15:42:58Z

@smichr But how will the above change remove the line's duplicate symbols?

akshayah3 · 2014-02-15T15:43:52Z

delete these 3 lines (a, b, p)
The reason for including these 3 lines were so to remove those duplicate symbols from line and ellipse.

smichr · 2014-02-15T16:03:01Z

sympy/geometry/ellipse.py

-        raise NotImplementedError('reflection line not horizontal | vertical.')
+        else:
+            expr = self.equation()
+            x, y = [_uniquely_named_symbol(name, expr) for name in 'xy']


That's a good catch regarding line -- how about the changes above with the following?

expr -> `expr.free_symbols.union(line.free_symbols)`

Or we could pass the expressions like _uniquely_named_symbol(name, expr, line) and then have def _uniquely_named_symbol(xname, *exprs) and do ... for s in set.union(*[e.free_symbols for e in exprs])

akshayah3 · 2014-02-15T16:05:20Z

@smichr expr -> expr.free_symbols.union(line.free_symbols)
This looks good.

akshayah3 · 2014-02-15T16:14:42Z

@smichr Ellipse(Point(3, 4), 1, 3).reflect(Line(Point(0, -4), Point(5, 0)))
(x - 3)**2 + (y/3 - 4/3)**2 - 1 == 0 I am getting this result.
Are you getting the same?

akshayah3 · 2014-02-15T16:15:53Z

@smichr
a = self.arbitrary_point()
b = a.reflect(line)
p = b.x

       x, y = [_uniquely_named_symbol(name, p) for name in 'xy']

This removes the duplicate symbols of both.

smichr · 2014-02-15T17:14:28Z

I'll open a new branch with a few corrections in which I get

>>> from sympy import *
>>> Ellipse(Point(3, 4), 1, 3).reflect(Line(Point(0, -4), Point(5, 0)))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sympy\geometry\ellipse.py", line 584, in reflect
    "f(%s, %s) = %s" % (str(x), str(y), str(result))))
NotImplementedError:
General Ellipse is not supported but the equation of the reflected
Ellipse is given by the zeros of: f(x, y) = (9*x/41 + 40*y/41 +
37/41)**2 + (40*x/123 - 3*y/41 - 364/123)**2 - 1

smichr · 2014-02-15T17:19:20Z

See #2911

Reflect method extended for all lines.

b277902

smichr reviewed Feb 11, 2014
View reviewed changes

akshayah3 added 2 commits February 12, 2014 02:53

Reflect method extended for all lines.

769a8b6

Reflect method extended for all lines.

09559db

smichr reviewed Feb 12, 2014
View reviewed changes

akshayah3 added 3 commits February 12, 2014 10:42

Reflect method extended for all lines.

e0e936f

Reflect method extended for all lines.

71ad806

Reflect method extended for all lines.

801eeaf

smichr reviewed Feb 12, 2014
View reviewed changes

Reflect method extended for all lines.

1b777af

smichr reviewed Feb 12, 2014
View reviewed changes

smichr reviewed Feb 13, 2014
View reviewed changes

Reflect method extended for all lines.

ea90176

smichr reviewed Feb 14, 2014
View reviewed changes

Reflect method extended for all lines.

5b06d6b

Reflect method extended for all lines.

8af6f8b

smichr reviewed Feb 15, 2014
View reviewed changes

Reflect method extended for all lines.

f59e278

smichr reviewed Feb 15, 2014
View reviewed changes

sympy/geometry/ellipse.py

"""

def _uniquely_named_symbol(xname, expr):

Copy link

Member

smichr Feb 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's change expr -> syms

smichr reviewed Feb 15, 2014
View reviewed changes

smichr closed this Feb 15, 2014

Reflect method extended for all lines. #2896

Reflect method extended for all lines. #2896

Conversation

akshayah3 commented Feb 9, 2014

smichr commented Feb 9, 2014

akshayah3 commented Feb 9, 2014

akshayah3 commented Feb 9, 2014

smichr commented Feb 9, 2014

akshayah3 commented Feb 9, 2014

smichr commented Feb 10, 2014

akshayah3 commented Feb 10, 2014

smichr commented Feb 11, 2014

akshayah3 commented Feb 11, 2014

akshayah3 commented Feb 11, 2014

smichr commented Feb 11, 2014

Choose a reason for hiding this comment

akshayah3 commented Feb 11, 2014

akshayah3 commented Feb 11, 2014

Choose a reason for hiding this comment

akshayah3 commented Feb 12, 2014

Choose a reason for hiding this comment

akshayah3 commented Feb 12, 2014

Choose a reason for hiding this comment

smichr commented Feb 12, 2014

akshayah3 commented Feb 12, 2014

akshayah3 commented Feb 12, 2014

akshayah3 commented Feb 12, 2014

Choose a reason for hiding this comment

akshayah3 commented Feb 14, 2014

Choose a reason for hiding this comment

akshayah3 commented Feb 14, 2014

smichr commented Feb 14, 2014

akshayah3 commented Feb 14, 2014

smichr commented Feb 15, 2014

Choose a reason for hiding this comment

akshayah3 commented Feb 15, 2014

akshayah3 commented Feb 15, 2014

Choose a reason for hiding this comment

akshayah3 commented Feb 15, 2014

akshayah3 commented Feb 15, 2014

Choose a reason for hiding this comment

akshayah3 commented Feb 15, 2014

akshayah3 commented Feb 15, 2014

akshayah3 commented Feb 15, 2014

smichr commented Feb 15, 2014

smichr commented Feb 15, 2014