Upgrade arb to 2.15.1 #26360

timokau · 2018-09-28T15:16:48Z

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 #25966, 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.

I suspect the solution here (lots of ...) 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 .mid() and .radius() in different tests each time. We may even want to test the .accuracy() instead of the radius.

Upstream tarball: https://github.com/fredrik-johansson/arb/archive/2.15.1.tar.gz

CC: @fredrik-johansson @embray @mezzarobba @jdemeyer @kiwifb

Component: packages: standard

Keywords: arb, upgrade

Author: Timo Kaufmann

Branch/Commit: 30cc778

Reviewer: Marc Mezzarobba

Issue created by migration from https://trac.sagemath.org/ticket/26360

The text was updated successfully, but these errors were encountered:

fredrik-johansson · 2018-09-28T15:25:42Z

comment:1

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.

mezzarobba · 2018-09-29T08:05:33Z

Replying to @timokau:

I suspect the solution here (lots of ...) will be controversial. I don't like it very much myself.

I'm okay with it, but I'd like to see what Jeroen says since he insisted for exact doctests back then.

What we really want are different tolerances for the "mid" and the "radius".

Is that really necessary? If you use absolute tolerances, having the same tolerance for the center and the radius isn't too bad.

Something like what Fredrik suggests would be better, of course. Since arb provides a way to parse strings of the form [mid +/- rad], it is perhaps doable. (I'm not volunteering, though...)

timokau · 2018-10-20T21:24:44Z

comment:4

Replying to @mezzarobba:

Replying to @timokau:

I suspect the solution here (lots of ...) will be controversial. I don't like it very much myself.

I'm okay with it, but I'd like to see what Jeroen says since he insisted for exact doctests back then.

Jeroen, do you still insist on exact doctests here?

timokau · 2018-10-20T21:25:25Z

comment:5

What we really want are different tolerances for the "mid" and the "radius".

Is that really necessary? If you use absolute tolerances, having the same tolerance for the center and the radius isn't too bad.

That might also a (while not pretty) workable solution.

Something like what Fredrik suggests would be better, of course. Since arb provides a way to parse strings of the form [mid +/- rad], it is perhaps doable. (I'm not volunteering, though...)

Is the doctesting framework even that flexible?

jdemeyer · 2018-10-21T08:30:59Z

comment:6

Replying to @timokau:

Jeroen, do you still insist on exact doctests here?

I don't recall why I originally insisted on that. So go ahead.

jdemeyer · 2018-10-21T08:40:34Z

comment:7

Replying to @mezzarobba:

Something like what Fredrik suggests would be better, of course. Since arb provides a way to parse strings of the form [mid +/- rad], it is perhaps doable. (I'm not volunteering, though...)

I'm not convinced that we need to patch the doctest framework for that.

timokau · 2018-10-23T21:34:11Z

comment:8

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:

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]

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]

fredrik-johansson · 2018-10-24T13:38:41Z

comment:9

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.

timokau · 2018-10-24T13:42:10Z

comment:10

it works if you add CBF("+/- 1e-10") to pi/2

So should we just do that in the doctests?

fredrik-johansson · 2018-10-24T20:38:47Z

comment:11

This commit should provide a bugfix: flintlib/arb@eb60d7a

The other failure ("unsupported string format") is due to too greedy search & replace to insert ...

timokau · 2018-10-24T21:26:59Z

comment:12

Do you plan to make a release with that fix included anytime soon?

fredrik-johansson · 2018-10-25T19:15:53Z

comment:13

Done. https://github.com/fredrik-johansson/arb/archive/2.15.1.tar.gz

timokau · 2018-10-26T16:19:57Z

comment:14

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 arb_set_str returning false.

fredrik-johansson · 2018-10-26T16:21:24Z

comment:15

There is "..." in the input string which shouldn't have been inserted there in place of the numerical value.

sagetrac-git · 2018-10-27T07:42:30Z

Changed commit from 00d3ccd to 30cc778

sagetrac-git · 2018-10-27T07:42:30Z

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

`30cc778`	`Upgrade arb to 2.15.1`

timokau · 2018-10-27T07:44:19Z

comment:17

Oh, stupid mistake. Thanks.

Doctests pass now.

embray · 2018-10-28T11:54:34Z

comment:18

I think rather than so many ..., 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...

timokau · 2018-10-28T12:35:39Z

comment:19

I wouldn't know how to do that.

embray · 2018-10-28T12:55:14Z

comment:20

I mean, I would, but I don't know exactly what it is you need in this case.

embray · 2018-10-28T12:59:02Z

comment:21

Replying to @timokau:

What we really want are different tolerances for the "mid" and the "radius".

Is that really necessary? If you use absolute tolerances, having the same tolerance for the center and the radius isn't too bad.

That might also a (while not pretty) workable solution.

Did you ever try this? I don't really see it as "pretty" or not "pretty". An # abs tol 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.

timokau · 2018-10-29T08:47:00Z

comment:22

Replying to @embray:

I mean, I would, but I don't know exactly what it is you need in this case.

We'd need to use proper comparison methods instead of just comparing the digits. Something like what Fredrik said.

Replying to @embray:

Replying to @timokau:

What we really want are different tolerances for the "mid" and the "radius".

Is that really necessary? If you use absolute tolerances, having the same tolerance for the center and the radius isn't too bad.

That might also a (while not pretty) workable solution.

Did you ever try this? I don't really see it as "pretty" or not "pretty". An # abs tol 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.

No I haven't. It would require either a lot of manual adding of # abs tol 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.

embray · 2018-10-31T13:21:22Z

comment:23

I'll give it a try and see. I'm pretty good at ridiculous regular expressions :)

