Sage: Ticket #26360: Upgrade arb to 2.15.1
https://trac.sagemath.org/ticket/26360
<p>
Arb 2.15.1 is out and the upgrade breaks a lot (~200) doctests with changes that are not actually failures. Nearly all the changes are minor differences in the accuracy of the radius. As previously discussed in <a class="closed ticket" href="https://trac.sagemath.org/ticket/25966" title="enhancement: Upgrade Arb to 2.14.0 (closed: fixed)">#25966</a>, I think we should fix this once and for all. The tests are brittle and break with more or less every arb upgrade. All those "failures" distract from the actual failures. It makes arb upgrades painful and is a burden on distributions.
</p>
<p>
I suspect the solution here (lots of <code>...</code>) will be controversial. I don't like it very much myself. But I think it is better than the status quo. What we really want are different tolerances for the "mid" and the "radius". I don't know if that is possible in the doctesting framework without explicitly testing for <code>.mid()</code> and <code>.radius()</code> in different tests each time. We may even want to test the <code>.accuracy()</code> instead of the radius.
</p>
<p>
Upstream tarball: <a class="ext-link" href="https://github.com/fredrik-johansson/arb/archive/2.15.1.tar.gz"><span class="icon"></span>https://github.com/fredrik-johansson/arb/archive/2.15.1.tar.gz</a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/26360
Trac 1.1.6fredrik.johanssonFri, 28 Sep 2018 15:25:42 GMT
https://trac.sagemath.org/ticket/26360#comment:1
https://trac.sagemath.org/ticket/26360#comment:1
<p>
It would be nice to have some hook for doctests so that balls by default are compared in terms of overlapping and that the radii are the same within a factor 10, say.
</p>
TicketfbisseyFri, 28 Sep 2018 20:31:20 GMTcc changed
https://trac.sagemath.org/ticket/26360#comment:2
https://trac.sagemath.org/ticket/26360#comment:2
<ul>
<li><strong>cc</strong>
<em>fbissey</em> added
</li>
</ul>
TicketmmezzarobbaSat, 29 Sep 2018 08:05:33 GMT
https://trac.sagemath.org/ticket/26360#comment:3
https://trac.sagemath.org/ticket/26360#comment:3
<p>
Replying to <a class="closed ticket" href="https://trac.sagemath.org/ticket/26360" title="enhancement: Upgrade arb to 2.15.1 (closed: fixed)">gh-timokau</a>:
</p>
<blockquote class="citation">
<p>
I suspect the solution here (lots of <code>...</code>) will be controversial. I don't like it very much myself.
</p>
</blockquote>
<p>
I'm okay with it, but I'd like to see what Jeroen says since he insisted for exact doctests back then.
</p>
<blockquote class="citation">
<p>
What we really want are different tolerances for the "mid" and the "radius".
</p>
</blockquote>
<p>
Is that really necessary? If you use absolute tolerances, having the same tolerance for the center and the radius isn't too bad.
</p>
<p>
Something like what Fredrik suggests would be better, of course. Since arb provides a way to parse strings of the form <code>[mid +/- rad]</code>, it is perhaps doable. (I'm not volunteering, though...)
</p>
Ticketgh-timokauSat, 20 Oct 2018 21:24:44 GMT
https://trac.sagemath.org/ticket/26360#comment:4
https://trac.sagemath.org/ticket/26360#comment:4
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/26360#comment:3" title="Comment 3">mmezzarobba</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="closed ticket" href="https://trac.sagemath.org/ticket/26360" title="enhancement: Upgrade arb to 2.15.1 (closed: fixed)">gh-timokau</a>:
</p>
<blockquote class="citation">
<p>
I suspect the solution here (lots of <code>...</code>) will be controversial. I don't like it very much myself.
</p>
</blockquote>
<p>
I'm okay with it, but I'd like to see what Jeroen says since he insisted for exact doctests back then.
</p>
</blockquote>
<p>
Jeroen, do you still insist on exact doctests here?
</p>
Ticketgh-timokauSat, 20 Oct 2018 21:25:25 GMT
https://trac.sagemath.org/ticket/26360#comment:5
https://trac.sagemath.org/ticket/26360#comment:5
<blockquote class="citation">
<blockquote class="citation">
<p>
What we really want are different tolerances for the "mid" and the "radius".
</p>
</blockquote>
<p>
Is that really necessary? If you use absolute tolerances, having the same tolerance for the center and the radius isn't too bad.
</p>
</blockquote>
<p>
That might also a (while not pretty) workable solution.
</p>
<blockquote class="citation">
<p>
Something like what Fredrik suggests would be better, of course. Since arb provides a way to parse strings of the form <code>[mid +/- rad]</code>, it is perhaps doable. (I'm not volunteering, though...)
</p>
</blockquote>
<p>
Is the doctesting framework even that flexible?
</p>
TicketjdemeyerSun, 21 Oct 2018 08:30:59 GMT
https://trac.sagemath.org/ticket/26360#comment:6
https://trac.sagemath.org/ticket/26360#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/26360#comment:4" title="Comment 4">gh-timokau</a>:
</p>
<blockquote class="citation">
<p>
Jeroen, do you still insist on exact doctests here?
</p>
</blockquote>
<p>
I don't recall why I originally insisted on that. So go ahead.
</p>
TicketjdemeyerSun, 21 Oct 2018 08:40:34 GMT
https://trac.sagemath.org/ticket/26360#comment:7
https://trac.sagemath.org/ticket/26360#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/26360#comment:3" title="Comment 3">mmezzarobba</a>:
</p>
<blockquote class="citation">
<p>
Something like what Fredrik suggests would be better, of course. Since arb provides a way to parse strings of the form <code>[mid +/- rad]</code>, it is perhaps doable. (I'm not volunteering, though...)
</p>
</blockquote>
<p>
I'm not convinced that we need to patch the doctest framework for that.
</p>
Ticketgh-timokauTue, 23 Oct 2018 21:34:11 GMT
https://trac.sagemath.org/ticket/26360#comment:8
https://trac.sagemath.org/ticket/26360#comment:8
<p>
I can't really do much about the remaining failures, since I lack the subject knowledge to decide weather or not they are real errors:
</p>
<pre class="wiki">sage -t --long src/sage/rings/complex_arb.pyx
**********************************************************************
File "src/sage/rings/complex_arb.pyx", line 4403, in sage.rings.complex_arb.ComplexBall.elliptic_f
Failed example:
(CBF.pi()/2).elliptic_f(phi)
Expected:
[1.5092369540513 +/- ...e-14] + [0.6251464152027 +/- ...e-14]*I
Got:
nan + nan*I
**********************************************************************
File "src/sage/rings/complex_arb.pyx", line 4409, in sage.rings.complex_arb.ComplexBall.elliptic_f
Failed example:
(CBF.pi()/2).elliptic_f(phi)
Expected:
[1.3393589639094 +/- ...e-14] + [1.1104369690719 +/- ...e-14]*I
Got:
nan + nan*I
**********************************************************************
File "src/sage/rings/complex_arb.pyx", line 4441, in sage.rings.complex_arb.ComplexBall.elliptic_e_inc
Failed example:
(CBF.pi()/2).elliptic_e_inc(phi)
Expected:
[1.2838409578982 +/- ...e-14] + [-0.5317843366915 +/- ...e-14]*I
Got:
nan + nan*I
**********************************************************************
File "src/sage/rings/complex_arb.pyx", line 4447, in sage.rings.complex_arb.ComplexBall.elliptic_e_inc
Failed example:
(CBF.pi()/2).elliptic_e_inc(phi)
Expected:
[0.787564350925 +/- ...e-13] + [-0.686896129145 +/- ...e-13]*I
Got:
nan + nan*I
**********************************************************************
File "src/sage/rings/complex_arb.pyx", line 4481, in sage.rings.complex_arb.ComplexBall.elliptic_pi_inc
Failed example:
n.elliptic_pi_inc(CBF.pi()/2, m)
Expected:
[0.8934793755173 +/- ...e-14] + [0.95707868710750 +/- ...e-15]*I
Got:
nan + nan*I
**********************************************************************
File "src/sage/rings/complex_arb.pyx", line 4488, in sage.rings.complex_arb.ComplexBall.elliptic_pi_inc
Failed example:
n.elliptic_pi_inc(CBF.pi()/2, m)
Expected:
[0.2969588746419 +/- ...e-14] + [1.318879533274 +/- ...e-13]*I
Got:
nan + nan*I
**********************************************************************
3 items had failures:
2 of 8 in sage.rings.complex_arb.ComplexBall.elliptic_e_inc
2 of 8 in sage.rings.complex_arb.ComplexBall.elliptic_f
2 of 10 in sage.rings.complex_arb.ComplexBall.elliptic_pi_inc
[622 tests, 6 failures, 11.97 s]
</pre><pre class="wiki">sage -t --long src/sage/rings/real_arb.pyx
**********************************************************************
File "src/sage/rings/real_arb.pyx", line 1305, in sage.rings.real_arb.RealBall.__init__
Failed example:
RBF("2.3e10000000000000000000000 +/- ...e10000000000000000000000")
Exception raised:
Traceback (most recent call last):
File "/home/timo/repos2/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 659, in _run
self.compile_and_execute(example, compiler, test.globs)
File "/home/timo/repos2/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1070, in compile_and_execute
exec(compiled, globs)
File "<doctest sage.rings.real_arb.RealBall.__init__[35]>", line 1, in <module>
RBF("2.3e10000000000000000000000 +/- ...e10000000000000000000000")
File "sage/structure/parent.pyx", line 921, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9679)
return mor._call_(x)
File "sage/structure/coerce_maps.pyx", line 145, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4574)
raise
File "sage/structure/coerce_maps.pyx", line 140, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4462)
return C._element_constructor(x)
File "sage/rings/real_arb.pyx", line 512, in sage.rings.real_arb.RealBallField._element_constructor_ (build/cythonized/sage/rings/real_arb.c:5843)
return self.element_class(self, mid, rad)
File "sage/rings/real_arb.pyx", line 1376, in sage.rings.real_arb.RealBall.__init__ (build/cythonized/sage/rings/real_arb.c:13451)
raise ValueError("unsupported string format")
ValueError: unsupported string format
**********************************************************************
1 item had failures:
1 of 48 in sage.rings.real_arb.RealBall.__init__
[530 tests, 1 failure, 0.78 s]
</pre>
Ticketfredrik.johanssonWed, 24 Oct 2018 13:38:41 GMT
https://trac.sagemath.org/ticket/26360#comment:9
https://trac.sagemath.org/ticket/26360#comment:9
<p>
That's a genuine regression in Arb 2.15. I have not tried to debug it, but quite likely the original code was buggy and only worked by accident -- it works if you add CBF("+/- 1e-10") to pi/2.
</p>
Ticketgh-timokauWed, 24 Oct 2018 13:42:10 GMT
https://trac.sagemath.org/ticket/26360#comment:10
https://trac.sagemath.org/ticket/26360#comment:10
<blockquote class="citation">
<p>
it works if you add CBF("+/- 1e-10") to pi/2
</p>
</blockquote>
<p>
So should we just do that in the doctests?
</p>
Ticketfredrik.johanssonWed, 24 Oct 2018 20:38:47 GMT
https://trac.sagemath.org/ticket/26360#comment:11
https://trac.sagemath.org/ticket/26360#comment:11
<p>
This commit should provide a bugfix: <a class="ext-link" href="https://github.com/fredrik-johansson/arb/commit/eb60d7ac4ee85cd6f8353dd9cf52a10bfd5967c3"><span class="icon"></span>https://github.com/fredrik-johansson/arb/commit/eb60d7ac4ee85cd6f8353dd9cf52a10bfd5967c3</a>
</p>
<p>
The other failure ("unsupported string format") is due to too greedy search & replace to insert ...
</p>
Ticketgh-timokauWed, 24 Oct 2018 21:26:59 GMT
https://trac.sagemath.org/ticket/26360#comment:12
https://trac.sagemath.org/ticket/26360#comment:12
<p>
Do you plan to make a release with that fix included anytime soon?
</p>
Ticketfredrik.johanssonThu, 25 Oct 2018 19:15:53 GMT
https://trac.sagemath.org/ticket/26360#comment:13
https://trac.sagemath.org/ticket/26360#comment:13
<p>
Done. <a class="ext-link" href="https://github.com/fredrik-johansson/arb/archive/2.15.1.tar.gz"><span class="icon"></span>https://github.com/fredrik-johansson/arb/archive/2.15.1.tar.gz</a>
</p>
Ticketgh-timokauFri, 26 Oct 2018 16:19:57 GMT
https://trac.sagemath.org/ticket/26360#comment:14
https://trac.sagemath.org/ticket/26360#comment:14
<p>
Thank you! Could you expand on what you mean by "due to too greedy search & replace to insert"? As far as I can see it is caused by <code>arb_set_str</code> returning false.
</p>
Ticketfredrik.johanssonFri, 26 Oct 2018 16:21:24 GMT
https://trac.sagemath.org/ticket/26360#comment:15
https://trac.sagemath.org/ticket/26360#comment:15
<p>
There is "..." in the input string which shouldn't have been inserted there in place of the numerical value.
</p>
TicketgitSat, 27 Oct 2018 07:42:30 GMTcommit changed
https://trac.sagemath.org/ticket/26360#comment:16
https://trac.sagemath.org/ticket/26360#comment:16
<ul>
<li><strong>commit</strong>
changed from <em>00d3ccd2ee230d9252b9cdc3183adf9c7f9f45ad</em> to <em>30cc778d46579bd0c7537ed33e8d7a4f40fd5c31</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="https://git.sagemath.org/sage.git/commit/?id=30cc778d46579bd0c7537ed33e8d7a4f40fd5c31"><span class="icon"></span>30cc778</a></td><td><code>Upgrade arb to 2.15.1</code>
</td></tr></table>
Ticketgh-timokauSat, 27 Oct 2018 07:44:19 GMTstatus, description, summary changed
https://trac.sagemath.org/ticket/26360#comment:17
https://trac.sagemath.org/ticket/26360#comment:17
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/26360?action=diff&version=17">diff</a>)
</li>
<li><strong>summary</strong>
changed from <em>Upgrade arb to 2.15.0</em> to <em>Upgrade arb to 2.15.1</em>
</li>
</ul>
<p>
Oh, stupid mistake. Thanks.
</p>
<p>
Doctests pass now.
</p>
TicketembraySun, 28 Oct 2018 11:54:34 GMT
https://trac.sagemath.org/ticket/26360#comment:18
https://trac.sagemath.org/ticket/26360#comment:18
<p>
I think rather than so many <code>...</code>, if there are changes we can make to the doctest parser to make these tests a little more natural that would be better, and probably not a big deal...
</p>
Ticketgh-timokauSun, 28 Oct 2018 12:35:39 GMT
https://trac.sagemath.org/ticket/26360#comment:19
https://trac.sagemath.org/ticket/26360#comment:19
<p>
I wouldn't know how to do that.
</p>
TicketembraySun, 28 Oct 2018 12:55:14 GMT
https://trac.sagemath.org/ticket/26360#comment:20
https://trac.sagemath.org/ticket/26360#comment:20
<p>
I mean, I would, but I don't know exactly what it is you need in this case.
</p>
TicketembraySun, 28 Oct 2018 12:59:02 GMT
https://trac.sagemath.org/ticket/26360#comment:21
https://trac.sagemath.org/ticket/26360#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/26360#comment:5" title="Comment 5">gh-timokau</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<p>
What we really want are different tolerances for the "mid" and the "radius".
</p>
</blockquote>
<p>
Is that really necessary? If you use absolute tolerances, having the same tolerance for the center and the radius isn't too bad.
</p>
</blockquote>
<p>
That might also a (while not pretty) workable solution.
</p>
</blockquote>
<p>
Did you ever try this? I don't really see it as "pretty" or not "pretty". An <code># abs tol</code> will apply to both values, but we really only care about it in this case for the radius. Technically it would be applied to the mid too but it doesn't matter either way for the present purpose.
</p>
Ticketgh-timokauMon, 29 Oct 2018 08:47:00 GMT
https://trac.sagemath.org/ticket/26360#comment:22
https://trac.sagemath.org/ticket/26360#comment:22
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/26360#comment:20" title="Comment 20">embray</a>:
</p>
<blockquote class="citation">
<p>
I mean, I would, but I don't know exactly what it is you need in this case.
</p>
</blockquote>
<p>
We'd need to use proper comparison methods instead of just comparing the digits. Something like what Frederik <a class="ticket" href="https://trac.sagemath.org/ticket/26360#comment:1" title="Comment 1">said</a>.
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/26360#comment:21" title="Comment 21">embray</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/26360#comment:5" title="Comment 5">gh-timokau</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<blockquote class="citation">
<p>
What we really want are different tolerances for the "mid" and the "radius".
</p>
</blockquote>
<p>
Is that really necessary? If you use absolute tolerances, having the same tolerance for the center and the radius isn't too bad.
</p>
</blockquote>
<p>
That might also a (while not pretty) workable solution.
</p>
</blockquote>
<p>
Did you ever try this? I don't really see it as "pretty" or not "pretty". An <code># abs tol</code> will apply to both values, but we really only care about it in this case for the radius. Technically it would be applied to the mid too but it doesn't matter either way for the present purpose.
</p>
</blockquote>
<p>
No I haven't. It would require either a lot of manual adding of <code># abs tol</code> or a very clever sed script. It doesn't have high enough priority for me right now to spend the time doing that, but I'd be happy with the solution if somebody else would.
</p>
TicketembrayWed, 31 Oct 2018 13:21:22 GMT
https://trac.sagemath.org/ticket/26360#comment:23
https://trac.sagemath.org/ticket/26360#comment:23
<p>
I'll give it a try and see. I'm pretty good at ridiculous regular expressions :)
</p>
Ticketfredrik.johanssonTue, 06 Nov 2018 09:47:07 GMT
https://trac.sagemath.org/ticket/26360#comment:24
https://trac.sagemath.org/ticket/26360#comment:24
<p>
I suggest merging this as-is. The doctest improvement would be nice to have but it's not worth delaying the upgrade over it.
</p>
Ticketgh-timokauMon, 26 Nov 2018 21:29:54 GMT
https://trac.sagemath.org/ticket/26360#comment:25
https://trac.sagemath.org/ticket/26360#comment:25
<p>
@fredrik.johansson was that a review? I'm guessing @embray has different priorities right now, would be nice to get this into the next release.
</p>
TicketembrayTue, 27 Nov 2018 10:21:04 GMT
https://trac.sagemath.org/ticket/26360#comment:26
https://trac.sagemath.org/ticket/26360#comment:26
<p>
It's true I have different priorities. I'm pretty sad about this doctest issue but not enough to do anything about it <em>right now</em> so I don't think this should be held up over it. I'll open a ticket though.
</p>
TicketembrayTue, 27 Nov 2018 10:28:52 GMT
https://trac.sagemath.org/ticket/26360#comment:27
https://trac.sagemath.org/ticket/26360#comment:27
<p>
See <a class="new ticket" href="https://trac.sagemath.org/ticket/26774" title="enhancement: Support custom doctest parser hooks (perhaps module-specific) (new)">#26774</a> with an idea for a generalized solution.
</p>
Ticketgh-timokauTue, 27 Nov 2018 14:06:33 GMT
https://trac.sagemath.org/ticket/26360#comment:28
https://trac.sagemath.org/ticket/26360#comment:28
<p>
<a class="new ticket" href="https://trac.sagemath.org/ticket/26774" title="enhancement: Support custom doctest parser hooks (perhaps module-specific) (new)">#26774</a> would be really nice. In addition to arb I'm sure there are a lot of other areas where the doctests could be improved. That'll take a while however.
</p>
<p>
Lets get this merged for now.
</p>
TicketmmezzarobbaWed, 05 Dec 2018 12:45:09 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/26360#comment:29
https://trac.sagemath.org/ticket/26360#comment:29
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Marc Mezzarobba</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/26360#comment:28" title="Comment 28">gh-timokau</a>:
</p>
<blockquote class="citation">
<p>
<a class="new ticket" href="https://trac.sagemath.org/ticket/26774" title="enhancement: Support custom doctest parser hooks (perhaps module-specific) (new)">#26774</a> would be really nice. In addition to arb I'm sure there are a lot of other areas where the doctests could be improved. That'll take a while however.
</p>
<p>
Lets get this merged for now.
</p>
</blockquote>
<p>
I agree.
</p>
TicketvbraunFri, 07 Dec 2018 12:10:46 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/26360#comment:30
https://trac.sagemath.org/ticket/26360#comment:30
<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>branch</strong>
changed from <em>u/gh-timokau/arb-2.15.0</em> to <em>30cc778d46579bd0c7537ed33e8d7a4f40fd5c31</em>
</li>
</ul>
Ticket