Opened 8 years ago

Closed 8 years ago

# weird conversion from QQ to RDF

Reported by: Owned by: zimmerma AlexGhitza major sage-5.11 basic arithmetic robertwb sage-5.11.beta0 Paul Zimmermann, Jeroen Demeyer Paul Zimmermann N/A #14335, #14336

the following is weird:

```sage: RDF(1/10)-RDF(1)/RDF(10)
-1.38777878078e-17
```

One would expect that the conversion from 1/10 to RDF is done as follows:

• first convert 1 to RDF, which is exact
• then convert 10 to RDF, which is exact
• then divide RDF(1) by RDF(10)

For RR we get as a comparison:

```sage: RR(1/10)-RR(1)/RR(10)
0.000000000000000
```

More examples:

```sage: for p in [1..10]:
....:     for q in [1..10]:
....:         if RDF(p/q) <> RDF(p)/RDF(q):
....:             print p, q
....:
1 5
1 10
2 5
2 10
4 5
4 10
5 3
5 6
5 7
5 9
7 3
7 6
7 9
8 5
8 10
9 5
9 7
9 10
10 3
10 6
10 7
10 9
```

and for RR:

```sage: for p in [1..10]:
....:     for q in [1..10]:
....:         if RR(p/q) <> RR(p)/RR(q):
....:             print p, q
....:
sage:
```

### comment:1 Changed 8 years ago by mhansen

Tracing things back, the conversion eventually gets done by:

```mpq_get_d(self.value)
```

since `Rational` is just a wrapper around an `mpq`. This is where the weirdness seems to be.

### comment:2 Changed 8 years ago by zimmerma

• Status changed from new to needs_review

ok, then the explanation is that `mpq_get_d` (according to the GMP manual) rounds towards zero.

One could call `mpz_get_d(numerator)/mpz_get(denominator)` but then for large numerator and denominator one would get three roundings, instead of only one when calling `mpq_get_d`.

I propose to close this ticket.

Paul

### comment:3 follow-up: ↓ 8 Changed 8 years ago by kcrisman

So it's okay that

```sage: RDF(1/10)*10 == RDF(1)
False
sage: RDF(1/10)*10 - RDF(1)
-1.11022302463e-16

sage: RR(1/10)*10 == RR(1)
True
sage: sage: RR(1/10)*10 - RR(1)
0.000000000000000
```

as Thierry pointed out here? Just asking. Or is it RR that is behaving sub-optimally in this case? I assume that three roundings is ordinarily worse than one - my apologies for asking dumb questions.

### comment:4 Changed 8 years ago by tscrim

• Component changed from basic arithmetic to documentation
• Milestone changed from sage-5.10 to sage-5.9
• Status changed from needs_review to needs_info

`RR` is not the same as `RDF` in regards to how rounding is done, so I would say yes, it is okay. I'd suspect if you change the rounding mode, you should get a similar result, but rounding and computations in `RDF` are tied to your hardware (up to a certain precision) since I believe they are done in your native machine `double` type. In particular, the ALU (arithmetic logic unit) of whatever CPU you're using. I should state for the record I'm not 100% certain of this.

The reason why `RR` works uniformly is as it was noted in the ask sage, it is emulating a CPU in a program (one can think of it as constant hardware).

However instead of closing this ticket, I propose someone should implement a tutorial and/or improve the documentation and/or sage basics. I can do it if needbe.

### comment:5 Changed 8 years ago by tscrim

• Milestone changed from sage-5.9 to sage-5.10

Whoops, didn't mean to change the milestone.

### comment:6 Changed 8 years ago by tmonteil

As Paul explained, the rounding is the same for RR and RDF: RDF rounds its computations to the nearest.

Actually, the problem is somewhere else: the conversion from QQ to RDF rounds towards zero, which is not consistent with the general behaviour of RDF.

IMHO, this conversion from QQ to RDF should be fixed, if possible.

### comment:7 Changed 8 years ago by tscrim

• Component changed from documentation to basic arithmetic

Ah I see. I misinterpreted Paul's explanation. I also agree that this should be fixed for consistency:

```sage: RDF(RDF(1)/10) - RDF(1)/RDF(10)
0.0
sage: RDF(RR(1)/10) - RDF(1)/RDF(10)
0.0
sage: RDF(1/RDF(10)) - RDF(1)/RDF(10)
0.0
sage: RDF(1/float(10)) - RDF(1)/RDF(10)
0.0
```
Version 0, edited 8 years ago by tscrim (next)

### comment:8 in reply to: ↑ 3 Changed 8 years ago by zimmerma

Karl-Dieter,

So it's okay that

```sage: RDF(1/10)*10 == RDF(1)
False
sage: RDF(1/10)*10 - RDF(1)
-1.11022302463e-16

sage: RR(1/10)*10 == RR(1)
True
sage: sage: RR(1/10)*10 - RR(1)
0.000000000000000
```

as Thierry pointed out here? Just asking. Or is it RR that is behaving sub-optimally in this case? I assume that three roundings is ordinarily worse than one - my apologies for asking dumb questions.

what is annoying is that RDF and RR give different results. The fact that RR gives `0.0` in that case is not that important, for example:

```sage: RR(1/49)*49-RR(1)
-1.11022302462516e-16
```

Paul

### comment:9 Changed 8 years ago by zimmerma

IMHO, this conversion from QQ to RDF should be fixed, if possible.

it is possible: first call the MPFR function `mpfr_set_q` on the rational fraction (which is what `RR(p/q)` does) then convert back to `double` with `mpfr_get_d`, but this might be less efficient than the current code, since it would convert from QQ to RR, then from RR to RDF.

Converting separately the numerator and the denominator to RDF, then dividing in RDF does not work, due to double rounding. Consider for example the fraction `p/q` where `p=2403806706169061971` and `q=983883817941434958`. If you first round p and q to RDF (say with `mpz_get_d`), then you get `2403806706169061888/983883817941435008`, which is rounded to nearest to `5501555564164225/2251799813685248`, whereas the direct rounding of p/q to nearest gives `2750777782082113/1125899906842624`:

```sage: p=2403806706169061971; q=983883817941434958; pp=RR(p); qq=RR(q)
sage: (pp/qq).exact_rational()
5501555564164225/2251799813685248
sage: RR(p/q).exact_rational()
2750777782082113/1125899906842624
```

Paul

### comment:10 Changed 8 years ago by robertwb

RDF is for when you want your computations to be fast, at the expense of a little bit of accuracy/less control of rounding/platform dependence. As such, I think different behavior than RR is completely acceptable. It would be nice if `mpq_get_d` rounded towards nearest, but until someone implements that in a manner that's at least comparable in speed to `mpq_get_d` I think the current behavior is more in line with the philosophy of RDF than making things slow to get the last bit correct.

### comment:11 Changed 8 years ago by zimmerma

Robert, would the following be ok in what concerns speed?

Let p/q the fraction to be converted to RDF, I assume p, q > 0.

1) multiply p or q by a power of 2 into pp and qq so that both have the same number of bits

(using mpz_sizeinbase and mpz_mul_2exp from GMP)

2) if pp < qq, multiply pp by 254, otherwise multiply pp by 253, using mpz_mul_2exp

3) now 253 <= pp/qq < 254, compute (using mpz_tdiv_qr) the quotient r = trunc(pp/qq) and the

remainder s; we know that r has exactly 54 bits

4) if r is even, return r*2k where k is the normalizing constant (exact since r is exact on 53 bits)

5) if s is not zero, return (r+1)*2k [r+1 is even, and exact on 53 bits, even in the

case where r+1 = 254]

5a) if s=0, return (r-1)*2k if the bit of weight 1 of r is 0, otherwise (r+1)*2k

I guess mpq_get_d does basically steps 1-4, thus it should be comparable in speed.