fredrik-johansson · 2018-11-06T09:47:07Z

comment:24

I suggest merging this as-is. The doctest improvement would be nice to have but it's not worth delaying the upgrade over it.

timokau · 2018-11-26T21:29:54Z

comment:25

@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.

embray · 2018-11-27T10:21:04Z

comment:26

It's true I have different priorities. I'm pretty sad about this doctest issue but not enough to do anything about it right now so I don't think this should be held up over it. I'll open a ticket though.

embray · 2018-11-27T10:28:52Z

comment:27

See #26774 with an idea for a generalized solution.

timokau · 2018-11-27T14:06:33Z

comment:28

#26774 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.

Lets get this merged for now.

mezzarobba · 2018-12-05T12:45:09Z

comment:29

Replying to @timokau:

#26774 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.

Lets get this merged for now.

I agree.

mezzarobba · 2018-12-05T12:45:09Z

Reviewer: Marc Mezzarobba

vbraun · 2018-12-07T12:10:46Z

Changed branch from u/gh-timokau/arb-2.15.0 to 30cc778

timokau added this to the sage-8.4 milestone Sep 28, 2018

timokau added c: packages: standard labels Sep 28, 2018

This comment has been minimized.

Sign in to view

timokau changed the title ~~Upgrade arb to 2.15.0~~ Upgrade arb to 2.15.1 Oct 27, 2018

timokau added the s: needs review label Oct 27, 2018

mezzarobba added s: positive review and removed s: needs review labels Dec 5, 2018

vbraun removed the s: positive review label Dec 7, 2018

vbraun closed this as completed in 5205bbc Dec 7, 2018

embray mentioned this issue Apr 16, 2019

Support custom doctest parser hooks (perhaps module-specific) #26774

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade arb to 2.15.1 #26360

Upgrade arb to 2.15.1 #26360

timokau commented Sep 28, 2018

fredrik-johansson commented Sep 28, 2018

mezzarobba commented Sep 29, 2018

timokau commented Oct 20, 2018

timokau commented Oct 20, 2018

jdemeyer commented Oct 21, 2018

jdemeyer commented Oct 21, 2018

timokau commented Oct 23, 2018

fredrik-johansson commented Oct 24, 2018

timokau commented Oct 24, 2018

fredrik-johansson commented Oct 24, 2018

timokau commented Oct 24, 2018

fredrik-johansson commented Oct 25, 2018

timokau commented Oct 26, 2018

fredrik-johansson commented Oct 26, 2018

sagetrac-git mannequin commented Oct 27, 2018

sagetrac-git mannequin commented Oct 27, 2018

timokau commented Oct 27, 2018

This comment has been minimized.

embray commented Oct 28, 2018

timokau commented Oct 28, 2018

embray commented Oct 28, 2018

embray commented Oct 28, 2018

timokau commented Oct 29, 2018

embray commented Oct 31, 2018

fredrik-johansson commented Nov 6, 2018

timokau commented Nov 26, 2018

embray commented Nov 27, 2018

embray commented Nov 27, 2018

timokau commented Nov 27, 2018

mezzarobba commented Dec 5, 2018

mezzarobba commented Dec 5, 2018

vbraun commented Dec 7, 2018

Upgrade arb to 2.15.1 #26360

Upgrade arb to 2.15.1 #26360

Comments

timokau commented Sep 28, 2018

fredrik-johansson commented Sep 28, 2018

mezzarobba commented Sep 29, 2018

timokau commented Oct 20, 2018

timokau commented Oct 20, 2018

jdemeyer commented Oct 21, 2018

jdemeyer commented Oct 21, 2018

timokau commented Oct 23, 2018

fredrik-johansson commented Oct 24, 2018

timokau commented Oct 24, 2018

fredrik-johansson commented Oct 24, 2018

timokau commented Oct 24, 2018

fredrik-johansson commented Oct 25, 2018

timokau commented Oct 26, 2018

fredrik-johansson commented Oct 26, 2018

sagetrac-git mannequin commented Oct 27, 2018

sagetrac-git mannequin commented Oct 27, 2018

timokau commented Oct 27, 2018

This comment has been minimized.

embray commented Oct 28, 2018

timokau commented Oct 28, 2018

embray commented Oct 28, 2018

embray commented Oct 28, 2018

timokau commented Oct 29, 2018

embray commented Oct 31, 2018

fredrik-johansson commented Nov 6, 2018

timokau commented Nov 26, 2018

embray commented Nov 27, 2018

embray commented Nov 27, 2018

timokau commented Nov 27, 2018

mezzarobba commented Dec 5, 2018

mezzarobba commented Dec 5, 2018

vbraun commented Dec 7, 2018