[PATCH] sqrt(x**2) == abs(x) works only when x is real (#600)
Kirill Smelkov
> # HG changeset patch
> # User Jaroslaw Tworek <dev...@gmail.com>
> # Date 1200260329 -3600
> # Node ID 4c7fa9eb04fe92b5c86b041428f6bfa734ed145c
> # Parent 3d032940e7340edf08b23a090a580be703c3eea1
> sqrt(x**2) == abs(x) works only when x is real (#600)
> * * *
>
> diff --git a/sympy/core/add.py b/sympy/core/add.py
> --- a/sympy/core/add.py
> +++ b/sympy/core/add.py
> @@ -227,6 +227,10 @@ class Add(AssocOp, RelMeths, ArithMeths)
> if c.is_nonpositive and r.is_nonpositive:
> return False
>
> + n = self.evalf()
> + if isinstance(n, Basic.Number):
> + return n.is_positive
> +
> def _eval_is_negative(self):
> c = self[0]
> r = Add(*self[1:])
> @@ -246,6 +250,9 @@ class Add(AssocOp, RelMeths, ArithMeths)
> return True
> if c.is_nonnegative and r.is_nonnegative:
> return False
> + n = self.evalf()
> + if isinstance(n, Basic.Number):
> + return n.is_negative
A test for it is lacking.
Maybe check that E**pi - pi**E is positive?
Also, it is not clear from the first glance why Add.is_pos/neg needs evalf, and
there is no need for evalf in Mul and Pow.
Also, maybe such fallbacks should live in general assumption code, so
that it first asks expr if it is say 'is_positive', and if the result is
not known -- it calls evalf?
>
> def as_coeff_terms(self, x=None):
> # -2 + 2 * a -> -1, 2-2*a
> diff --git a/sympy/core/function.py b/sympy/core/function.py
> --- a/sympy/core/function.py
> +++ b/sympy/core/function.py
> @@ -279,7 +279,7 @@ class Function(Basic, ArithMeths, RelMet
>
> def _eval_expand_complex(self, *args):
> func = self.func(*[ a._eval_expand_complex(*args) for a in self ])
> - return Basic.Re()(func) + S.ImaginaryUnit * Basic.Im()(func)
> + return Basic.re(func) + S.ImaginaryUnit * Basic.im(func)
Oops, I've spot this when reviewing sqrt class -> function.
But there is also similar bug in Pow.
btw: it is semantically different change -- maybe put such changes in
separate patches? Let's respect each other and help each other to
see/understand/review each other commitments?
>
> def _eval_rewrite(self, pattern, rule, **hints):
> if hints.get('deep', False):
> diff --git a/sympy/core/power.py b/sympy/core/power.py
> --- a/sympy/core/power.py
> +++ b/sympy/core/power.py
> @@ -72,13 +72,10 @@ class Pow(Basic, ArithMeths, RelMeths):
> def _eval_power(self, other):
> if isinstance(other, Basic.Number):
> if self.base.is_real:
> - if isinstance(self.exp, Basic.Number):
> - # (a ** 2) ** 3 -> a ** (2 * 3)
> - return Pow(self.base, self.exp * other)
> - if isinstance(other, Basic.Rational):
> - if self.exp.is_even and Basic.Integer(other.q).is_even:
> - return abs( Pow(self.base, self.exp * other))
> - return Pow(self.base, self.exp * other)
> + if isinstance(other, Basic.Rational):
> + if self.exp.is_even and Basic.Integer(other.q).is_even:
> + return abs( Pow(self.base, self.exp * other))
> + return Pow(self.base, self.exp * other)
This is the core of the patch, and it seems ok.
However maybe let's create an issue for (z**a)**b == z**(a*b), since it
sometimes useful to work with only principal branch.
Maybe z=Symbol(complex=True, principal_only=True) will lead to such
behaviour? But I'm not sure...
> if isinstance(other, Basic.Integer):
> # (a ** b) ** 3 -> a ** (3 * b)
> return Pow(self.base, self.exp * other)
> diff --git a/sympy/core/tests/test_eval_power.py b/sympy/core/tests/test_eval_power.py
> --- a/sympy/core/tests/test_eval_power.py
> +++ b/sympy/core/tests/test_eval_power.py
> @@ -34,7 +34,7 @@ def test_issue350():
> #test if powers are simplified correctly
> a = Symbol('a')
> assert ((a**Rational(1,3))**Rational(2)) == a**Rational(2,3)
> - assert ((a**Rational(3))**Rational(2,5)) == a**Rational(6,5)
> + assert ((a**Rational(3))**Rational(2,5)) != a**Rational(6,5)
We change this test from positive from negative -- this is ok, but a new
positive test with what the result should be is also asks us for inclusion.
>
> a = Symbol('a', real = True)
> assert (a**Rational(3))**Rational(2,5) == a**Rational(6,5)
> diff --git a/sympy/core/tests/test_numbers.py b/sympy/core/tests/test_numbers.py
> --- a/sympy/core/tests/test_numbers.py
> +++ b/sympy/core/tests/test_numbers.py
> @@ -204,6 +204,6 @@ def test_issue324():
> assert sqrt(x-1) != I*(1-x)**Rational(1,2)
>
> def test_issue350():
> - x = Symbol("x")
> + x = Symbol("x",real=True)
> assert sqrt(x**2) == abs(x)
> assert sqrt(x-1).subs(x,5) == 2
> diff --git a/sympy/functions/elementary/complexes.py b/sympy/functions/elementary/complexes.py
> --- a/sympy/functions/elementary/complexes.py
> +++ b/sympy/functions/elementary/complexes.py
> @@ -228,6 +228,10 @@ class abs(Function):
> def _eval_conjugate(self):
> return self
>
> + @property
> + def is_real(self):
> + return self[0].is_real
> +
Hmm, isn't abs(complex) real?
Also I though abs is always real and non-negative?
So I think we should put as class constants into abs:
is_real = True
is_nonnegative = True
Right?
> class arg(Function):
>
> nargs = 1
> diff --git a/sympy/functions/elementary/tests/test_complexes.py b/sympy/functions/elementary/tests/test_complexes.py
> --- a/sympy/functions/elementary/tests/test_complexes.py
> +++ b/sympy/functions/elementary/tests/test_complexes.py
> @@ -76,6 +76,13 @@ def test_im():
> assert im(log(2*I)) == pi/2
>
> def test_abs():
> - x, y = symbols('xy')
> + x = Symbol('x', real=True)
> assert sqrt(x**2) == abs(x)
> assert abs(x).diff(x) == sign(x)
> +
> + y = Symbol('y')
> + assert sqrt(y**2) != abs(y)
> +
> +def test_imabs():
> + th = Symbol("theta", real = True)
> + assert im(abs(th)) == 0
> diff --git a/sympy/functions/special/tests/test_spherical_harmonics.py b/sympy/functions/special/tests/test_spherical_harmonics.py
> --- a/sympy/functions/special/tests/test_spherical_harmonics.py
> +++ b/sympy/functions/special/tests/test_spherical_harmonics.py
> @@ -35,25 +35,25 @@ def test_Plmcos():
> #http://en.wikipedia.org/wiki/Legendre_function
> th = Symbol("th", real = True)
> assert Plmcos(0, 0, th) == 1
> - assert Plmcos(1, -1, th) == sin(th)/2
> + assert Plmcos(1, -1, th) == abs(sin(th))/2
> assert Plmcos(1, 0, th) == cos(th)
> - assert Plmcos(1, 1, th) == -sin(th)
> + assert Plmcos(1, 1, th) == -abs(sin(th))
> assert Plmcos(2, 0, th) == (3*cos(th)**2-1)/2
> - assert Plmcos(2, 1, th) == -3*cos(th)*sin(th)
> + assert Plmcos(2, 1, th) == -3*cos(th)*abs(sin(th))
> assert Plmcos(2, 2, th) in [3*sin(th)**2, 3*(1-cos(th)**2)]
> assert Plmcos(3, 0, th) == (5*cos(th)**3-3*cos(th))/2
> - assert Plmcos(3, 1, th) == -3*(5*cos(th)**2-1)/2 *sin(th)
> + assert Plmcos(3, 1, th) == -3*(5*cos(th)**2-1)/2 *abs(sin(th))
> assert Plmcos(3, 2, th) == 15*cos(th)*sin(th)**2
> - assert Plmcos(3, 3, th) == -15*sin(th)**3
> + assert Plmcos(3, 3, th) == -15*abs(sin(th)**3)
>
> def test_Ylm():
> #http://en.wikipedia.org/wiki/Spherical_harmonics
> th, ph = Symbol("theta", real = True), Symbol("phi", real = True)
> assert Ylm(0, 0, th, ph) == sympify(1)/(2*sqrt(pi))
> - assert Ylm(1, -1, th, ph) == sympify(1)/2 * sqrt(3/(2*pi)) * sin(th) * \
> + assert Ylm(1, -1, th, ph) == sympify(1)/2 * sqrt(3/(2*pi)) * abs(sin(th)) * \
> exp(-I*ph)
> assert Ylm(1, 0, th, ph) == sympify(1)/2 * sqrt(3/pi) * cos(th)
> - assert Ylm(1, 1, th, ph) == -sympify(1)/2 * sqrt(3/(2*pi)) * sin(th) * \
> + assert Ylm(1, 1, th, ph) == -sympify(1)/2 * sqrt(3/(2*pi)) * abs(sin(th)) * \
> exp(I*ph)
> #Ylm returns here a correct, but different expression:
> #assert Ylm(2, -2, th, ph).expand() == (sympify(1)/4 * sqrt(15/(2*pi)) * \
> @@ -61,7 +61,7 @@ def test_Ylm():
> assert Ylm(2, 0, th, ph).expand() == (sympify(1)/4 * sqrt(5/pi) * \
> (3*cos(th)**2-1)).expand()
> assert Ylm(2, 1, th, ph).expand() == (-sympify(1)/2 * \
> - sqrt(3)*sqrt(5/(2*pi)) * (sin(th)*cos(th)) * exp(I*ph)).expand()
> + sqrt(3)*sqrt(5/(2*pi)) * (abs(sin(th))*cos(th)) * exp(I*ph)).expand()
> #Ylm returns here a correct, but different expression:
> #assert Ylm(2, 2, th, ph).expand() == (sympify(1)/4 * sqrt(15/(2*pi)) * \
> # sin(th)**2 * exp(2*I*ph)).expand()
> @@ -70,11 +70,12 @@ def test_Zlm():
> #http://en.wikipedia.org/wiki/Solid_harmonics#List_of_lowest_functions
> th, ph = Symbol("theta", real = True), Symbol("phi", real = True)
> assert Zlm(0, 0, th, ph) == sqrt(1/(4*pi))
> - assert Zlm(1, -1, th, ph) == sqrt(3/(4*pi))*sin(th)*sin(ph)
> + assert Zlm(1, -1, th, ph) == sqrt(3/(4*pi))*abs(sin(th))*sin(ph)
> assert Zlm(1, 0, th, ph) == sqrt(3/(4*pi))*cos(th)
> - assert Zlm(1, 1, th, ph) == sqrt(3/(4*pi))*sin(th)*cos(ph)
> + assert Zlm(1, 1, th, ph) == sqrt(3/(4*pi))*abs(sin(th))*cos(ph)
>
> - assert Zlm(2, -1, th, ph) == sqrt(15/(4*pi))*sin(th)*cos(th)*sin(ph)
> + assert Zlm(2, -1, th, ph) == sqrt(15/(4*pi))*abs(sin(th))*cos(th)*sin(ph)
> +
> assert Zlm(2, 0, th, ph).expand() == (sympify(1)/4 * sqrt(5/pi) * \
> (3*cos(th)**2-1)).expand()
> - assert Zlm(2, 1, th, ph) == sqrt(15/(4*pi))*sin(th)*cos(th)*cos(ph)
> + assert Zlm(2, 1, th, ph) == sqrt(15/(4*pi))*abs(sin(th))*cos(th)*cos(ph)
Please XFAIL this.
It's not Ylm/Zlm tests who should be fixed -- it's Ylm/Zlm.
I'm ok with Y/Z being broken by this patch -- let's create separate
issue for this.
Overall looks good, thanks.
--
Всего хорошего, Кирилл.
http://landau.phys.spbu.ru/~kirr/aiv/
Ondrej Certik
I respect you very much, if you mean this. :) I like that you spotted
a lot of potential problems with the patches.
Only I don't have time today and tomorrow, so I cannot rework this now.
I wasn't sure about it, so I just implemented this. But I think
you are right - abs(a+b*I) is still real and positive. So you are right.
The above results are correct, aren't they? the abs() should simplify.
Or, if we rework Ylm and Zlm,
it will work too - in which case we'll change the tests again. If we
xfail them, we can never be sure
in the future, that we don't break the code in Ylm and Zlm by some
further changes.
So I am for leaving the tests and creating a new issue to rework Ylm.
Ondrej
Kirill Smelkov
> > > diff --git a/sympy/core/function.py b/sympy/core/function.py
> > > --- a/sympy/core/function.py
> > > +++ b/sympy/core/function.py
> > > @@ -279,7 +279,7 @@ class Function(Basic, ArithMeths, RelMet
> > >
> > > def _eval_expand_complex(self, *args):
> > > func = self.func(*[ a._eval_expand_complex(*args) for a in self ])
> > > - return Basic.Re()(func) + S.ImaginaryUnit * Basic.Im()(func)
> > > + return Basic.re(func) + S.ImaginaryUnit * Basic.im(func)
> >
> > Oops, I've spot this when reviewing sqrt class -> function.
> > But there is also similar bug in Pow.
> >
> > btw: it is semantically different change -- maybe put such changes in
> > separate patches? Let's respect each other and help each other to
> > see/understand/review each other commitments?
>
> I respect you very much, if you mean this. :) I like that you spotted
> a lot of potential problems with the patches.
I mean let's assist each other -- it is easier to deal with one patch at
a time which does one thing and does it well.
> Only I don't have time today and tomorrow, so I cannot rework this now.
This is ok, we are not in a hurry.
btw: the patch author is Jaroslaw, so why don't he prepare this patch by
himself? I've got the impressions that he would be capable of doing it.
Jaroslaw? :)
[...]
> > > diff --git a/sympy/functions/elementary/complexes.py b/sympy/functions/elementary/complexes.py
> > > --- a/sympy/functions/elementary/complexes.py
> > > +++ b/sympy/functions/elementary/complexes.py
> > > @@ -228,6 +228,10 @@ class abs(Function):
> > > def _eval_conjugate(self):
> > > return self
> > >
> > > + @property
> > > + def is_real(self):
> > > + return self[0].is_real
> > > +
> >
> > Hmm, isn't abs(complex) real?
> > Also I though abs is always real and non-negative?
> >
> > So I think we should put as class constants into abs:
> >
> > is_real = True
> > is_nonnegative = True
> >
> > Right?
>
> I wasn't sure about it, so I just implemented this. But I think
> you are right - abs(a+b*I) is still real and positive. So you are right.
ok. should I put this into separate patch to assist you and Jaroslaw, or
should I wait?
Ok, I agree.
This will be high-priority for me -- I need that spherical functions.
Ondrej Certik
If you can do it, it'd be awesome. Or ask Jaroslaw in the issue or IRC.
I'd like these patches to get in, so that we don't have to think about
it, but I cannot do it today.
Thanks very much,
Ondrej
Kirill Smelkov
Relevant patches are here:
http://landau.phys.spbu.ru/~kirr/cgi-bin/hg.cgi/sympy--kirr-patches/file/tip/abs-is-real.patch
http://landau.phys.spbu.ru/~kirr/cgi-bin/hg.cgi/sympy--kirr-patches/file/tip/abs-zero.patch
Unfortunately abs-is-real depends on
http://code.google.com/p/sympy/issues/detail?id=614
Ondrej Certik
On Jan 15, 9:55 am, Kirill Smelkov <k...@landau.phys.spbu.ru> wrote:
> On Mon, Jan 14, 2008 at 02:09:22PM +0100, Ondrej Certik wrote:
>
Please apply the abs-zero.patch, looks good to me.
Ondrej
Kirill Smelkov
> > Relevant patches are here:
> >
> > http://landau.phys.spbu.ru/~kirr/cgi-bin/hg.cgi/sympy--kirr-patches/f...http://landau.phys.spbu.ru/~kirr/cgi-bin/hg.cgi/sympy--kirr-patches/f...
> >
> > Unfortunately abs-is-real depends on
> >
> > http://code.google.com/p/sympy/issues/detail?id=614
>
> Please apply the abs-zero.patch, looks good to me.
abs-zero.patch commited.