Ticket #10761(closed defect: fixed)

Opened 2 years ago

Numerical approximation of an algebraic number raises a ValueError

Reported by: Owned by: slabbe tbd major sage-4.7 numerical numerical_approx, AlgebraicNumber N/A Rob Beezer Simon Spicer sage-4.7.alpha5

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
```

Change History

comment:1 Changed 2 years ago by spice

• Status changed from new to needs_review
• Component changed from PLEASE CHANGE to numerical
• Authors set to Simon Spicer

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 2 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 2 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 2 years ago by rbeezer

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

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 2 years ago by kini

This works for me...

• sage/misc/functional.py

`diff -r 361a4ad7d52c -r 87dedc409966 sage/misc/functional.py`
 a if not (is_ComplexNumber(x) or is_ComplexDoubleElement(x)): try: return sage.rings.real_mpfr.RealField(prec)(x) except TypeError: except (TypeError, ValueError): pass 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 2 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 2 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 2 years ago by spice

Replaces existing patch.

comment:8 Changed 2 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 2 years ago by spice

• Status changed from needs_work to needs_review

comment:10 Changed 2 years ago by kini

• Status changed from needs_review to positive_review