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

Simplify of small imaginary number yields 0 #9398

Closed
TheLartians opened this Issue May 12, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@TheLartians
Copy link

TheLartians commented May 12, 2015

Using the most recent release (0.7.6) the following expression yields 0:

from sympy import *
(I*Number(1.)*Number(10)**Number(-14)).simplify()

Using the absolute value this does not happen.

 (Number(1.)*Number(10)**Number(-14)).simplify()
@Sumith1896

This comment has been minimized.

Copy link
Member

Sumith1896 commented May 12, 2015

On master

In [7]: (I*Number(1)*Number(10)**Number(-14)).simplify()
Out[7]: I/100000000000000

In [8]: (Number(1)*Number(10)**Number(-14)).simplify()
Out[8]: 1/100000000000000

In [9]: (I*Number(1.0)*Number(10)**Number(-14)).simplify()
Out[9]: 0

In [10]: (Number(1.0)*Number(10)**Number(-14)).simplify()
Out[10]: 1.00000000000000e-14

Doesn't seem to happen with ints, but happens with float.

@certik certik added the Wrong Result label May 16, 2015

@certik

This comment has been minimized.

Copy link
Member

certik commented May 16, 2015

The problem is with simplifying floating points, e.g. with the latest master (d9b59fd):

In [1]: simplify(1e-14*I)
Out[1]: 0

In [2]: simplify(1e-14)
Out[2]: 1.00000000000000e-14

Here [2] is correct, but [1] is wrong.

@certik

This comment has been minimized.

Copy link
Member

certik commented May 16, 2015

The problem is on the line

_e = cancel(expr)
, due to the fact that:

In [1]: cancel(1e-14*I)
Out[1]: 0

In [2]: cancel(1e-14)
Out[2]: 1.00000000000000e-14

So simplify() then sees 0 as one of the options and since it is short, it returns it. We need to fix cancel().

@certik

This comment has been minimized.

Copy link
Member

certik commented May 16, 2015

Found the problem. Mpmath converts 1e-14 to 0 here:

> /home/ondrej/.hashdist/bld/profile/rt2qwfzuuaxa/lib/python2.7/site-packages/mpmath/ctx_mp_python.py(77)__new__()
     76             v = new(cls)
---> 77             v._mpf_ = mpf_pos(cls.mpf_convert_arg(val, prec, rounding), prec, rounding)
     78             return v

Where:

ipdb> p val
1.00000000000000e-14
ipdb> p prec
53
ipdb> p v
0.0

Which might be correct. This is called from:

> /home/ondrej/repos/sympy/sympy/polys/domains/realfield.py(72)from_sympy()
     71         if number.is_Number:
---> 72             return self.dtype(number)

where number=1e-14 and it returns 0. The issue is that polynomials convert the floating point number to a real number RR which are """Real numbers up to the given precision. """ .

@certik

This comment has been minimized.

Copy link
Member

certik commented May 16, 2015

So a simple workaround is this:

diff --git a/sympy/polys/domains/realfield.py b/sympy/polys/domains/realfield.py
index a34aa81..86aa367 100644
--- a/sympy/polys/domains/realfield.py
+++ b/sympy/polys/domains/realfield.py
@@ -25,7 +25,7 @@ class RealField(Field, CharacteristicZero, SimpleDomain):
     has_assoc_Ring = False
     has_assoc_Field = True

-    _default_precision = 53
+    _default_precision = 100

     @property
     def has_default_precision(self):

Now it works:

In [1]: simplify(1e-14*I)
Out[1]: 1.0e-14⋅ⅈ

as well as the original issue:

In [2]: (I*Number(1.0)*Number(10)**Number(-14)).simplify()
Out[2]: 1.0e-14⋅ⅈ

The patch just increases the precision of the RR ring. Now we just need to figure out how to do this automatically somehow, perhaps based on the accuracy of the numbers that enter simplify().

certik added a commit to certik/sympy that referenced this issue May 16, 2015

Use correct accuracy for the RR domain
We use two extra guard digits from the maximum precision of all coefficients.

Previously:

In [1]: simplify(1e-14*I)
Out[1]: 0

Now:

In [1]: simplify(1e-14*I)
Out[1]: 1.0e-14⋅ⅈ

Fixes sympy#9398
@certik

This comment has been minimized.

Copy link
Member

certik commented May 16, 2015

I posted a possible fix in #9403.

Do you think this is the right fix for this problem?

certik added a commit to certik/sympy that referenced this issue May 17, 2015

Add a test for sympy#9398
We test the failure on simple cases first, and then on the actual problem that
was reported.

@certik certik closed this in #9403 Jun 5, 2015

skirpichev added a commit to diofant/diofant that referenced this issue Jul 1, 2015

Use correct accuracy for the RR domain
Previously:

In [1]: simplify(1e-14*I)
Out[1]: 0

Now:

In [1]: simplify(1e-14*I)
Out[1]: 1.0e-14⋅ⅈ

Fixes sympy/sympy#9398

// edited by skirpichev

skirpichev added a commit to diofant/diofant that referenced this issue Jul 1, 2015

Add a test for sympy/sympy#9398
We test the failure on simple cases first, and then on the actual problem that
was reported.

// edited by skirpichev

skirpichev added a commit to diofant/diofant that referenced this issue Jul 1, 2015

Use correct accuracy for the RR domain
Previously:

In [1]: simplify(1e-14*I)
Out[1]: 0

Now:

In [1]: simplify(1e-14*I)
Out[1]: 1.0e-14⋅ⅈ

Fixes sympy/sympy#9398

We test the failure on simple cases first, and then on the actual
problem that was reported.

// edited by skirpichev

skirpichev added a commit to diofant/diofant that referenced this issue Jul 3, 2015

Use correct accuracy for the RR domain
Previously:

In [1]: simplify(1e-14*I)
Out[1]: 0

Now:

In [1]: simplify(1e-14*I)
Out[1]: 1.0e-14⋅ⅈ

Fixes sympy/sympy#9398

We test the failure on simple cases first, and then on the actual
problem that was reported.

// edited by skirpichev

skirpichev added a commit to diofant/diofant that referenced this issue Jul 3, 2015

Use correct accuracy for the RR domain
Previously:

In [1]: simplify(1e-14*I)
Out[1]: 0

Now:

In [1]: simplify(1e-14*I)
Out[1]: 1.0e-14⋅ⅈ

Fixes sympy/sympy#9398

We test the failure on simple cases first, and then on the actual
problem that was reported.

// edited by skirpichev
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment