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: sage-7.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:

Status badges

Description

On sage-6.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 Vincent Delecroix

Authors: Vincent Delecroix
Branch: u/vdelecroix/20209
Commit: 14e4192f494b8c404d5aee980c3ce13e145b7fed
Status: newneeds_review

New commits:

10f03e0Trac 19036: use vectors instead of tuples in polyomino
14e4192Trac 20209: Fix conversion QQbar -> RIF

comment:2 Changed 7 years ago by git

Commit: 14e4192f494b8c404d5aee980c3ce13e145b7fed53d955c1c7c9fb0cdc8e4b0d8338544e7af629f8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

53d955cTrac 20209: Fix conversion QQbar -> RIF

comment:3 Changed 7 years ago by Travis Scrimshaw

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 try-except block.)

comment:4 Changed 7 years ago by Jeroen Demeyer

Why not replace

val = self.real().interval_diameter(target)

by

val = val.real()

comment:5 Changed 7 years ago by git

Commit: 53d955c1c7c9fb0cdc8e4b0d8338544e7af629f8a0646da0189cd7d177e63128fc6b233aff5f99cc

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a0646daTrac 20209: Fix conversion QQbar -> RIF

comment:6 Changed 7 years ago by Vincent Delecroix

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 Travis Scrimshaw

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 Travis Scrimshaw

One other thing, you're missing a colon before trac here: Check that trac:`20209` is fixed::

comment:9 Changed 7 years ago by git

Commit: a0646da0189cd7d177e63128fc6b233aff5f99cc8a45d080883e0349a132878d4fe063a9455e2e39

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

8a45d08Trac 20209: colon

comment:10 Changed 7 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

Thanks.

comment:11 Changed 7 years ago by Volker Braun

Branch: u/vdelecroix/202098a45d080883e0349a132878d4fe063a9455e2e39
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.