Opened 6 years ago

Closed 6 years ago

#20209 closed defect (fixed)

Failing conversion QQbar -> RIF

Reported by: vdelecroix 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 6 years ago by vdelecroix

  • Authors set to Vincent Delecroix
  • Branch set to u/vdelecroix/20209
  • Commit set to 14e4192f494b8c404d5aee980c3ce13e145b7fed
  • Status changed from new to needs_review

New commits:

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

comment:2 Changed 6 years ago by git

  • Commit changed from 14e4192f494b8c404d5aee980c3ce13e145b7fed to 53d955c1c7c9fb0cdc8e4b0d8338544e7af629f8

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 6 years ago by tscrim

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 6 years ago by jdemeyer

Why not replace

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

by

val = val.real()

comment:5 Changed 6 years ago by git

  • Commit changed from 53d955c1c7c9fb0cdc8e4b0d8338544e7af629f8 to a0646da0189cd7d177e63128fc6b233aff5f99cc

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 6 years ago by vdelecroix

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 tscrim

  • 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 tscrim

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

comment:9 Changed 6 years ago by git

  • Commit changed from a0646da0189cd7d177e63128fc6b233aff5f99cc to 8a45d080883e0349a132878d4fe063a9455e2e39

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

8a45d08Trac 20209: colon

comment:10 Changed 6 years ago by tscrim

  • Status changed from needs_review to positive_review

Thanks.

comment:11 Changed 6 years ago by vbraun

  • Branch changed from u/vdelecroix/20209 to 8a45d080883e0349a132878d4fe063a9455e2e39
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.