Paul

### comment:12 Changed 8 years ago by zimmerma

here is some tentative code which converts from QQ to RDF with rounding to nearest. In the usual case where both the numerator and the denominator are less than 253 in absolute value, it is about twice as fast as `mpq_get_d`.

```double
mpq_get_d_nearest (mpq_t q)
{
mpz_ptr a = mpq_numref (q);
mpz_ptr b = mpq_denref (q);
size_t sa = mpz_sizeinbase (a, 2);
size_t sb = mpz_sizeinbase (b, 2);
size_t na, nb;
mpz_t aa, bb;
double d;

/* easy case: |a|, |b| < 2^53, no overflow nor underflow can occur */
if (sa <= 53 && sb <= 53)
return mpz_get_d (a) / mpz_get_d (b);

/* same if a = m*2^e with m representable on 53 bits, idem for b, but beware
that both a and b do not give an overflow */
na = sa - mpz_scan1 (a, 0);
nb = sb - mpz_scan1 (b, 0);
if (sa <= 1024 && na <= 53 && sb <= 1024 && nb <= 53)
return mpz_get_d (a) / mpz_get_d (b);

/* hard case */
mpz_init (aa);
mpz_init (bb);

if (sa >= sb)
{
mpz_set (aa, a);
mpz_mul_2exp (bb, b, sa - sb);
}
else
{
mpz_mul_2exp (aa, a, sb - sa);
mpz_set (bb, b);
}

/* q = aa/bb*2^(sa-sb) */

if (mpz_cmpabs (aa, bb) >= 0)
{
mpz_mul_2exp (bb, bb, 1);
sa ++;
}

mpz_mul_2exp (aa, aa, 54);
sb += 54;

mpz_tdiv_qr (aa, bb, aa, bb);

/* the quotient aa should have exactly 54 bits */

if (mpz_tstbit (aa, 0) == 0)
{
}
else if (mpz_cmp_ui (bb, 0) != 0)
{
if (mpz_sgn (aa) > 0)
else
mpz_sub_ui (aa, aa, 1);
}
else /* mid case: round to even */
{
if (mpz_tstbit (aa, 1) == 0)
{
if (mpz_sgn (aa) > 0)
mpz_sub_ui (aa, aa, 1);
else
}
else
{
if (mpz_sgn (aa) > 0)
else
mpz_sub_ui (aa, aa, 1);
}
}

mpz_clear (aa);
mpz_clear (bb);
d = mpz_get_d (aa); /* exact */
return ldexp (d, (long) sa - (long) sb);
}
```

However I don't know where to incorporate that code in Sage. Could someone help?

Paul

### comment:13 Changed 8 years ago by kcrisman

Volker points to here in sage/rings/rational.pyx. I imagine that one could just replace that `return mpq_get_d(self.value)` with your code, or maybe make an auxiliary function since I suspect yours is pure C or C++ and this is a Cython file.

### comment:14 Changed 8 years ago by tscrim

That looks like pure C code. Isn't there an mpz/mpq package with the underlying C(++) code, and wouldn't this go in there...?

### comment:15 Changed 8 years ago by zimmerma

yes this is pure C code.

Paul

### comment:16 Changed 8 years ago by zimmerma

note that we have currently:

```sage: RDF(2^(-1075))
0.0
sage: RDF(-2^(-1075))
0.0
```

The last result should be `-0.0`, as in `-RDF(2^(-1075))`. I guess my code above fixes that (not tested).

Paul

### comment:17 Changed 8 years ago by jdemeyer

Is it really worth to do

```  /* same if a = m*2^e with m representable on 53 bits, idem for b, but beware
that both a and b do not give an overflow */
na = sa - mpz_scan1 (a, 0);
nb = sb - mpz_scan1 (b, 0);
if (sa <= 1024 && na <= 53 && sb <= 1024 && nb <= 53)
return mpz_get_d (a) / mpz_get_d (b);
```

I think that this case is pretty rare, so I wouldn't special-case it.

### comment:18 Changed 8 years ago by zimmerma

Jeroen,

I think that this case is pretty rare, so I wouldn't special-case it.

this was my first idea. Then I realized I was often using say `RDF(1/2^100)`. Yes we can remove that special case, but it would be interesting to see how often it is used in the whole Sage test suite.

Paul

### comment:19 Changed 8 years ago by jdemeyer

There is also this obvious bug:

```mpz_clear (aa);
[...]
mpz_get_d (aa);
```

### comment:20 Changed 8 years ago by zimmerma

There is also this obvious bug:...

sorry, please replace the last lines by:

```  d = mpz_get_d (aa); /* exact */
mpz_clear (aa);
mpz_clear (bb);
return ldexp (d, (long) sa - (long) sb);
```

I also realize in the "hard case", there is a double-rounding issue for subnormals. This could be fixed if needed.

Paul

### comment:21 Changed 8 years ago by jdemeyer

• Authors set to Paul ZImmermann, Jeroen Demeyer

I'm working on a Sage patch based on Paul's code.

### comment:22 Changed 8 years ago by zimmerma

• Authors changed from Paul ZImmermann, Jeroen Demeyer to Paul Zimmermann, Jeroen Demeyer

I'm working on a Sage patch based on Paul's code.

great! Please ask if you need some help. My code should deal correctly with underflow or overflow (through the call to ldexp).

Paul

### comment:23 Changed 8 years ago by jdemeyer

• Status changed from needs_info to needs_review

### comment:24 follow-ups: ↓ 28 ↓ 35 Changed 8 years ago by zimmerma

Jeroen,

as an author I'm not supposed to review that ticket, but I have a few questions:

• why the `or A.condition()` in `matrix/matrix_double_dense.pyx`?
• in `matrix/matrix_mod2e_dense.pyx` I wonder why you had to change only one value, and not all from lines 788 to 811
• in `plot/colors.py` the new value is closer to 2/7 than the old one (as expected):
```sage: (RR(rainbow(7, 'rgbtuple')[5][0]).exact_rational()-2/7)*1.0
-1.26882631385732e-16
```
• idem in `rings/contfrac.py`:
```sage: (RR(float(a)).exact_rational()+17/389)*1.0
-1.60539961787057e-18
```
• the test `float(1/10) * 10 == float(1)` is misleading: it might make think that this is now true for any value of q instead of 10, which is wrong (consider q=49).
• efficiency is not that bad in the "easy" case (was 4.8 ms before):
```sage: l=[a/b for a in [1..99] for b in [1..99]]
sage: %timeit c = map(RDF,l)
100 loops, best of 3: 2.97 ms per loop
```
• we have lost a little in the "general" case (was 5.04 ms before):
```sage: l=[(2^53+a)/(3^53+b) for a in [1..99] for b in [1..99]]
sage: %timeit c = map(RDF,l)
100 loops, best of 3: 7.38 ms per loop
```
• `q0 = aa/bb / 2^shift` should be `q0 = a/b / 2^shift`
• `|d| <= 2^-1075` should be `|d| < 2^-1075`
• `shift >= 972` can be changed to `shift >= 971`, since for 971 we get `|d| >= 2^1024` and `2^1024` gives infinity
• maybe we can avoid the copy done by `mpz_init_set` and point directly to the corresponding numerator or denominator?
• I would add an example showing that the conversion now agrees with RR:
```sage: all([RDF(a/b) == RR(a/b) for a in [1..99] for b in [1..99]])
True
```

I see you have dealt with the subnormal case too: great!

Paul

### comment:25 Changed 8 years ago by vbraun

Shouldn't we push that upstream to MPIR/GMP? From a cursory Google search it seems that we are not the first ones to trip over this. Did anybody contact upstream for their opinion?

### comment:26 Changed 8 years ago by zimmerma

Volker, you are right. I will ask the GMP developers.

Paul

### comment:28 in reply to: ↑ 24 ; follow-up: ↓ 29 Changed 8 years ago by jdemeyer

Jeroen,

as an author I'm not supposed to review that ticket

I don't think that's true. I read your patch, understood it (so that's a review of your code) and made some modifications. So I think it is perfectly fine if you review the patch.

• why the `or A.condition()` in `matrix/matrix_double_dense.pyx`?

That's a cool doctest trick I learned recently. If the condition fails, it will output the exact value of `A.condition()` instead of simply returning `False`.

• in `matrix/matrix_mod2e_dense.pyx` I wonder why you had to change only one value, and not all from lines 788 to 811

Well, I assume the others are cases where the old and new mpq->double conversion code agree.

• the test `float(1/10) * 10 == float(1)` is misleading: it might make think that this is now true for any value of q instead of 10, which is wrong (consider q=49).

OK, true. I will remove that test.

• maybe we can avoid the copy done by `mpz_init_set` and point directly to the corresponding numerator or denominator?

I have no idea how to do that in Cython. Besides, we do change the value of `aa` and `bb`, so we need an init anyway.

• I would add an example showing that the conversion now agrees with RR:
```sage: all([RDF(a/b) == RR(a/b) for a in [1..99] for b in [1..99]])
True
```

OK, good idea. I'll also test negative numerators.

Last edited 8 years ago by jdemeyer (previous) (diff)

### comment:29 in reply to: ↑ 28 ; follow-up: ↓ 30 Changed 8 years ago by zimmerma

Jeroen,

as an author I'm not supposed to review that ticket

I don't think that's true. I read your patch, understood it (so that's a review of your code) and made some modifications. So I think it is perfectly fine if you review the patch.

I'm fine in giving comments, but I would prefer someone else gives "positive review".

• why the `or A.condition()` in `matrix/matrix_double_dense.pyx`?

That's a cool doctest trick I learned recently. If the condition fails, it will output the exact value of `A.condition()` instead of simply returning `False`.

good. I learned something too!

• in `matrix/matrix_mod2e_dense.pyx` I wonder why you had to change only one value, and not all from lines 788 to 811

Well, I assume the others are cases where the old and new mpq->double conversion code agree.

when I did those tests by hand in a vanilla session, I got different values, thus I guess they depend on the seed value, and the previous tests, which is not very good.

• maybe we can avoid the copy done by `mpz_init_set` and point directly to the corresponding numerator or denominator?

I have no idea how to do that in Cython. Besides, we do change the value of `aa` and `bb`, so we need an init anyway.

you could have different variables to store the quotient and remainder, and split the code to perform the division directly on a or b.

Paul

### comment:30 in reply to: ↑ 29 Changed 8 years ago by jdemeyer

you could have different variables to store the quotient and remainder, and split the code to perform the division directly on a or b.

Yes, that's what I did in the new patch.

### comment:31 follow-up: ↓ 33 Changed 8 years ago by zimmerma

with the new patch (I did not check with the old one) I get warnings:

```warning: sage/rings/../ext/gmp.pxi:65:22: local variable 'p1' referenced before assignment
warning: sage/rings/../ext/gmp.pxi:66:15: local variable 'p2' referenced before assignment
warning: sage/rings/../ext/gmp.pxi:87:22: local variable 'p1' referenced before assignment
warning: sage/rings/../ext/gmp.pxi:173:14: local variable 'g' referenced before assignment
warning: sage/rings/../ext/gmp.pxi:173:27: local variable 's' referenced before assignment
warning: sage/rings/../ext/gmp.pxi:173:40: local variable 't' referenced before assignment
warning: sage/rings/../ext/gmp.pxi:173:54: local variable 'mn' referenced before assignment
warning: sage/rings/rational.pyx:1433:25: local variable 'prod' referenced before assignment
warning: sage/rings/rational.pyx:1444:29: local variable 'prod' referenced before assignment
warning: sage/rings/rational.pyx:1454:33: local variable 'prod' referenced before assignment
warning: sage/rings/rational.pyx:1465:33: local variable 'prod' referenced before assignment
warning: sage/rings/rational.pyx:1467:29: local variable 'prod' referenced before assignment
warning: sage/rings/rational.pyx:1768:20: local variable 'tmp' referenced before assignment
warning: sage/rings/rational.pyx:2427:20: local variable 'num' referenced before assignment
warning: sage/rings/rational.pyx:2428:20: local variable 'den' referenced before assignment
warning: sage/rings/rational.pyx:2763:22: local variable 'x' referenced before assignment
warning: sage/rings/rational.pyx:3576:14: local variable 'q' referenced before assignment
warning: sage/rings/rational.pyx:3577:14: local variable 'r' referenced before assignment
```

Paul

### comment:32 Changed 8 years ago by zimmerma

performance is not better in the general case:

```sage: l=[(2^53+a)/(3^53+b) for a in [1..99] for b in [1..99]]
sage: %timeit c = map(RDF,l)
10 loops, best of 3: 8.66 ms per loop
sage: l=[(2^53+a)/(3^53+b) for a in [1..99] for b in [1..99]]
sage: %timeit c = map(RDF,l)
100 loops, best of 3: 9.38 ms per loop
sage: l=[(2^53+a)/(3^53+b) for a in [1..99] for b in [1..99]]
sage: %timeit c = map(RDF,l)
100 loops, best of 3: 10.8 ms per loop
```

Paul

### comment:33 in reply to: ↑ 31 Changed 8 years ago by jdemeyer

with the new patch (I did not check with the old one) I get warnings

Sure, we get these warnings all over the place. One could say this is a Cython bug.

### comment:34 Changed 8 years ago by jdemeyer

• Description modified (diff)

I added a second version which uses `uint64_t` in the second part, which is hopefully slightly faster.

### comment:35 in reply to: ↑ 24 Changed 8 years ago by jdemeyer

• why the `or A.condition()` in `matrix/matrix_double_dense.pyx`?

To elaborate on this: `X or Y` returns `X` if `bool(X)` is true and returns `Y` if `bool(X)` is false. The opposite for `X and Y`.

### comment:36 Changed 8 years ago by zimmerma

the new version is slightly better in the general case:

```sage: l=[(2^53+a)/(3^53+b) for a in [1..99] for b in [1..99]]
sage: %timeit c = map(RDF,l)
100 loops, best of 3: 8.15 ms per loop
sage: %timeit c = map(RDF,l)
100 loops, best of 3: 8.07 ms per loop
sage: %timeit c = map(RDF,l)
100 loops, best of 3: 7.97 ms per loop
```

Paul

### comment:37 Changed 8 years ago by zimmerma

I believe it will be difficult to do better, unless we avoid mpz and use mpn instead. Assuming all tests still work, I am fine with the new code.

Could anybody out there have a final look and give a positive review?

Paul

### comment:38 Changed 8 years ago by zimmerma

btw, a small typo in the patch: occured should be occurred...

Paul

### comment:39 follow-up: ↓ 41 Changed 8 years ago by tmonteil

I am definitely not able to review this ticket, but i would suggest to add the initial bug in the doctest to detect regression (not only comparing RDF with RR since they could change their behaviour simultaneously).

```sage: RDF(1/10)*10 == RDF(1)
True
```

and even

```sage: all([RDF(p/q) == RDF(p)/RDF(q) for p in [-100..100] for q in [1..100]])
True
```

### comment:40 Changed 8 years ago by zimmerma

Thierry, I asked Jeroen to remove from the doctest `RDF(1/10)*10 == RDF(1)` since this is wrong if you replace 10 by say 49, so this was not a bug, but a feature of rounding.

The second test you propose is good (however there is a very small probability that RDF and RR change simultaneously, but the current test doesn't exercise negative p).

Paul

### comment:41 in reply to: ↑ 39 Changed 8 years ago by jdemeyer

```sage: all([RDF(p/q) == RDF(p)/RDF(q) for p in [-100..100] for q in [1..100]])
True
```

I would say this test is also misleading since `RDF(p/q)` and `RDF(p)/RDF(q)` don't have to be the same for large value of `p` and `q`.

### comment:42 Changed 8 years ago by zimmerma

I would say this test is also misleading since RDF(p/q) and RDF(p)/RDF(q) don't have to be the same...

yes, but for p, q integers in [-100,100] the conversion to RDF is exact.

Paul

### Changed 8 years ago by tmonteil

Tested on sage 5.9.beta5, depends on #14448

### comment:43 Changed 8 years ago by tmonteil

• Dependencies set to #14448

Perhaps the following test has a better semantics, and still represents the initial bug.

```for p in [-100..100]:
for q in [1..100]:
r = p/q
s, m, e = RDF(r).sign_mantissa_exponent()
if not abs(s*m*2^(e) - r) <= 2^(e-1):
print 'Bug #14416 reappeared with rational', r
```

Unfortunately, this lets me found a bug in the `.sign_mantissa_exponent()` (which currently gives negative mantissa to negative numbers), see #14448.

### comment:44 Changed 8 years ago by jdemeyer

• Dependencies #14448 deleted

OK, I added your doctest in a simplified way such that it doesn't depend on #14448.

### comment:45 follow-up: ↓ 46 Changed 8 years ago by zimmerma

Jeroen, what is status of this ticket for the patchbot? Last time I looked, some tests were failing.

Paul

### comment:46 in reply to: ↑ 45 Changed 8 years ago by jdemeyer

Jeroen, what is status of this ticket for the patchbot? Last time I looked, some tests were failing.

I fixed some more doctest failures in `sage/matrix/matrix_double_dense.pyx`.

### comment:47 Changed 8 years ago by zimmerma

I fixed some more doctest failures in sage/matrix/matrix_double_dense.pyx.

all tests do pass now?

Paul

### comment:48 Changed 8 years ago by jdemeyer

Some tests might be machine-specific, but at least on most machines, all tests pass indeed.

### comment:49 Changed 8 years ago by jdemeyer

• Dependencies set to #14335, #14336
• Status changed from needs_review to needs_work

### comment:50 Changed 8 years ago by jdemeyer

• Status changed from needs_work to needs_review

Rebased for doctest failures with #14336.

Paul: are you sure you don't want to review the patch? If the both of us look at the patch, that should be sufficient, no?

### comment:51 Changed 8 years ago by zimmerma

I will try to review it next week. But if someone beats me, no problem!

Paul

### comment:52 follow-up: ↓ 53 Changed 8 years ago by zimmerma

• Reviewers set to Paul Zimmermann
• Status changed from needs_review to needs_work

• in the following test, you can replace 20 by 13. Also, you could add `all([RDF(q) == RR(q) for q in Q])` which would exercise the code for large numerators and denominators.
```sage: Q = continued_fraction(pi, bits=3000).convergents()[20:]
sage: RDFpi = RDF(pi)
sage: all([RDF(q) == RDFpi for q  in Q])
```
• is the `except?` value really needed in `mpq_get_d_nearest`?
• please replace `round-to-even` with `round-to-nearest-even`. Round to even and round to odd also exist.
• please replace `occured` by `occurred` (several places, already mentioned)

Apart from that (and if tests still pass) I'm fine with the patch.

Paul

### comment:53 in reply to: ↑ 52 Changed 8 years ago by jdemeyer

• in the following test, you can replace 20 by 13.

Sure, but I wanted some safety margin.

Also, you could add `all([RDF(q) == RR(q) for q in Q])`

OK, but then for all covergents (before throwing away the first 20).

• is the `except?` value really needed in `mpq_get_d_nearest`?

Yes, because `sig_on()` can throw exceptions.

### comment:54 follow-up: ↓ 55 Changed 8 years ago by zimmerma

I could not apply the patch to 5.9 (after applying successfully the two dependencies):

```----------------------------------------------------------------------
| Sage Version 5.9, Release Date: 2013-04-30                         |
| Type "notebook()" for the browser-based notebook interface.        |
| Type "help()" for help.                                            |
----------------------------------------------------------------------
sage: hg_sage.import_patch("/tmp/14416_QQ_to_RDF_v2.patch")
cd "/localdisk/tmp/sage-5.9/devel/sage" && sage --hg import   "/tmp/14416_QQ_to_RDF_v2.patch"
applying /tmp/14416_QQ_to_RDF_v2.patch
patching file sage/matrix/matrix_double_dense.pyx
Hunk #1 FAILED at 1018
Hunk #2 FAILED at 1947
Hunk #3 FAILED at 2087
3 out of 3 hunks FAILED -- saving rejects to file sage/matrix/matrix_double_dense.pyx.rej
patching file sage/plot/colors.py
Hunk #1 FAILED at 1301
1 out of 1 hunks FAILED -- saving rejects to file sage/plot/colors.py.rej
patching file sage/rings/contfrac.py
Hunk #1 FAILED at 632
1 out of 1 hunks FAILED -- saving rejects to file sage/rings/contfrac.py.rej
patching file sage/rings/rational.pyx
Hunk #1 FAILED at 73
Hunk #2 FAILED at 1999
Hunk #3 succeeded at 3660 with fuzz 2 (offset 228 lines).
2 out of 3 hunks FAILED -- saving rejects to file sage/rings/rational.pyx.rej
abort: patch failed to apply
```

Thus I cannot check all tests still pass. We'll have to rely on the testbot.

Paul

### comment:55 in reply to: ↑ 54 Changed 8 years ago by jdemeyer

I could not apply the patch to 5.9 (after applying successfully the two dependencies):

Since essentially every hunk fails, it looks like you're applying the patch on top of itself.

Anyway, I updated the patch with your suggestions.

### comment:56 Changed 8 years ago by jdemeyer

• Status changed from needs_work to needs_review

### comment:57 Changed 8 years ago by jdemeyer

Paul, any chance for a review again?

### comment:58 Changed 8 years ago by zimmerma

Paul, any chance for a review again?

yes, will do.

### comment:59 Changed 8 years ago by zimmerma

on top of Sage 5.9, I get doctest failures:

```sage -t --long __init__.pyc  # AttributeError in doctesting framework
sage -t --long env.pyc  # AttributeError in doctesting framework
sage -t --long misc/interpreter.py  # 1 doctest failed
sage -t --long misc/trace.py  # 2 doctests failed
sage -t --long tests/cmdline.py  # 11 doctests failed
sage -t --long version.pyc  # AttributeError in doctesting framework
sage -t --long tests/interrupt.pyx  # Time out
```

Paul

### comment:60 Changed 8 years ago by jdemeyer

None of these failures look related to this ticket, what does a "clean" Sage 5.9 (without any applied patches) give? Please attach the actual doctest failures, not just the summary at the end, otherwise it is impossible to find out what went wrong.

### comment:61 Changed 8 years ago by zimmerma

maybe the doctest failures are due to some interaction with a spkg I installed in another clone of Sage (I installed the patches needed for #9880, and I believe there is side effect from one clone to the other ones for installed spkgs). Is there any way to get a "clean" Sage 5.9 without recompiling the sources again?

Anyway my remarks from comment 52 are taken into account, thus provided all tests pass with the testbot, I give a positive review.

Paul

### comment:62 Changed 8 years ago by jdemeyer

• Milestone changed from sage-5.10 to sage-5.11
• Status changed from needs_review to positive_review

### comment:63 Changed 8 years ago by jdemeyer

• Merged in set to sage-5.11.beta0
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.