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

abs(pi+I) = pi+I #11155

Closed
sagetrac-benreynwar mannequin opened this issue Apr 8, 2011 · 14 comments
Closed

abs(pi+I) = pi+I #11155

sagetrac-benreynwar mannequin opened this issue Apr 8, 2011 · 14 comments

Comments

@sagetrac-benreynwar
Copy link
Mannequin

sagetrac-benreynwar mannequin commented Apr 8, 2011

abs(pi+I) returns pi+I

one would have expected:

sqrt(pi^2+1)

Apply attachment: trac_11155-doctest.patch

Depends on #12950

CC: @burcin

Component: symbolics

Author: Alexei Sheplyakov, Titus Nicolae

Reviewer: Burcin Erocal

Merged: sage-5.1.beta4

Issue created by migration from https://trac.sagemath.org/ticket/11155

@kcrisman
Copy link
Member

comment:1

In the future, be sure to pick a component (for instance, calculus or symbolics); that will help people find it more easily. Thanks for the bug report; mathematically incorrect is definitely bad!

I can confirm this. Moving to major.

Note the following.

sage: abs(pi+i)
pi + I
sage: abs(1+i)
sqrt(2)
sage: abs(n(pi)+i)
3.29690830947562

Pynac is somehow missing this. Note that it gets e right.

sage: abs(e+i)
abs(e + I)

@williamstein

This comment has been minimized.

@williamstein
Copy link
Contributor

comment:2

I can confirm this further. It's mathematically seriously incorrect... so now moving from major to critical, and cc'ing burcin.

Note:

sage: z = pi + i
sage: abs(z)
pi + I
sage: sqrt((z*z.conjugate()).expand())
sqrt(pi^2 + 1)

@burcin
Copy link
Contributor

burcin commented Feb 28, 2012

comment:3

Reported to the ginac-devel list:

http://thread.gmane.org/gmane.comp.mathematics.ginac.devel/1363

More detailed about the problem are in my message linked above.

@kcrisman
Copy link
Member

comment:4

Apparently already fixed!

@kcrisman
Copy link
Member

comment:5

Here is the diff.

diff --git a/check/exam_numeric.cpp b/check/exam_numeric.cpp
index 715acff..d5ae27b 100644 (file)
--- a/check/exam_numeric.cpp
+++ b/check/exam_numeric.cpp
@@ -68,6 +68,11 @@ static unsigned exam_numeric1()
                     << " erroneously not recognized as complex rational" << endl;
                ++result;
        }
+       if (test_crat.info(info_flags::nonnegative)) {
+               clog << test_crat
+                    << " erroneously recognized as non-negative number" << endl;
+               ++result;
+       }
        
        int i = numeric(1984).to_int();
        if (i-1984) {
diff --git a/ginac/numeric.cpp b/ginac/numeric.cpp
index f05763d..18725b2 100644 (file)
--- a/ginac/numeric.cpp
+++ b/ginac/numeric.cpp
@@ -700,7 +700,7 @@ bool numeric::info(unsigned inf) const
                case info_flags::negative:
                        return is_negative();
                case info_flags::nonnegative:
-                       return !is_negative();
+                       return is_zero() || is_positive();
                case info_flags::posint:
                        return is_pos_integer();
                case info_flags::negint:

@kcrisman
Copy link
Member

kcrisman commented Mar 1, 2012

comment:6

Making this change in Pynac does fix it:

sage: abs(pi+i)
abs(pi + I)
sage: abs(pi+i).n()
3.29690830947562

So now we just need to package this up. See #12501 for the most recently merged Pynac.

@sagetrac-titusn
Copy link
Mannequin

sagetrac-titusn mannequin commented May 12, 2012

comment:7

Attachment: trac_11155-doctest.patch.gz

Pull request of the patch to the main repository https://bitbucket.org/burcin/pynac/pull-request/1/bugfix-numeric-info-nonnegative-correctly

@burcin
Copy link
Contributor

burcin commented May 15, 2012

comment:8

Pynac 0.2.4 available in #12950 contains the upstream fix. This ticket can be closed once that is merged.

@burcin
Copy link
Contributor

burcin commented May 15, 2012

Reviewer: Burcin Erocal

@burcin
Copy link
Contributor

burcin commented May 15, 2012

Author: Alexei Sheplyakov, Titus Nicolae

@burcin

This comment has been minimized.

@burcin
Copy link
Contributor

burcin commented May 23, 2012

Dependencies: #12950

@jdemeyer
Copy link
Contributor

Merged: sage-5.1.beta4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants