Opened 5 years ago

Last modified 6 weeks ago

#21838 needs_work enhancement

Better representation of exact reals in QQbar

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-9.6
Component: number theory Keywords:
Cc: tdumont, tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: u/chapoton/21838 (Commits, GitHub, GitLab) Commit: 33ac1cd1f3bcf822e98c9da161c06fd2088cbe8b
Dependencies: Stopgaps:

Status badges

Description

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?

Change History (22)

comment:1 Changed 5 years ago by jdemeyer

Possibly related: #20288.

comment:2 Changed 5 years ago by vdelecroix

The problem is directly related to the not careful enough conversion

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

comment:3 Changed 18 months ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/21838
  • Commit set to 52c05b268a5408d79f75e0e265af537c5a6a35f7

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


New commits:

52c05b2change repr of known reals in QQbar

comment:4 Changed 18 months ago by chapoton

  • Milestone changed from sage-7.5 to sage-9.3
  • Status changed from new to needs_review

comment:5 Changed 18 months ago by chapoton

  • Cc tscrim added

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 ?

comment:6 Changed 18 months ago by tscrim

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

comment:7 Changed 18 months ago by git

  • Commit changed from 52c05b268a5408d79f75e0e265af537c5a6a35f7 to 8a2c91906de16f1ab1a531ff9352f828b44deff5

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

8a2c919fixing doctests after changing repr of QQbar real elements

comment:8 Changed 18 months ago by git

  • Commit changed from 8a2c91906de16f1ab1a531ff9352f828b44deff5 to eebe445da8f76e0b29b7a2fdca694cbaa0adb291

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

eebe445fix QQbar doctests in matrix2

comment:9 Changed 18 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Green patchbot => positive review.

comment:10 Changed 18 months ago by chapoton

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

comment:11 Changed 18 months ago by tscrim

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.

comment:12 Changed 18 months ago by chapoton

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

comment:13 Changed 18 months ago by gh-mwageringel

  • Status changed from positive_review to needs_work

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?]

comment:14 Changed 18 months ago by git

  • Commit changed from eebe445da8f76e0b29b7a2fdca694cbaa0adb291 to d454b3cbf546c35fb1fba1b20d8a926e177ce440

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

d454b3cfix again matrix2

comment:15 Changed 18 months ago by git

  • Commit changed from d454b3cbf546c35fb1fba1b20d8a926e177ce440 to 33ac1cd1f3bcf822e98c9da161c06fd2088cbe8b

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

33ac1cdone detail in doctest

comment:16 Changed 18 months ago by chapoton

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 ?

comment:17 Changed 18 months ago by mmezzarobba

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.)

comment:18 Changed 18 months ago by chapoton

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

comment:19 Changed 18 months ago by mmezzarobba

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.

comment:20 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

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

comment:21 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

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

comment:22 Changed 6 weeks ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6
Note: See TracTickets for help on using tickets.