Opened 6 years ago
Closed 6 years ago
#20209 closed defect (fixed)
Failing conversion QQbar > RIF
Reported by:  vdelecroix  Owned by:  

Priority:  blocker  Milestone:  sage7.1 
Component:  number fields  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  8a45d08 (Commits, GitHub, GitLab)  Commit:  8a45d080883e0349a132878d4fe063a9455e2e39 
Dependencies:  Stopgaps: 
Description
On sage6.7.rc1 the following fails
sage: a = QQbar(2).sqrt() sage: RIF(a) Traceback (most recent call last): ... TypeError: unable to convert 1.4142135623730950488? to real interval
Change History (11)
comment:1 Changed 6 years ago by
 Branch set to u/vdelecroix/20209
 Commit set to 14e4192f494b8c404d5aee980c3ce13e145b7fed
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Commit changed from 14e4192f494b8c404d5aee980c3ce13e145b7fed to 53d955c1c7c9fb0cdc8e4b0d8338544e7af629f8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
53d955c  Trac 20209: Fix conversion QQbar > RIF

comment:3 Changed 6 years ago by
I think a better solution would be to do
def interval(self, field): target = RR(1.0) >> field.prec() val = self.interval_diameter(target) from sage.rings.real_mpfi import RealIntervalField_class if isinstance(field, RealIntervalField_class) and self.imag().is_zero(): val = self.real().interval_diameter(target) return field(val)
(I would probably even put the import at the namespace level too.) That way we do not generate the error and catch it, which is a slow operation compared to an isinstance
check:
sage: def foo(): ....: try: ....: raise ValueError ....: except ValueError: ....: pass sage: %timeit foo() The slowest run took 22.94 times longer than the fastest. This could mean that an intermediate result is being cached. 1000000 loops, best of 3: 832 ns per loop sage: def bar(): ....: return isinstance(RIF, type(RIF)) sage: %timeit bar() The slowest run took 27.02 times longer than the fastest. This could mean that an intermediate result is being cached. 10000000 loops, best of 3: 185 ns per loop
(This test is also somewhat unfair to the isinstance
check.) I could see this conversion being used in a tight loop in many computations. (We also loose about 20ns just for having that tryexcept block.)
comment:4 Changed 6 years ago by
Why not replace
val = self.real().interval_diameter(target)
by
val = val.real()
comment:5 Changed 6 years ago by
 Commit changed from 53d955c1c7c9fb0cdc8e4b0d8338544e7af629f8 to a0646da0189cd7d177e63128fc6b233aff5f99cc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a0646da  Trac 20209: Fix conversion QQbar > RIF

comment:6 Changed 6 years ago by
Replying to tscrim:
I think a better solution would be to do [...]
The operation self.imag().is_zero()
is potentially costly ("exactification"). But we can check it on the interval instead. This is what contains the last commit.
(I would probably even put the import at the namespace level too.) That way we do not generate the error and catch it, which is a slow operation compared to an
isinstance
check: ...
Done.
comment:7 Changed 6 years ago by
 Reviewers set to Travis Scrimshaw
LGTM modulo running tests. If the patchbot returns green, then you can set this to a positive review (unless Jeroen, you have any more comments).
comment:8 Changed 6 years ago by
One other thing, you're missing a colon before trac
here: Check that trac:`20209` is fixed::
comment:9 Changed 6 years ago by
 Commit changed from a0646da0189cd7d177e63128fc6b233aff5f99cc to 8a45d080883e0349a132878d4fe063a9455e2e39
Branch pushed to git repo; I updated commit sha1. New commits:
8a45d08  Trac 20209: colon

comment:11 Changed 6 years ago by
 Branch changed from u/vdelecroix/20209 to 8a45d080883e0349a132878d4fe063a9455e2e39
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac 19036: use vectors instead of tuples in polyomino
Trac 20209: Fix conversion QQbar > RIF