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:  sage4.7 
Component:  numerical  Keywords:  numerical_approx, AlgebraicNumber 
Cc:  Merged in:  sage4.7.alpha5  
Authors:  Simon Spicer  Reviewers:  Rob Beezer 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
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 nonzero imaginary part to algebraic real
Attachments (1)
Change History (13)
comment:1 Changed 10 years ago by
 Component changed from PLEASE CHANGE to numerical
 Keywords numerical_approx AlgebraicNumber added
 Status changed from new to needs_review
comment:2 Changed 10 years ago by
Hi Simon,
This will be very useful, thanks for digging it up. Three comments:

type(E[1])
would be more accurate (and readable) asE[1].parent()
, more Sagelike.
 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 theTypeError
?
 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
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
 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 followup: ↓ 6 Changed 10 years ago by
This works for me...

sage/misc/functional.py
diff r 361a4ad7d52c r 87dedc409966 sage/misc/functional.py
a b 1260 1270 if not (is_ComplexNumber(x) or is_ComplexDoubleElement(x)): 1261 1271 try: 1262 1272 return sage.rings.real_mpfr.RealField(prec)(x) 1263 except TypeError:1273 except (TypeError, ValueError): 1264 1274 pass 1265 1275 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
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
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.
comment:8 Changed 10 years ago by
 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
 Status changed from needs_work to needs_review
comment:10 Changed 10 years ago by
 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
Thank you Simon for the fix !
Sébastien
comment:12 Changed 10 years ago by
 Merged in set to sage4.7.alpha5
 Resolution set to fixed
 Status changed from positive_review to closed
The patch provides a short and sweet fix to the above problem. The issue arises in sage/misc/functional.py's numerical_approx() function:
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: