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:  sage9.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: 
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.?e36*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
comment:2 Changed 5 years ago by
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
 Branch set to u/chapoton/21838
 Commit set to 52c05b268a5408d79f75e0e265af537c5a6a35f7
Voilà une tentative. Ca va probablement changer pas mal de doctests.
New commits:
52c05b2  change repr of known reals in QQbar

comment:4 Changed 18 months ago by
 Milestone changed from sage7.5 to sage9.3
 Status changed from new to needs_review
comment:5 Changed 18 months ago by
 Cc tscrim added
Looks rather good, not so many doctest failures. And they seem to be improvements
sage t long randomseed=0 src/sage/matrix/matrix2.pyx # 8 doctests failed sage t long randomseed=0 src/sage/dynamics/arithmetic_dynamics/projective_ds.py # 1 doctest failed sage t long randomseed=0 src/sage/geometry/polyhedron/base.py # 2 doctests failed sage t long randomseed=0 src/sage/schemes/generic/algebraic_scheme.py # 2 doctests failed sage t long randomseed=0 src/sage/matrix/matrix0.pyx # 1 doctest failed sage t long randomseed=0 src/sage/schemes/elliptic_curves/period_lattice.py # 2 doctests failed sage t long randomseed=0 src/sage/symbolic/expression_conversions.py # 1 doctest failed sage t long randomseed=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
I agree with your assessment and change. It does seem like an improvement.
comment:7 Changed 18 months ago by
 Commit changed from 52c05b268a5408d79f75e0e265af537c5a6a35f7 to 8a2c91906de16f1ab1a531ff9352f828b44deff5
Branch pushed to git repo; I updated commit sha1. New commits:
8a2c919  fixing doctests after changing repr of QQbar real elements

comment:8 Changed 18 months ago by
 Commit changed from 8a2c91906de16f1ab1a531ff9352f828b44deff5 to eebe445da8f76e0b29b7a2fdca694cbaa0adb291
Branch pushed to git repo; I updated commit sha1. New commits:
eebe445  fix QQbar doctests in matrix2

comment:9 Changed 18 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Green patchbot => positive review.
comment:10 Changed 18 months ago by
I was wondering (for no special reason) about possible slowing effects of the changes ?
comment:11 Changed 18 months ago by
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
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
 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.?e17 0.?e17 + 0.?e17*I 0.?e16 + 0.?e16*I 0.?e16 + 0.?e16*I]  [ 0.?e18 0.?e17 + 0.?e17*I 0.?e16 + 0.?e16*I 0.?e16 + 0.?e16*I]  [0.?e17 + 0.?e18*I 0.?e17 + 0.?e17*I 0.?e16 + 0.?e16*I 0.?e16 + 0.?e16*I]  [0.?e18 + 0.?e18*I 0.?e18 + 0.?e18*I 0.?e16 + 0.?e16*I 0.?e16 + 0.?e16*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.?e14*I 0.?e14 + 0.?e14*I 0.?e14 + 0.?e14*I] [0.66666666666667? + 0.?e15*I 0.166666666666667? + 0.?e15*I 0.83333333333334? + 0.?e14*I] [ 0.66666666666667? + 0.?e14*I 0.?e14 + 0.?e14*I 0.33333333333333? + 0.?e14*I] Got: [ 1 0 0] [0.6666666666666667? 0.1666666666666667? 0.8333333333333333?] [ 0.6666666666666667? 0 0.3333333333333334?]
comment:14 Changed 18 months ago by
 Commit changed from eebe445da8f76e0b29b7a2fdca694cbaa0adb291 to d454b3cbf546c35fb1fba1b20d8a926e177ce440
Branch pushed to git repo; I updated commit sha1. New commits:
d454b3c  fix again matrix2

comment:15 Changed 18 months ago by
 Commit changed from d454b3cbf546c35fb1fba1b20d8a926e177ce440 to 33ac1cd1f3bcf822e98c9da161c06fd2088cbe8b
Branch pushed to git repo; I updated commit sha1. New commits:
33ac1cd  one detail in doctest

comment:16 Changed 18 months ago by
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
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
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
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
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:21 Changed 6 months ago by
 Milestone changed from sage9.4 to sage9.5
Setting a new milestone for this ticket based on a cursory review.
comment:22 Changed 6 weeks ago by
 Milestone changed from sage9.5 to sage9.6
Possibly related: #20288.