Opened 7 years ago
Closed 7 years ago
#20209 closed defect (fixed)
Failing conversion QQbar > RIF
Reported by:  Vincent Delecroix  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 7 years ago by
Authors:  → Vincent Delecroix 

Branch:  → u/vdelecroix/20209 
Commit:  → 14e4192f494b8c404d5aee980c3ce13e145b7fed 
Status:  new → needs_review 
comment:2 Changed 7 years ago by
Commit:  14e4192f494b8c404d5aee980c3ce13e145b7fed → 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 7 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 7 years ago by
Why not replace
val = self.real().interval_diameter(target)
by
val = val.real()
comment:5 Changed 7 years ago by
Commit:  53d955c1c7c9fb0cdc8e4b0d8338544e7af629f8 → 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 7 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 7 years ago by
Reviewers:  → 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 7 years ago by
One other thing, you're missing a colon before trac
here: Check that trac:`20209` is fixed::
comment:9 Changed 7 years ago by
Commit:  a0646da0189cd7d177e63128fc6b233aff5f99cc → 8a45d080883e0349a132878d4fe063a9455e2e39 

Branch pushed to git repo; I updated commit sha1. New commits:
8a45d08  Trac 20209: colon

comment:11 Changed 7 years ago by
Branch:  u/vdelecroix/20209 → 8a45d080883e0349a132878d4fe063a9455e2e39 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Trac 19036: use vectors instead of tuples in polyomino
Trac 20209: Fix conversion QQbar > RIF