Sage: Ticket #14416: weird conversion from QQ to RDF
https://trac.sagemath.org/ticket/14416
<p>
the following is weird:
</p>
<pre class="wiki">sage: RDF(1/10)-RDF(1)/RDF(10)
-1.38777878078e-17
</pre><p>
One would expect that the conversion from 1/10 to RDF is done as follows:
</p>
<ul><li>first convert 1 to RDF, which is exact
</li><li>then convert 10 to RDF, which is exact
</li><li>then divide RDF(1) by RDF(10)
</li></ul><p>
For RR we get as a comparison:
</p>
<pre class="wiki">sage: RR(1/10)-RR(1)/RR(10)
0.000000000000000
</pre><p>
More examples:
</p>
<pre class="wiki">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
</pre><p>
and for RR:
</p>
<pre class="wiki">sage: for p in [1..10]:
....: for q in [1..10]:
....: if RR(p/q) <> RR(p)/RR(q):
....: print p, q
....:
sage:
</pre><p>
<strong>Apply</strong> <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/14416/14416_QQ_to_RDF_v2.patch" title="Attachment '14416_QQ_to_RDF_v2.patch' in Ticket #14416">14416_QQ_to_RDF_v2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/14416/14416_QQ_to_RDF_v2.patch" title="Download"></a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/14416
Trac 1.1.6mhansenFri, 05 Apr 2013 14:52:27 GMT
https://trac.sagemath.org/ticket/14416#comment:1
https://trac.sagemath.org/ticket/14416#comment:1
<p>
Tracing things back, the conversion eventually gets done by:
</p>
<pre class="wiki">mpq_get_d(self.value)
</pre><p>
since <code>Rational</code> is just a wrapper around an <code>mpq</code>. This is where the weirdness seems to be.
</p>
TicketzimmermaFri, 05 Apr 2013 14:58:19 GMTstatus changed
https://trac.sagemath.org/ticket/14416#comment:2
https://trac.sagemath.org/ticket/14416#comment:2
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
<p>
ok, then the explanation is that <code>mpq_get_d</code> (according to the GMP manual) rounds towards zero.
</p>
<p>
One could call <code>mpz_get_d(numerator)/mpz_get(denominator)</code> but then for large numerator and denominator one would get three roundings, instead of only one when calling <code>mpq_get_d</code>.
</p>
<p>
I propose to close this ticket.
</p>
<p>
Paul
</p>
TicketkcrismanFri, 05 Apr 2013 17:21:52 GMT
https://trac.sagemath.org/ticket/14416#comment:3
https://trac.sagemath.org/ticket/14416#comment:3
<p>
So it's okay that
</p>
<pre class="wiki">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
</pre><p>
as Thierry pointed out <a class="ext-link" href="http://ask.sagemath.org/question/2444/what-are-the-differences-between-realdoublefield"><span class="icon"></span>here</a>? 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.
</p>
TickettscrimFri, 05 Apr 2013 18:34:34 GMTstatus, component, milestone changed
https://trac.sagemath.org/ticket/14416#comment:4
https://trac.sagemath.org/ticket/14416#comment:4
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_info</em>
</li>
<li><strong>component</strong>
changed from <em>basic arithmetic</em> to <em>documentation</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-5.10</em> to <em>sage-5.9</em>
</li>
</ul>
<p>
<code>RR</code> is not the same as <code>RDF</code> in regards to <em>how</em> 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 <code>RDF</code> are tied to your hardware (up to a certain precision) since I believe they are done in your native machine <code>double</code> 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.
</p>
<p>
The reason why <code>RR</code> works uniformly is as it was noted in the ask sage, it is <em>emulating</em> a CPU in a program (one can think of it as constant hardware).
</p>
<p>
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.
</p>
TickettscrimFri, 05 Apr 2013 18:35:08 GMTmilestone changed
https://trac.sagemath.org/ticket/14416#comment:5
https://trac.sagemath.org/ticket/14416#comment:5
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.9</em> to <em>sage-5.10</em>
</li>
</ul>
<p>
Whoops, didn't mean to change the milestone.
</p>
TickettmonteilFri, 05 Apr 2013 19:13:28 GMT
https://trac.sagemath.org/ticket/14416#comment:6
https://trac.sagemath.org/ticket/14416#comment:6
<p>
As Paul explained, the rounding is the same for RR and RDF: RDF rounds its computations to the nearest.
</p>
<p>
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.
</p>
<p>
IMHO, this conversion from QQ to RDF should be fixed, if possible.
</p>
TickettscrimFri, 05 Apr 2013 19:31:38 GMTcomponent changed
https://trac.sagemath.org/ticket/14416#comment:7
https://trac.sagemath.org/ticket/14416#comment:7
<ul>
<li><strong>component</strong>
changed from <em>documentation</em> to <em>basic arithmetic</em>
</li>
</ul>
<p>
Ah I see. I misread/misinterpreted Paul's explanation. I also agree that this should be fixed for consistency:
</p>
<pre class="wiki">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
</pre>
TicketzimmermaFri, 05 Apr 2013 20:50:14 GMT
https://trac.sagemath.org/ticket/14416#comment:8
https://trac.sagemath.org/ticket/14416#comment:8
<p>
Karl-Dieter,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:3" title="Comment 3">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
So it's okay that
</p>
<pre class="wiki">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
</pre><p>
as Thierry pointed out <a class="ext-link" href="http://ask.sagemath.org/question/2444/what-are-the-differences-between-realdoublefield"><span class="icon"></span>here</a>? 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.
</p>
</blockquote>
<p>
what is annoying is that RDF and RR give different results. The fact that RR gives <code>0.0</code> in that case
is not that important, for example:
</p>
<pre class="wiki">sage: RR(1/49)*49-RR(1)
-1.11022302462516e-16
</pre><p>
Paul
</p>
TicketzimmermaFri, 05 Apr 2013 21:06:23 GMT
https://trac.sagemath.org/ticket/14416#comment:9
https://trac.sagemath.org/ticket/14416#comment:9
<blockquote class="citation">
<p>
IMHO, this conversion from QQ to RDF should be fixed, if possible.
</p>
</blockquote>
<p>
it is possible: first call the MPFR function <code>mpfr_set_q</code> on the rational fraction
(which is what <code>RR(p/q)</code> does) then convert back to <code>double</code> with <code>mpfr_get_d</code>, but this might be less efficient than the current code, since it would convert from QQ to RR, then from RR to RDF.
</p>
<p>
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 <code>p/q</code> where <code>p=2403806706169061971</code> and <code>q=983883817941434958</code>. If you first round p and q to RDF (say with <code>mpz_get_d</code>), then you get
<code>2403806706169061888/983883817941435008</code>, which is rounded to nearest to
<code>5501555564164225/2251799813685248</code>, whereas the direct rounding of p/q to nearest gives
<code>2750777782082113/1125899906842624</code>:
</p>
<pre class="wiki">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
</pre><p>
Paul
</p>
TicketrobertwbSat, 06 Apr 2013 00:42:13 GMT
https://trac.sagemath.org/ticket/14416#comment:10
https://trac.sagemath.org/ticket/14416#comment:10
<p>
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 <code>mpq_get_d</code> rounded towards nearest, but until someone implements that in a manner that's at least comparable in speed to <code>mpq_get_d</code> I think the current behavior is more in line with the philosophy of RDF than making things slow to get the last bit correct.
</p>
TicketzimmermaSat, 06 Apr 2013 07:48:42 GMT
https://trac.sagemath.org/ticket/14416#comment:11
https://trac.sagemath.org/ticket/14416#comment:11
<p>
Robert, would the following be ok in what concerns speed?
</p>
<p>
Let p/q the fraction to be converted to RDF, I assume p, q > 0.
</p>
<p>
1) multiply p or q by a power of 2 into pp and qq so that both have the same number of bits
</p>
<blockquote>
<p>
(using mpz_sizeinbase and mpz_mul_2exp from GMP)
</p>
</blockquote>
<p>
2) if pp < qq, multiply pp by 2<sup>54</sup>, otherwise multiply pp by 2<sup>53</sup>, using mpz_mul_2exp
</p>
<p>
3) now 2<sup>53</sup> <= pp/qq < 2<sup>54</sup>, compute (using mpz_tdiv_qr) the quotient r = trunc(pp/qq) and the
</p>
<blockquote>
<p>
remainder s; we know that r has exactly 54 bits
</p>
</blockquote>
<p>
4) if r is even, return r*2<sup>k</sup> where k is the normalizing constant (exact since r is exact on 53 bits)
</p>
<p>
5) if s is not zero, return (r+1)*2<sup>k</sup> [r+1 is even, and exact on 53 bits, even in the
</p>
<blockquote>
<p>
case where r+1 = 2<sup>54</sup>]
</p>
</blockquote>
<p>
5a) if s=0, return (r-1)*2<sup>k</sup> if the bit of weight 1 of r is 0, otherwise (r+1)*2<sup>k</sup>
</p>
<p>
I guess mpq_get_d does basically steps 1-4, thus it should be comparable in speed.
</p>
<p>
Paul
</p>
TicketzimmermaTue, 09 Apr 2013 13:55:40 GMT
https://trac.sagemath.org/ticket/14416#comment:12
https://trac.sagemath.org/ticket/14416#comment:12
<p>
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 2<sup>53</sup> in absolute value, it is about twice as fast as <code>mpq_get_d</code>.
</p>
<pre class="wiki">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)
mpz_add_ui (aa, aa, 1);
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
mpz_add_ui (aa, aa, 1);
}
else
{
if (mpz_sgn (aa) > 0)
mpz_add_ui (aa, aa, 1);
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);
}
</pre><p>
However I don't know where to incorporate that code in Sage. Could someone help?
</p>
<p>
Paul
</p>
TicketkcrismanTue, 09 Apr 2013 14:27:35 GMT
https://trac.sagemath.org/ticket/14416#comment:13
https://trac.sagemath.org/ticket/14416#comment:13
<p>
<a class="ext-link" href="http://ask.sagemath.org/question/2444/what-are-the-differences-between-realdoublefield?answer=3357#3357"><span class="icon"></span>Volker points</a> to <a class="ext-link" href="http://hg.sagemath.org/sage-main/file/5714ed3eab6a/sage/rings/rational.pyx#l2020"><span class="icon"></span>here</a> in sage/rings/rational.pyx. I imagine that one could just replace that <code>return mpq_get_d(self.value)</code> with your code, or maybe make an auxiliary function since I suspect yours is pure C or C++ and this is a Cython file.
</p>
TickettscrimTue, 09 Apr 2013 14:45:16 GMT
https://trac.sagemath.org/ticket/14416#comment:14
https://trac.sagemath.org/ticket/14416#comment:14
<p>
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...?
</p>
TicketzimmermaTue, 09 Apr 2013 14:48:25 GMT
https://trac.sagemath.org/ticket/14416#comment:15
https://trac.sagemath.org/ticket/14416#comment:15
<p>
yes this is pure C code.
</p>
<p>
Paul
</p>
TicketzimmermaWed, 10 Apr 2013 09:09:24 GMT
https://trac.sagemath.org/ticket/14416#comment:16
https://trac.sagemath.org/ticket/14416#comment:16
<p>
note that we have currently:
</p>
<pre class="wiki">sage: RDF(2^(-1075))
0.0
sage: RDF(-2^(-1075))
0.0
</pre><p>
The last result should be <code>-0.0</code>, as in <code>-RDF(2^(-1075))</code>.
I guess my code above fixes that (not tested).
</p>
<p>
Paul
</p>
TicketjdemeyerWed, 10 Apr 2013 10:03:51 GMT
https://trac.sagemath.org/ticket/14416#comment:17
https://trac.sagemath.org/ticket/14416#comment:17
<p>
Is it really worth to do
</p>
<pre class="wiki"> /* 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);
</pre><p>
I think that this case is pretty rare, so I wouldn't special-case it.
</p>
TicketzimmermaWed, 10 Apr 2013 10:16:35 GMT
https://trac.sagemath.org/ticket/14416#comment:18
https://trac.sagemath.org/ticket/14416#comment:18
<p>
Jeroen,
</p>
<blockquote class="citation">
<p>
I think that this case is pretty rare, so I wouldn't special-case it.
</p>
</blockquote>
<p>
this was my first idea. Then I realized I was often using say <code>RDF(1/2^100)</code>.
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.
</p>
<p>
Paul
</p>
TicketjdemeyerWed, 10 Apr 2013 10:19:04 GMT
https://trac.sagemath.org/ticket/14416#comment:19
https://trac.sagemath.org/ticket/14416#comment:19
<p>
There is also this obvious bug:
</p>
<pre class="wiki">mpz_clear (aa);
[...]
mpz_get_d (aa);
</pre>
TicketzimmermaWed, 10 Apr 2013 10:59:33 GMT
https://trac.sagemath.org/ticket/14416#comment:20
https://trac.sagemath.org/ticket/14416#comment:20
<blockquote class="citation">
<p>
There is also this obvious bug:...
</p>
</blockquote>
<p>
sorry, please replace the last lines by:
</p>
<pre class="wiki"> d = mpz_get_d (aa); /* exact */
mpz_clear (aa);
mpz_clear (bb);
return ldexp (d, (long) sa - (long) sb);
</pre><p>
I also realize in the "hard case", there is a double-rounding issue for subnormals. This could be fixed if needed.
</p>
<p>
Paul
</p>
TicketjdemeyerWed, 10 Apr 2013 13:39:33 GMTauthor set
https://trac.sagemath.org/ticket/14416#comment:21
https://trac.sagemath.org/ticket/14416#comment:21
<ul>
<li><strong>author</strong>
set to <em>Paul ZImmermann, Jeroen Demeyer</em>
</li>
</ul>
<p>
I'm working on a Sage patch based on Paul's code.
</p>
TicketzimmermaWed, 10 Apr 2013 13:45:44 GMTauthor changed
https://trac.sagemath.org/ticket/14416#comment:22
https://trac.sagemath.org/ticket/14416#comment:22
<ul>
<li><strong>author</strong>
changed from <em>Paul ZImmermann, Jeroen Demeyer</em> to <em>Paul Zimmermann, Jeroen Demeyer</em>
</li>
</ul>
<blockquote class="citation">
<p>
I'm working on a Sage patch based on Paul's code.
</p>
</blockquote>
<p>
great! Please ask if you need some help. My code should deal correctly with underflow or overflow (through the call to ldexp).
</p>
<p>
Paul
</p>
TicketjdemeyerThu, 11 Apr 2013 07:03:39 GMTstatus changed
https://trac.sagemath.org/ticket/14416#comment:23
https://trac.sagemath.org/ticket/14416#comment:23
<ul>
<li><strong>status</strong>
changed from <em>needs_info</em> to <em>needs_review</em>
</li>
</ul>
TicketzimmermaFri, 12 Apr 2013 07:45:55 GMT
https://trac.sagemath.org/ticket/14416#comment:24
https://trac.sagemath.org/ticket/14416#comment:24
<p>
Jeroen,
</p>
<p>
as an author I'm not supposed to review that ticket, but I have a few questions:
</p>
<ul><li>why the <code>or A.condition()</code> in <code>matrix/matrix_double_dense.pyx</code>?
</li></ul><ul><li>in <code>matrix/matrix_mod2e_dense.pyx</code> I wonder why you had to change only one value,
and not all from lines 788 to 811
</li></ul><ul><li>in <code>plot/colors.py</code> the new value is closer to 2/7 than the old one (as expected):
<pre class="wiki">sage: (RR(rainbow(7, 'rgbtuple')[5][0]).exact_rational()-2/7)*1.0
-1.26882631385732e-16
</pre></li></ul><ul><li>idem in <code>rings/contfrac.py</code>:
<pre class="wiki">sage: (RR(float(a)).exact_rational()+17/389)*1.0
-1.60539961787057e-18
</pre></li></ul><ul><li>the test <code>float(1/10) * 10 == float(1)</code> 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).
</li></ul><ul><li>efficiency is not that bad in the "easy" case (was 4.8 ms before):
<pre class="wiki">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
</pre></li></ul><ul><li>we have lost a little in the "general" case (was 5.04 ms before):
<pre class="wiki">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
</pre></li></ul><ul><li><code>q0 = aa/bb / 2^shift</code> should be <code>q0 = a/b / 2^shift</code>
</li></ul><ul><li><code>|d| <= 2^-1075</code> should be <code>|d| < 2^-1075</code>
</li></ul><ul><li><code>shift >= 972</code> can be changed to <code>shift >= 971</code>, since for 971 we get <code>|d| >= 2^1024</code> and <code>2^1024</code> gives infinity
</li></ul><ul><li>maybe we can avoid the copy done by <code>mpz_init_set</code> and point directly to the corresponding numerator or denominator?
</li></ul><ul><li>I would add an example showing that the conversion now agrees with RR:
<pre class="wiki">sage: all([RDF(a/b) == RR(a/b) for a in [1..99] for b in [1..99]])
True
</pre></li></ul><p>
I see you have dealt with the subnormal case too: great!
</p>
<p>
Paul
</p>
TicketvbraunFri, 12 Apr 2013 08:58:15 GMT
https://trac.sagemath.org/ticket/14416#comment:25
https://trac.sagemath.org/ticket/14416#comment:25
<p>
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?
</p>
TicketzimmermaFri, 12 Apr 2013 09:03:44 GMT
https://trac.sagemath.org/ticket/14416#comment:26
https://trac.sagemath.org/ticket/14416#comment:26
<p>
Volker, you are right. I will ask the GMP developers.
</p>
<p>
Paul
</p>
TicketzimmermaFri, 12 Apr 2013 09:14:51 GMT
https://trac.sagemath.org/ticket/14416#comment:27
https://trac.sagemath.org/ticket/14416#comment:27
<p>
cf <a class="ext-link" href="http://gmplib.org/list-archives/gmp-devel/2013-April/003223.html"><span class="icon"></span>http://gmplib.org/list-archives/gmp-devel/2013-April/003223.html</a>
</p>
<p>
Paul
</p>
TicketjdemeyerFri, 12 Apr 2013 09:44:26 GMT
https://trac.sagemath.org/ticket/14416#comment:28
https://trac.sagemath.org/ticket/14416#comment:28
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:24" title="Comment 24">zimmerma</a>:
</p>
<blockquote class="citation">
<p>
Jeroen,
</p>
<p>
as an author I'm not supposed to review that ticket
</p>
</blockquote>
<p>
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.
</p>
<blockquote class="citation">
<ul><li>why the <code>or A.condition()</code> in <code>matrix/matrix_double_dense.pyx</code>?
</li></ul></blockquote>
<p>
That's a cool doctest trick I learned recently. If the condition fails, it will output the exact value of <code>A.condition()</code> instead of simply returning <code>False</code>.
</p>
<blockquote class="citation">
<ul><li>in <code>matrix/matrix_mod2e_dense.pyx</code> I wonder why you had to change only one value,
and not all from lines 788 to 811
</li></ul></blockquote>
<p>
Well, I assume the others are cases where the old and new mpq->double conversion code agree.
</p>
<blockquote class="citation">
<ul><li>the test <code>float(1/10) * 10 == float(1)</code> 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).
</li></ul></blockquote>
<p>
OK, true. I will remove that test.
</p>
<blockquote class="citation">
<ul><li>maybe we can avoid the copy done by <code>mpz_init_set</code> and point directly to the corresponding numerator or denominator?
</li></ul></blockquote>
<p>
I have no idea how to do that in Cython. Besides, we do change the value of <code>aa</code> and <code>bb</code>, so we need an init anyway.
</p>
<blockquote class="citation">
<ul><li>I would add an example showing that the conversion now agrees with RR:
<pre class="wiki">sage: all([RDF(a/b) == RR(a/b) for a in [1..99] for b in [1..99]])
True
</pre></li></ul></blockquote>
<p>
OK, good idea. I'll also test negative numerators.
</p>
TicketzimmermaFri, 12 Apr 2013 09:56:04 GMT
https://trac.sagemath.org/ticket/14416#comment:29
https://trac.sagemath.org/ticket/14416#comment:29
<p>
Jeroen,
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
as an author I'm not supposed to review that ticket
</p>
</blockquote>
<p>
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.
</p>
</blockquote>
<p>
I'm fine in giving comments, but I would prefer someone else gives "positive review".
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>why the <code>or A.condition()</code> in <code>matrix/matrix_double_dense.pyx</code>?
</li></ul></blockquote>
<p>
That's a cool doctest trick I learned recently. If the condition fails, it will output the exact value of <code>A.condition()</code> instead of simply returning <code>False</code>.
</p>
</blockquote>
<p>
good. I learned something too!
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>in <code>matrix/matrix_mod2e_dense.pyx</code> I wonder why you had to change only one value,
and not all from lines 788 to 811
</li></ul></blockquote>
<p>
Well, I assume the others are cases where the old and new mpq->double conversion code agree.
</p>
</blockquote>
<p>
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.
</p>
<blockquote class="citation">
<blockquote class="citation">
<ul><li>maybe we can avoid the copy done by <code>mpz_init_set</code> and point directly to the corresponding numerator or denominator?
</li></ul></blockquote>
<p>
I have no idea how to do that in Cython. Besides, we do change the value of <code>aa</code> and <code>bb</code>, so we need an init anyway.
</p>
</blockquote>
<p>
you could have different variables to store the quotient and remainder, and split the code
to perform the division directly on a or b.
</p>
<p>
Paul
</p>
TicketjdemeyerFri, 12 Apr 2013 10:43:39 GMTattachment set
https://trac.sagemath.org/ticket/14416
https://trac.sagemath.org/ticket/14416
<ul>
<li><strong>attachment</strong>
set to <em>14416_QQ_to_RDF.patch</em>
</li>
</ul>
TicketjdemeyerFri, 12 Apr 2013 11:22:13 GMT
https://trac.sagemath.org/ticket/14416#comment:30
https://trac.sagemath.org/ticket/14416#comment:30
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:29" title="Comment 29">zimmerma</a>:
</p>
<blockquote class="citation">
<p>
you could have different variables to store the quotient and remainder, and split the code
to perform the division directly on a or b.
</p>
</blockquote>
<p>
Yes, that's what I did in the new patch.
</p>
TicketzimmermaFri, 12 Apr 2013 11:38:55 GMT
https://trac.sagemath.org/ticket/14416#comment:31
https://trac.sagemath.org/ticket/14416#comment:31
<p>
with the new patch (I did not check with the old one) I get warnings:
</p>
<pre class="wiki">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
</pre><p>
Paul
</p>
TicketzimmermaFri, 12 Apr 2013 11:42:20 GMT
https://trac.sagemath.org/ticket/14416#comment:32
https://trac.sagemath.org/ticket/14416#comment:32
<p>
performance is not better in the general case:
</p>
<pre class="wiki">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
</pre><p>
Paul
</p>
TicketjdemeyerFri, 12 Apr 2013 11:45:40 GMT
https://trac.sagemath.org/ticket/14416#comment:33
https://trac.sagemath.org/ticket/14416#comment:33
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:31" title="Comment 31">zimmerma</a>:
</p>
<blockquote class="citation">
<p>
with the new patch (I did not check with the old one) I get warnings
</p>
</blockquote>
<p>
Sure, we get these warnings all over the place. One could say this is a Cython bug.
</p>
TicketjdemeyerFri, 12 Apr 2013 12:49:14 GMTdescription changed
https://trac.sagemath.org/ticket/14416#comment:34
https://trac.sagemath.org/ticket/14416#comment:34
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/14416?action=diff&version=34">diff</a>)
</li>
</ul>
<p>
I added a second version which uses <code>uint64_t</code> in the second part, which is hopefully slightly faster.
</p>
TicketjdemeyerFri, 12 Apr 2013 13:05:55 GMT
https://trac.sagemath.org/ticket/14416#comment:35
https://trac.sagemath.org/ticket/14416#comment:35
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:24" title="Comment 24">zimmerma</a>:
</p>
<blockquote class="citation">
<ul><li>why the <code>or A.condition()</code> in <code>matrix/matrix_double_dense.pyx</code>?
</li></ul></blockquote>
<p>
To elaborate on this: <code>X or Y</code> returns <code>X</code> if <code>bool(X)</code> is true and returns <code>Y</code> if <code>bool(X)</code> is false. The opposite for <code>X and Y</code>.
</p>
TicketzimmermaFri, 12 Apr 2013 13:24:36 GMT
https://trac.sagemath.org/ticket/14416#comment:36
https://trac.sagemath.org/ticket/14416#comment:36
<p>
the new version is slightly better in the general case:
</p>
<pre class="wiki">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
</pre><p>
Paul
</p>
TicketzimmermaFri, 12 Apr 2013 13:34:55 GMT
https://trac.sagemath.org/ticket/14416#comment:37
https://trac.sagemath.org/ticket/14416#comment:37
<p>
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.
</p>
<p>
Could anybody out there have a final look and give a positive review?
</p>
<p>
Paul
</p>
TicketzimmermaFri, 12 Apr 2013 14:40:39 GMT
https://trac.sagemath.org/ticket/14416#comment:38
https://trac.sagemath.org/ticket/14416#comment:38
<p>
btw, a small typo in the patch: occured should be occurred...
</p>
<p>
Paul
</p>
TickettmonteilFri, 12 Apr 2013 22:56:27 GMT
https://trac.sagemath.org/ticket/14416#comment:39
https://trac.sagemath.org/ticket/14416#comment:39
<p>
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).
</p>
<pre class="wiki">sage: RDF(1/10)*10 == RDF(1)
True
</pre><p>
and even
</p>
<pre class="wiki">sage: all([RDF(p/q) == RDF(p)/RDF(q) for p in [-100..100] for q in [1..100]])
True
</pre>
TicketzimmermaSat, 13 Apr 2013 07:30:52 GMT
https://trac.sagemath.org/ticket/14416#comment:40
https://trac.sagemath.org/ticket/14416#comment:40
<p>
Thierry, I asked Jeroen to remove from the doctest <code>RDF(1/10)*10 == RDF(1)</code> since this is wrong if you replace 10 by say 49, so this was not a bug, but a feature of rounding.
</p>
<p>
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).
</p>
<p>
Paul
</p>
TicketjdemeyerSat, 13 Apr 2013 11:09:34 GMT
https://trac.sagemath.org/ticket/14416#comment:41
https://trac.sagemath.org/ticket/14416#comment:41
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:39" title="Comment 39">tmonteil</a>:
</p>
<blockquote class="citation">
<pre class="wiki">sage: all([RDF(p/q) == RDF(p)/RDF(q) for p in [-100..100] for q in [1..100]])
True
</pre></blockquote>
<p>
I would say this test is also misleading since <code>RDF(p/q)</code> and <code>RDF(p)/RDF(q)</code> don't have to be the same for large value of <code>p</code> and <code>q</code>.
</p>
TicketzimmermaSat, 13 Apr 2013 17:11:51 GMT
https://trac.sagemath.org/ticket/14416#comment:42
https://trac.sagemath.org/ticket/14416#comment:42
<blockquote class="citation">
<p>
I would say this test is also misleading since RDF(p/q) and RDF(p)/RDF(q) don't have to be the same...
</p>
</blockquote>
<p>
yes, but for p, q integers in [-100,100] the conversion to RDF is exact.
</p>
<p>
Paul
</p>
TickettmonteilSun, 14 Apr 2013 22:48:02 GMTattachment set
https://trac.sagemath.org/ticket/14416
https://trac.sagemath.org/ticket/14416
<ul>
<li><strong>attachment</strong>
set to <em>trac_14416_rounding_doctest-tm.patch</em>
</li>
</ul>
<p>
Tested on sage 5.9.beta5, depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/14448" title="defect: Bad sign in sign_mantissa_exponent() methods (closed: fixed)">#14448</a>
</p>
TickettmonteilSun, 14 Apr 2013 22:49:48 GMTdependencies set
https://trac.sagemath.org/ticket/14416#comment:43
https://trac.sagemath.org/ticket/14416#comment:43
<ul>
<li><strong>dependencies</strong>
set to <em>#14448</em>
</li>
</ul>
<p>
Perhaps the following test has a better semantics, and still represents the initial bug.
</p>
<pre class="wiki">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
</pre><p>
Unfortunately, this lets me found a bug in the <code>.sign_mantissa_exponent()</code> (which currently gives negative mantissa to negative numbers), see <a class="closed ticket" href="https://trac.sagemath.org/ticket/14448" title="defect: Bad sign in sign_mantissa_exponent() methods (closed: fixed)">#14448</a>.
</p>
TicketjdemeyerMon, 15 Apr 2013 07:27:57 GMTdependencies deleted
https://trac.sagemath.org/ticket/14416#comment:44
https://trac.sagemath.org/ticket/14416#comment:44
<ul>
<li><strong>dependencies</strong>
<em>#14448</em> deleted
</li>
</ul>
<p>
OK, I added your doctest in a simplified way such that it doesn't depend on <a class="closed ticket" href="https://trac.sagemath.org/ticket/14448" title="defect: Bad sign in sign_mantissa_exponent() methods (closed: fixed)">#14448</a>.
</p>
TicketzimmermaMon, 15 Apr 2013 07:44:11 GMT
https://trac.sagemath.org/ticket/14416#comment:45
https://trac.sagemath.org/ticket/14416#comment:45
<p>
Jeroen, what is status of this ticket for the patchbot? Last time I looked, some tests were failing.
</p>
<p>
Paul
</p>
TicketjdemeyerMon, 15 Apr 2013 08:15:24 GMT
https://trac.sagemath.org/ticket/14416#comment:46
https://trac.sagemath.org/ticket/14416#comment:46
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:45" title="Comment 45">zimmerma</a>:
</p>
<blockquote class="citation">
<p>
Jeroen, what is status of this ticket for the patchbot? Last time I looked, some tests were failing.
</p>
</blockquote>
<p>
I fixed some more doctest failures in <code>sage/matrix/matrix_double_dense.pyx</code>.
</p>
TicketzimmermaMon, 15 Apr 2013 08:27:27 GMT
https://trac.sagemath.org/ticket/14416#comment:47
https://trac.sagemath.org/ticket/14416#comment:47
<blockquote class="citation">
<p>
I fixed some more doctest failures in sage/matrix/matrix_double_dense.pyx.
</p>
</blockquote>
<p>
all tests do pass now?
</p>
<p>
Paul
</p>
TicketjdemeyerMon, 15 Apr 2013 08:35:58 GMT
https://trac.sagemath.org/ticket/14416#comment:48
https://trac.sagemath.org/ticket/14416#comment:48
<p>
Some tests might be machine-specific, but at least on most machines, all tests pass indeed.
</p>
TicketjdemeyerTue, 30 Apr 2013 11:11:53 GMTstatus changed; dependencies set
https://trac.sagemath.org/ticket/14416#comment:49
https://trac.sagemath.org/ticket/14416#comment:49
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>dependencies</strong>
set to <em>#14335, #14336</em>
</li>
</ul>
TicketjdemeyerTue, 30 Apr 2013 20:33:04 GMTstatus changed
https://trac.sagemath.org/ticket/14416#comment:50
https://trac.sagemath.org/ticket/14416#comment:50
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Rebased for doctest failures with <a class="closed ticket" href="https://trac.sagemath.org/ticket/14336" title="enhancement: update M4RIE to version 20130416 (closed: fixed)">#14336</a>.
</p>
<p>
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?
</p>
TicketzimmermaFri, 03 May 2013 15:18:24 GMT
https://trac.sagemath.org/ticket/14416#comment:51
https://trac.sagemath.org/ticket/14416#comment:51
<p>
I will try to review it next week. But if someone beats me, no problem!
</p>
<p>
Paul
</p>
TicketzimmermaTue, 07 May 2013 06:19:50 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/14416#comment:52
https://trac.sagemath.org/ticket/14416#comment:52
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>reviewer</strong>
set to <em>Paul Zimmermann</em>
</li>
</ul>
<p>
Jeroen, a few tiny comments:
</p>
<ul><li>in the following test, you can replace 20 by 13. Also, you could add <code>all([RDF(q) == RR(q) for q in Q])</code> which would exercise the code for large numerators and denominators.
<pre class="wiki">sage: Q = continued_fraction(pi, bits=3000).convergents()[20:]
sage: RDFpi = RDF(pi)
sage: all([RDF(q) == RDFpi for q in Q])
</pre></li></ul><ul><li>is the <code>except?</code> value really needed in <code>mpq_get_d_nearest</code>?
</li></ul><ul><li>please replace <code>round-to-even</code> with <code>round-to-nearest-even</code>. Round to even and round to odd also exist.
</li></ul><ul><li>please replace <code>occured</code> by <code>occurred</code> (several places, already mentioned)
</li></ul><p>
Apart from that (and if tests still pass) I'm fine with the patch.
</p>
<p>
Paul
</p>
TicketjdemeyerTue, 07 May 2013 06:43:38 GMT
https://trac.sagemath.org/ticket/14416#comment:53
https://trac.sagemath.org/ticket/14416#comment:53
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:52" title="Comment 52">zimmerma</a>:
</p>
<blockquote class="citation">
<p>
Jeroen, a few tiny comments:
</p>
<ul><li>in the following test, you can replace 20 by 13.
</li></ul></blockquote>
<p>
Sure, but I wanted some safety margin.
</p>
<blockquote class="citation">
<p>
Also, you could add <code>all([RDF(q) == RR(q) for q in Q])</code>
</p>
</blockquote>
<p>
OK, but then for all covergents (before throwing away the first 20).
</p>
<blockquote class="citation">
<ul><li>is the <code>except?</code> value really needed in <code>mpq_get_d_nearest</code>?
</li></ul></blockquote>
<p>
Yes, because <code>sig_on()</code> can throw exceptions.
</p>
TicketzimmermaTue, 07 May 2013 07:47:55 GMT
https://trac.sagemath.org/ticket/14416#comment:54
https://trac.sagemath.org/ticket/14416#comment:54
<p>
I could not apply the patch to 5.9 (after applying successfully the two dependencies):
</p>
<pre class="wiki">----------------------------------------------------------------------
| 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
</pre><p>
Thus I cannot check all tests still pass. We'll have to rely on the testbot.
</p>
<p>
Paul
</p>
TicketjdemeyerTue, 07 May 2013 08:57:31 GMTattachment set
https://trac.sagemath.org/ticket/14416
https://trac.sagemath.org/ticket/14416
<ul>
<li><strong>attachment</strong>
set to <em>14416_QQ_to_RDF_v2.patch</em>
</li>
</ul>
TicketjdemeyerTue, 07 May 2013 08:58:45 GMT
https://trac.sagemath.org/ticket/14416#comment:55
https://trac.sagemath.org/ticket/14416#comment:55
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:54" title="Comment 54">zimmerma</a>:
</p>
<blockquote class="citation">
<p>
I could not apply the patch to 5.9 (after applying successfully the two dependencies):
</p>
</blockquote>
<p>
Since essentially every hunk fails, it looks like you're applying the patch on top of itself.
</p>
<p>
Anyway, I updated the patch with your suggestions.
</p>
TicketjdemeyerTue, 07 May 2013 08:59:29 GMTstatus changed
https://trac.sagemath.org/ticket/14416#comment:56
https://trac.sagemath.org/ticket/14416#comment:56
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketjdemeyerFri, 24 May 2013 11:59:04 GMT
https://trac.sagemath.org/ticket/14416#comment:57
https://trac.sagemath.org/ticket/14416#comment:57
<p>
Paul, any chance for a review again?
</p>
TicketzimmermaFri, 24 May 2013 12:24:49 GMT
https://trac.sagemath.org/ticket/14416#comment:58
https://trac.sagemath.org/ticket/14416#comment:58
<blockquote class="citation">
<p>
Paul, any chance for a review again?
</p>
</blockquote>
<p>
yes, will do.
</p>
TicketzimmermaMon, 27 May 2013 08:26:58 GMT
https://trac.sagemath.org/ticket/14416#comment:59
https://trac.sagemath.org/ticket/14416#comment:59
<p>
on top of Sage 5.9, I get doctest failures:
</p>
<pre class="wiki">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
</pre><p>
Paul
</p>
TicketjdemeyerMon, 27 May 2013 08:31:02 GMT
https://trac.sagemath.org/ticket/14416#comment:60
https://trac.sagemath.org/ticket/14416#comment:60
<p>
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.
</p>
TicketzimmermaMon, 27 May 2013 08:38:14 GMT
https://trac.sagemath.org/ticket/14416#comment:61
https://trac.sagemath.org/ticket/14416#comment:61
<p>
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 <a class="closed ticket" href="https://trac.sagemath.org/ticket/9880" title="defect: Pynac comparison functions do not provide a SWO (closed: fixed)">#9880</a>, 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?
</p>
<p>
Anyway my remarks from comment <a class="ticket" href="https://trac.sagemath.org/ticket/14416#comment:52" title="Comment 52">52</a> are taken into account, thus provided all tests pass with the testbot, I give a positive review.
</p>
<p>
Paul
</p>
TicketjdemeyerThu, 30 May 2013 08:01:36 GMTstatus, milestone changed
https://trac.sagemath.org/ticket/14416#comment:62
https://trac.sagemath.org/ticket/14416#comment:62
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-5.10</em> to <em>sage-5.11</em>
</li>
</ul>
TicketjdemeyerThu, 06 Jun 2013 12:32:33 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/14416#comment:63
https://trac.sagemath.org/ticket/14416#comment:63
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-5.11.beta0</em>
</li>
</ul>
Ticket