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: |
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)
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 Sage-like.
- 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 follow-up: ↓ 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 sage-4.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: