Opened 10 years ago

Closed 10 years ago

#10761 closed defect (fixed)

Numerical approximation of an algebraic number raises a ValueError

Reported by: slabbe Owned by: tbd
Priority: major Milestone: sage-4.7
Component: numerical Keywords: numerical_approx, AlgebraicNumber
Cc: Merged in: sage-4.7.alpha5
Authors: Simon Spicer Reviewers: Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Numerical approximation works for complex numbers:

sage: n(1 + I)
1.00000000000000 + 1.00000000000000*I
sage: (1 + I).n()
1.00000000000000 + 1.00000000000000*I

but not for algebraic numbers:

sage: m = matrix(3, [3,1,6,5,2,9,7,3,13])
sage: E = m.eigenvalues()
sage: E
[18.16815365088822?, -0.08407682544410650? - 0.2190261484802906?*I, -0.08407682544410650? + 0.2190261484802906?*I]
sage: map(type, E)
[<class 'sage.rings.qqbar.AlgebraicNumber'>, <class 'sage.rings.qqbar.AlgebraicNumber'>, <class 'sage.rings.qqbar.AlgebraicNumber'>]
sage: map(n, E)
Traceback (most recent call last):
...
ValueError: Cannot coerce algebraic number with non-zero imaginary part to algebraic real

Attachments (1)

trac_10761_AlgebraicNumber_numerical_approx.patch (1.5 KB) - added by spice 10 years ago.
Replaces existing patch.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by spice

  • Authors set to Simon Spicer
  • Component changed from PLEASE CHANGE to numerical
  • Keywords numerical_approx AlgebraicNumber added
  • Status changed from new to needs_review

The patch provides a short and sweet fix to the above problem. The issue arises in sage/misc/functional.py's numerical_approx() function:

if not (is_ComplexNumber(x) or is_ComplexDoubleElement(x)):
            try:
                return sage.rings.real_mpfr.RealField(prec)(x)                                                                              
            except TypeError:
                pass
        return sage.rings.complex_field.ComplexField(prec)(x)

Attempting to call RealField?() on a complex AlgebraicNumber? raises a ValueError? and not a TypeError?, so the exception is not caught. Changing the except clause to catch all errors fixes this:

if not (is_ComplexNumber(x) or is_ComplexDoubleElement(x)):
            try:
                return sage.rings.real_mpfr.RealField(prec)(x)
            # Trac 10761: now catches all errors (instead of just                                                 
            # a TypeError), since calling RealField on AlgebraicNumbers                                           
            # can raise a ValueError                                                                              
            except:
                pass
        return sage.rings.complex_field.ComplexField(prec)(x)

comment:2 Changed 10 years ago by rbeezer

Hi Simon,

This will be very useful, thanks for digging it up. Three comments:

  1. type(E[1]) would be more accurate (and readable) as E[1].parent(), more Sage-like.
  1. I write lots of "naked" except clauses, which I think is a bad practice. Is this a place where could just add ", Value Error" after the TypeError?
  1. Style Points: I think three lines of source comments for this fix is more than you would normally see. With the Trac number in the docstring, and a patch on Trac, you don't need to say so much. With one line here, it'd warn anybody away from messing with it.

Passes all long tests right now.

Rob

comment:3 Changed 10 years ago by spice

Hi Rob

On the issues you've raised:

1) type(E[1]) changed to E[1].parent() in the example as per your suggestion.

2) Regarding the naked except clause, I tried to add a catch of ValueError, but the function was still breaking. I wasn't able to figure out why, which is why I defaulted to catching everything. I figured this was okay, though, since this was just one part of a multistep coercion attempt - and thus errors are to be expected. Thoughts?

3) Inline comment streamlined to a single line.

Simon

comment:4 Changed 10 years ago by rbeezer

  • Reviewers set to Rob Beezer
  • Status changed from needs_review to positive_review

Simon,

Looks real good. If the precise error list does not do the trick, then I think leaving it empty is fine.

Applies and builds, passes tests, docs build and look good. Positive review.

Rob

comment:5 follow-up: Changed 10 years ago by kini

This works for me...

  • sage/misc/functional.py

    diff -r 361a4ad7d52c -r 87dedc409966 sage/misc/functional.py
    a b  
    12601270        if not (is_ComplexNumber(x) or is_ComplexDoubleElement(x)):
    12611271            try:
    12621272                return sage.rings.real_mpfr.RealField(prec)(x)
    1263             except TypeError:
     1273            except (TypeError, ValueError):
    12641274                pass
    12651275        return sage.rings.complex_field.ComplexField(prec)(x)

I think it would be cleaner than just catching all exceptions. Simon, what exactly was wrong with catching ValueErrors??

comment:6 in reply to: ↑ 5 Changed 10 years ago by slabbe

Simon, what exactly was wrong with catching ValueErrors???

I was going to ask the same, but kini beated me.

I also was going to suggest this if the tuple didn't work :

except TypeError:
    pass
except ValueError:
    pass

but I think what kini proposes is best. No?

comment:7 Changed 10 years ago by kini

Did you do something like this?

except TypeError, ValueError:

This will attempt to catch TypeError exceptions and assign them to the variable name ValueError. It's a deprecated behavior from old versions of Python.

Changed 10 years ago by spice

Replaces existing patch.

comment:8 Changed 10 years ago by spice

  • Status changed from positive_review to needs_work

Ah. n00b error - I forgot to add parentheses. It works now.

Simon

comment:9 Changed 10 years ago by spice

  • Status changed from needs_work to needs_review

comment:10 Changed 10 years ago by kini

  • Status changed from needs_review to positive_review

Er, sorry, I of course meant to link to this page. Patch looks good.

comment:11 Changed 10 years ago by slabbe

Thank you Simon for the fix !

Sébastien

comment:12 Changed 10 years ago by jdemeyer

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