Opened 10 years ago

Closed 8 years ago

#11155 closed defect (fixed)

abs(pi+I) = pi+I

Reported by: benreynwar Owned by: burcin
Priority: critical Milestone: sage-5.1
Component: symbolics Keywords:
Cc: burcin Merged in: sage-5.1.beta4
Authors: Alexei Sheplyakov, Titus Nicolae Reviewers: Burcin Erocal
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12950 Stopgaps:

Description (last modified by burcin)

abs(pi+I) returns pi+I

one would have expected:


Apply attachment:trac_11155-doctest.patch

Attachments (1)

trac_11155-doctest.patch (665 bytes) - added by titusn 9 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by kcrisman

  • Component changed from PLEASE CHANGE to symbolics
  • Owner changed from tbd to burcin
  • Priority changed from minor to major

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)
sage: abs(n(pi)+i)

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

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

comment:2 Changed 9 years ago by was

  • Cc burcin added
  • Description modified (diff)
  • Milestone set to sage-5.0
  • Priority changed from major to critical

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


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

comment:3 Changed 9 years ago by burcin

Reported to the ginac-devel list:

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

comment:4 Changed 9 years ago by kcrisman

Apparently already fixed!

comment:5 Changed 9 years ago by kcrisman

Here is the diff.

  • check/exam_numeric.cpp

    diff --git a/check/exam_numeric.cpp b/check/exam_numeric.cpp
    index 715acff..d5ae27b 100644 (file)
    a b static unsigned exam_numeric1() 
    6868                    << " erroneously not recognized as complex rational" << endl;
    6969               ++result;
    7070       }
     71       if ( {
     72               clog << test_crat
     73                    << " erroneously recognized as non-negative number" << endl;
     74               ++result;
     75       }
    7277       int i = numeric(1984).to_int();
    7378       if (i-1984) {
  • ginac/numeric.cpp

    diff --git a/ginac/numeric.cpp b/ginac/numeric.cpp
    index f05763d..18725b2 100644 (file)
    a b bool numeric::info(unsigned inf) const 
    700700               case info_flags::negative:
    701701                       return is_negative();
    702702               case info_flags::nonnegative:
    703                        return !is_negative();
     703                       return is_zero() || is_positive();
    704704               case info_flags::posint:
    705705                       return is_pos_integer();
    706706               case info_flags::negint:

comment:6 Changed 9 years ago by kcrisman

Making this change in Pynac does fix it:

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

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

Changed 9 years ago by titusn

comment:8 Changed 9 years ago by burcin

  • Authors set to Alexei Sheplyakov, Titus Nicolae
  • Description modified (diff)
  • Reviewers set to Burcin Erocal

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

comment:9 Changed 9 years ago by burcin

  • Dependencies set to #12950
  • Status changed from new to needs_review

comment:10 Changed 9 years ago by burcin

  • Status changed from needs_review to positive_review

comment:11 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.1.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.