Better representation of exact reals in QQbar

videlec commented 8 years ago

The following example is confusing because of the apparition of the imaginary part

sage: y = QQbar(cos(pi/18))
sage: y.imag() == 0
True
sage: y
0.9848077530122081? + 0.?e-36*I

This ticket proposes to change the representation of known exact reals to be without imaginary part, that is

sage: y
0.9848077530122081?

CC: @Thierry-Dumont @tscrim

Component: number theory

Author: Frédéric Chapoton

Branch/Commit: u/chapoton/21838 @ 33ac1cd

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/21838

jdemeyer commented 8 years ago

comment:1

Possibly related: #20288.

videlec commented 7 years ago

comment:2

The problem is directly related to the not careful enough conversion

sage: CIF(QQbar(cos(pi/18))).imag().is_zero()
False

fchapoton commented 4 years ago

Author: Frédéric Chapoton

fchapoton commented 4 years ago

Commit: 52c05b2

fchapoton commented 4 years ago

Branch: u/chapoton/21838

fchapoton commented 4 years ago

comment:3

Voilà une tentative. Ca va probablement changer pas mal de doctests.

New commits:

52c05b2 change repr of known reals in QQbar

fchapoton commented 4 years ago

comment:5

Looks rather good, not so many doctest failures. And they seem to be improvements

sage -t --long --random-seed=0 src/sage/matrix/matrix2.pyx  # 8 doctests failed
sage -t --long --random-seed=0 src/sage/dynamics/arithmetic_dynamics/projective_ds.py  # 1 doctest failed
sage -t --long --random-seed=0 src/sage/geometry/polyhedron/base.py  # 2 doctests failed
sage -t --long --random-seed=0 src/sage/schemes/generic/algebraic_scheme.py  # 2 doctests failed
sage -t --long --random-seed=0 src/sage/matrix/matrix0.pyx  # 1 doctest failed
sage -t --long --random-seed=0 src/sage/schemes/elliptic_curves/period_lattice.py  # 2 doctests failed
sage -t --long --random-seed=0 src/sage/symbolic/expression_conversions.py  # 1 doctest failed
sage -t --long --random-seed=0 src/sage/geometry/polyhedron/backend_field.py  # 2 doctests failed

Opinions ? Is this the way to go ?

tscrim commented 4 years ago

comment:6

I agree with your assessment and change. It does seem like an improvement.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

8a2c919 fixing doctests after changing repr of QQbar real elements

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 52c05b2 to 8a2c919

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 8a2c919 to eebe445

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

eebe445 fix QQbar doctests in matrix2

tscrim commented 4 years ago

comment:9

Green patchbot => positive review.

tscrim commented 4 years ago

Reviewer: Travis Scrimshaw

fchapoton commented 4 years ago

comment:10

I was wondering (for no special reason) about possible slowing effects of the changes ?

tscrim commented 4 years ago

comment:11

I guess in principle there could be 2 extra is_zero tests that could be done, but I doubt that would cause any significant slowdown. Since the results are usually more accurate types (well, better reflect the element), I think the result should be either net 0 or a speedup due to subsequent computations. However, without any data, I couldn't say.

In short, I doubt there would be any.

fchapoton commented 4 years ago

comment:12

I was more worried about the change

-            return repr(CIF(self._value))
+            return repr(self.interval(CIF))
         else:
-            return repr(RIF(self._value))
+            return repr(self.interval(RIF))

because I am not sure of the exact status of _value

mwageringel commented 4 years ago

comment:13

I was also wondering whether this change means that now exactifications are computed that were not needed before?

             sage: Q*R - A
-            [            0.?e-17 0.?e-17 + 0.?e-17*I 0.?e-16 + 0.?e-16*I 0.?e-16 + 0.?e-16*I]
-            [            0.?e-18 0.?e-17 + 0.?e-17*I 0.?e-16 + 0.?e-16*I 0.?e-16 + 0.?e-16*I]
-            [0.?e-17 + 0.?e-18*I 0.?e-17 + 0.?e-17*I 0.?e-16 + 0.?e-16*I 0.?e-16 + 0.?e-16*I]
-            [0.?e-18 + 0.?e-18*I 0.?e-18 + 0.?e-18*I 0.?e-16 + 0.?e-16*I 0.?e-16 + 0.?e-16*I]
+            [0 0 0 0]
+            [0 0 0 0]
+            [0 0 0 0]
+            [0 0 0 0]

Also, there is one more test failure. Therefore I am setting this back to needs work.

File "src/sage/matrix/matrix2.pyx", line 11149, in sage.matrix.matrix2.Matrix.is_similar
Failed example:
    T
Expected:
    [ 1.00000000000000? + 0.?e-14*I            0.?e-14 + 0.?e-14*I            0.?e-14 + 0.?e-14*I]
    [-0.66666666666667? + 0.?e-15*I 0.166666666666667? + 0.?e-15*I -0.83333333333334? + 0.?e-14*I]
    [ 0.66666666666667? + 0.?e-14*I            0.?e-14 + 0.?e-14*I -0.33333333333333? + 0.?e-14*I]
Got:
    [                   1                    0                    0]
    [-0.6666666666666667?  0.1666666666666667? -0.8333333333333333?]
    [ 0.6666666666666667?                    0 -0.3333333333333334?]

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from eebe445 to d454b3c

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

d454b3c fix again matrix2

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from d454b3c to 33ac1cd

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

33ac1cd one detail in doctest

fchapoton commented 4 years ago

comment:16

Well, one could argue that we now do in every case what we were only doing (these .is_zero tests) for real fields before. Moreover, the changes only touch either the "repr", which is not time critical, or the method "interval", which is used only for inexact conversion to real numbers or complex numbers.

I would therefore advocate that this is ok. Maybe benchmarks would be welcome ?

mezzarobba commented 4 years ago

comment:17

Are you saying that with these changes, converting an element of QQbar to a complex interval can trigger an exactification to test if the element is real? Then I think that's a very bad idea. Testing if an element is real can be extremely costly, while converting to intervals (without caring if the result is real or not) is something you do all the time in some types of computations.

(I'm all in favor of printing elements of QQbar that are already known to be real without an imaginary part if we're not already doing it. And I don't really like the idea of testing if the real part is zero in _repr_, though I can live with it.)

fchapoton commented 4 years ago

comment:18

not in the conversion to RIF or CIF, but in conversion to RR or CC, I would say

mezzarobba commented 4 years ago

comment:19

Regarding RR, I expect the conversion to fail if the imaginary part is not exactly zero.

However, conversion to CC shouldn't ever call exactify() IMO.

mkoeppe commented 3 years ago

comment:20

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

mkoeppe commented 3 years ago

comment:21

Setting a new milestone for this ticket based on a cursory review.

sagemath / sage

Better representation of exact reals in QQbar #21838