{Real,Complex}Ball: Miscellaneous fixes and improvements

[mmezzarobba]

Mostly for compatibility with RIF/CIF.

[mmezzarobba]

Last 10 new commits:

​f9e13d6	ComplexBall: conversions to ZZ, RR, RIF, CC, CIF
​4a7b5c4	ComplexBallField: faster access to real field
​b4e82aa	{Real,Complex}Ball: implement conversions to QQ
​5e1cbda	ComplexBall: containment and overlapping predicates
​d44c1b1	ComplexBall: abs(), arg()
​62af563	RealBall: implement abs()
​448bf24	{Real,Complex}Ball: implement {above,below}_abs()
​0196f69	ComplexBallField: minor documentation improvements
​f2d093e	RealBall: upper(), lower(), endpoints()
​d0e948b	{Real,Complex}Ball: change __nonzero__ to check for syntactic !=0

[git]

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

​cfdffdd

{Real,Complex}Ball: contains_zero()

[git]

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

​a53da39

{Real,Complex}Ball: <<, >> (of a ball by an integer)

[git]

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

​63e4b76	{Real,Complex}Ball: add some tests
​0db149f	{Real,Complex}Ball: rename abs() to __abs__()
​1ee9919	RealBall: implement min(), max()
​fa2c0b7	{Real,Complex}Ball: __hash__()
​1741bce	{Real,Complex}Ball: bind arb's add_error
​feb8384	Implement ComplexBall.rad()
​a724343	real_arb: lone (and misplaced) sig_on()
​9e3b251	real_arb: work around weakness of arb_set_interval_mpfr
​f30820e	RealBall: union()
​50f4459	Implement CBF.complex_field()

[mmezzarobba]

Rebased and extended.

[git]

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

​560fe0b	{Real,Complex}Ball: add some tests
​a1d9086	{Real,Complex}Ball: rename abs() to __abs__()
​0a8efdd	RealBall: implement min(), max()
​8049aad	{Real,Complex}Ball: __hash__()
​e9700cb	{Real,Complex}Ball: bind arb's add_error
​761d53f	Implement ComplexBall.rad()
​ad628b8	real_arb: lone (and misplaced) sig_on()
​030652a	real_arb: work around weakness of arb_set_interval_mpfr
​3aa26c6	RealBall: union()
​9097cd4	Implement CBF.complex_field()

[git]

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

​7b67b5e	{Real,Complex}Ball: add some tests
​202f64c	{Real,Complex}Ball: rename abs() to __abs__()
​8f1ddaf	RealBall: implement min(), max()
​77f1174	{Real,Complex}Ball: __hash__()
​0d312da	{Real,Complex}Ball: bind arb's add_error
​11649d7	Implement ComplexBall.rad()
​638607e	real_arb: lone (and misplaced) sig_on()
​eaf70fa	real_arb: work around weakness of arb_set_interval_mpfr
​23c34f8	RealBall: union()
​c802d70	Implement CBF.complex_field()

[cheuberg]

As discussed in #17220, the experimental decorator should be removed.

[cheuberg]

I read the modifications and have the following comments.

1. arf.pxd: You define arf_rnd_t as an enum and rely on the fact that the numeric values of ARF_RND_DOWN etc. will never change in the C header files. However, there, the values are macros, so I do not know whether we can safely assume the values never to change.
2. polynomial_element.pyx:
   • I am very surprised to see this change here in an arb ticket.
   • The documentation of is_gen states: "Important - this function doesn’t return True if self equals the generator; it returns True if self is the generator." I.e., the outcome differs from the old behaviour.
3. power_series_ring_element.pyx: I am surprised to see this change here in an arb ticket, but I guess that it will not do any harm.
4. RealBall.__hash__: I do not like the fact that the fractional part of the midpoint is ignored completely:

   sage: hash(RBF(3/4))
   1048577
   sage: hash(RBF(5/8))
   1048577
   sage: hash(RBF(7/8))
   1048577
   

5. RealBall.upper, RealBall.lower, RealBall.endpoints: missing INPUT: and OUTPUT: sections, document and test parameter rnd
6. RealBall.add_error: missing INPUT: and OUTPUT: sections
7. RealBall.is_nonzero: The note section is hard to understand because __nonzero__ and is_zero should do very different things.
8. RealBall.__lshift__, RealBall.__rshift__: why is the first parameter called val instead of self and how can it happen that it is not a RealBall if this is a method of RealBall?
9. ComplexBall._is_atomic: self.is_real() or self.imag().is_zero() seems to be redundant. Do you want to say self.real().is_zero() instead?
10. ComplexBall.add_error: missing INPUT: and OUTPUT: sections
11. ComplexBall.is_nonzero: The note section is hard to understand because __nonzero__ and is_zero should do very different things.
12. ComplexBall.__lshift__, ComplexBall.__rshift__: why is the first parameter called val instead of self and how can it happen that it is not a ComplexBall if this is a method of ComplexBall?

---

New commits:

​a725299	Trac #19152: fix error in module description
​ada2759	Trac #19152: Fix hyperlinks in documentation
​32cc12a	Trac #19152: add SEEALSO blocks
​617b9a7	Trac #19152: fix typo and spacing
​59ab3cb	Trac #19152: fix _repr_option
​16a04a0	Trac #19152: fix docstring

[mmezzarobba]

Replying to cheuberg:

I read the modifications and have the following comments.

Thank you very much for your comments. I'm a bit busy with other things right now, but I'll try to have a look next Friday, if that's okay with you.

[mmezzarobba]

Replying to mmezzarobba:

Thank you very much for your comments. I'm a bit busy with other things right now, but I'll try to have a look next Friday, if that's okay with you.

I couldn't do much due to network problems, sorry. Hopefully Monday...

[jdemeyer]

I am having a quick look though this branch, mostly to check for technicalities. It's a lot, so I'm not saying that you really must fix all these for positive_review.

1. The naming of the files is inconsistent: why real_arb and complex_ball_acb?

2. You can remove the include_dirs=[SAGE_INC + '/flint']) from src/module_list.py.

3. Ideally, the type declarations in the arb .pxd files should be in a different file: try to follow the model of src/sage/libs/gmp for example. Then you should add # distutils: libraries = arb to the files with library functions (i.e. not types).

4. The __hash__ for RBF is bad:

   sage: from sage.rings.real_arb import RBF
   sage: hash(RBF(0.6))
   1048577
   sage: hash(RBF(0.7))
   1048577
   sage: hash(RBF(0.8))
   1048577
   sage: hash(RBF(0.9))
   1048577
   

5. For CBF.__hash__, what's the point of >> 7?

6. This is a very strange way to handle exceptions, what's wrong with raise?

           if arf_get_mpfr(rad.value, tmp, GMP_RNDN):
               abort()
   

7. Replace the GMP_RND macros by MPFR_RND.

8. You can replace

   cdef extern from "math.h":
       long lrint(double)
   

by from libc.math cimport lrint (although I would prefer to not use it in the hash).

9. What's with this comment?

                   # FIXME: RBF is not even associative, but CompletionFunctor
                   # only works with rings, and other coercion features require
                   # methods like is_integral_domain() to be defined
                   category=category or sage.categories.fields.Fields().Infinite())
   

Your class models a mathematical field (real or complex numbers), so I don't see the problem. How would you want to fix this?

10. Now that all this is standard, can you add RBF, RealBallField, CBF and ComplexBallField to the global namespace and then remove all doctest lines of the form from sage.rings.real_arb import RBF.

11. You should really avoid this: include "sage/ext/python.pxi", just cimport what you need.

12. What's the difference between contains_exact and __contains__? I don't understand why you need both these methods.

13. When raising exceptions, add a string: don't just raise TypeError, but raise TypeError("useful message here")

14. You write .. SEEALSO:: :meth:abs` but there is no such method.

15. I still don't understand why below_abs and above_abs return a ball instead of a real number. At the very least, this should be documentated in an OUTPUT: block in those methods.

16. This is confusing documentation: Return the right endpoint of this ball, viewed as an interval. I would read it like this method returns an interval. Why not Return the right endpoint of this ball.?

17. I don't understand the purpose of this comment: # TODO: once CBF is there, also wrap functions that only exist in acb*

18. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

[cheuberg]

Replying to jdemeyer:

1. The naming of the files is inconsistent: why real_arb and complex_ball_acb?

real_arb followed the pattern of real_mpfi; whereas complex_ball_acb somehow followed complex_interval. What names would you suggest?

[mmezzarobba]

Hello Clemens,

Thanks again for your comments and the changes you made.

Replying to cheuberg:

I read the modifications and have the following comments.

1. arf.pxd: You define arf_rnd_t as an enum and rely on the fact that the numeric values of ARF_RND_DOWN etc. will never change in the C header files. However, there, the values are macros, so I do not know whether we can safely assume the values never to change.

I fear I don't understand what you mean here, sorry. I thought that cython would insert textual references to ARF_RND_* in the generated C code. Isn't that the case?

2. polynomial_element.pyx:
   • I am very surprised to see this change here in an arb ticket.

I tend to consider that what matters is commits, not tickets. So I don't have any problem with that as long as it doesn't make the review harder, and I actually feel if it makes reviews easier... But please tell me if you don't like that and I will try to create separate tickets in the future.

• The documentation of is_gen states: "Important - this function doesn’t return True if self equals the generator; it returns True if self is the generator." I.e., the outcome differs from the old behaviour.

Good catch! I'd say it is okay, because the operation that really should be fast IMO is x^n where x = Pol.gen(), not (2*x - x)^n, and the new version seems to be about 10% faster when n == 2. But I'm open to suggestions.

4. RealBall.__hash__: I do not like the fact that the fractional part of the midpoint is ignored completely:

   sage: hash(RBF(3/4))
   1048577
   sage: hash(RBF(5/8))
   1048577
   sage: hash(RBF(7/8))
   1048577
   

Yes, I don't know what I was doing with that hash!

5. RealBall.upper, RealBall.lower, RealBall.endpoints: missing INPUT: and OUTPUT: sections, document and test parameter rnd

The problem here is that, unlike elements of RealIntervalField(p), elements of RealBallField(p) may have endpoints that are not be representable on p bits. I wasn't sure what to do in this case, so I implemented a quick wrapper for the interval version and then forgot to come back to it. (This is also the reason for the clumsy formulation “viewed as an interval” Jeroen asks about.)

The main options I see are:

• either to return the endpoints rounded (in which direction? we can't be 100% compatible with interval fields!) to the parent's precision,
• or return the exact endpoints, but then, the output parent will be hard to predict.

Which do you think is best?

6. RealBall.add_error: missing INPUT: and OUTPUT: sections

What would you suggest to say there?

7. RealBall.is_nonzero: The note section is hard to understand because __nonzero__ and is_zero should do very different things.

I'm not sure I understand your comment. As far as I understand (but all this stuff is very badly documented!), in the rest of Sage, __nonzero__() means “syntactically nonzero” (for example, the degree() methods of polynomials look for the leading __nonzero__ term), which for balls is exactly the negation of is_zero().

8. RealBall.__lshift__, RealBall.__rshift__: why is the first parameter called val instead of self and how can it happen that it is not a RealBall if this is a method of RealBall?

That's how special methods work in Cython... See ​http://docs.cython.org/src/userguide/special_methods.html#arithmetic-methods

9. ComplexBall._is_atomic: self.is_real() or self.imag().is_zero() seems to be redundant. Do you want to say self.real().is_zero() instead?

I think so, yes, thanks! But actually I don't understand what _is_atomic() really should do: for example, the documentation seems to suggest that -2 should not be atomic (since x + -2 is not correct), while it is atomic in various standard rings, and I think it has to in order to avoid redundant parentheses...

[mmezzarobba]

Hello Jeroen,

Thank you very much for your remarks.

Replying to jdemeyer:

2. You can remove the include_dirs=[SAGE_INC + '/flint']) from src/module_list.py.

Are you sure? I get:

[2/2] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/marc/co/sage/local/include -I/home/marc/co/sage/local/include/python2.7 -I/home/marc/co/sage/local/lib/python2.7/site-packages/numpy/core/include -I/home/marc/co/sage/src -I/home/marc/co/sage/src/sage/ext -I/home/marc/co/sage/src/build/cythonized -I/home/marc/co/sage/src/build/cythonized/sage/ext -I/home/marc/co/sage/local/include/python2.7 -c /home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.c -o build/temp.linux-x86_64-2.7/home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.o -fno-strict-aliasing -w
In file included from /home/marc/co/sage/src/build/cythonized/sage/rings/complex_ball_acb.c:310:0:
/home/marc/co/sage/local/include/mag.h:36:19: fatal error: flint.h: No such file or directory
compilation terminated.
In file included from /home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.c:309:0:
/home/marc/co/sage/local/include/mag.h:36:19: fatal error: flint.h: No such file or directory

3. Ideally, the type declarations in the arb .pxd files should be in a different file: try to follow the model of src/sage/libs/gmp for example. Then you should add # distutils: libraries = arb to the files with library functions (i.e. not types).

A difference is that arb comes with several .h files (one for each module, with a more or less well-defined dependency graph): are you suggesting to put all the type definitions in a single .pxd file nevertheless?

4. The __hash__ for RBF is bad:

   sage: from sage.rings.real_arb import RBF
   sage: hash(RBF(0.6))
   1048577
   sage: hash(RBF(0.7))
   1048577
   sage: hash(RBF(0.8))
   1048577
   sage: hash(RBF(0.9))
   1048577
   

Fixed (see above).

5. For CBF.__hash__, what's the point of >> 7?

The idea was just to avoid that the λ(1+i) all hash to zero...

6. This is a very strange way to handle exceptions, what's wrong with raise?

           if arf_get_mpfr(rad.value, tmp, GMP_RNDN):
               abort()
   

I don't remember for sure. I guess I may have wanted to make sure that errors raised by arf_get_mpfr() itself and errors raised manually give the same exception, and to avoid repeating the error message.

7. Replace the GMP_RND macros by MPFR_RND.

Done.

9. What's with this comment?

                   # FIXME: RBF is not even associative, but CompletionFunctor
                   # only works with rings, and other coercion features require
                   # methods like is_integral_domain() to be defined
                   category=category or sage.categories.fields.Fields().Infinite())
   

Your class models a mathematical field (real or complex numbers), so I don't see the problem. How would you want to fix this?

Well, people seem to disagree about the meaning of categories in Sage, and some people apparently assume that a parent that belongs to the category of fields will satisfy the axioms of fields, not just vaguely model a field. This is particularly harmful in the context of interval arithmetic.

In the long term, I'd want to fix this by (i) clarifying the “global conventions” that need to be used consistently across the whole sage codebase and (ii) depending on the outcome, perhaps creating categories for “inexact fields” and the like.

10. Now that all this is standard, can you add RBF, RealBallField, CBF and ComplexBallField to the global namespace and then remove all doctest lines of the form from sage.rings.real_arb import RBF.

I'm no fan of adding anything to the global namespace, but I'll do it if you insist.

11. You should really avoid this: include "sage/ext/python.pxi", just cimport what you need.

Done.

12. What's the difference between contains_exact and __contains__? I don't understand why you need both these methods.

The difference is that contains_exact() works for fewer kinds of objects, but is guaranteed not to return false negatives, while __contains__() just does the check at the parent's precision. Of course, one could do with __contains__() alone, but then it would be too easy to call x in b by accident with an x for which it may not return the correct result.

13. When raising exceptions, add a string: don't just raise TypeError, but raise TypeError("useful message here")

Done. (But I didn't include a message because I couldn't think of a useful message, so I doubt the new version is much better...)

14. You write .. SEEALSO:: :meth:abs` but there is no such method.

Indeed, I forgot to remove these references, thanks!

15. I still don't understand why below_abs and above_abs return a ball instead of a real number. At the very least, this should be documentated in an OUTPUT: block in those methods.

Why: because I often compute upper or lower bounds on quantities represented by balls and then do arithmetic operations on the bounds that may result in rounding errors. Representing the bounds themselves as thin intervals is pretty convenient then. I also find that choice more consistent with the rest of the interface of real/complex balls...

I added OUPUT blocks, please tell me if there is anything else you feel these blocks should say.

16. This is confusing documentation: Return the right endpoint of this ball, viewed as an interval. I would read it like this method returns an interval. Why not Return the right endpoint of this ball.?

Indeed! See my answer to Clemens above.

17. I don't understand the purpose of this comment: # TODO: once CBF is there, also wrap functions that only exist in acb*

There are special functions in arb that are only implemented for complex arguments, but that would make sense on sage real balls.

18. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

An arb ball has no intrinsic precision, and (more or less because of that), an element of RealBallField(p) can have a midpoint that doesn't fit on p bits. The round() method rounds such elements to the precision of their parent.

[mmezzarobba]

New commits:

​07e365c	RealBall: more robust __hash__
​f0f4d6c	ComplexBall: fix _is_atomic()
​d17f6da	arb interface: use MPFR_RND? instead of GMP_RND?
​31e3b4b	arb interface: avoid including python.pxi
​ea08942	{Real,Complex}Ball.*abs: add error message, fix crossrefs

[jdemeyer]

Replying to cheuberg:

real_arb followed the pattern of real_mpfi; whereas complex_ball_acb somehow followed complex_interval.

I also find these names confusing, so I don't think it's a model to follow.

What names would you suggest?

Why not simply real_arb and complex_arb? At least something more consistent.

[jdemeyer]

Replying to mmezzarobba:

Replying to cheuberg:

1. arf.pxd: You define arf_rnd_t as an enum and rely on the fact that the numeric values of ARF_RND_DOWN etc. will never change in the C header files. However, there, the values are macros, so I do not know whether we can safely assume the values never to change.

I fear I don't understand what you mean here, sorry. I thought that cython would insert textual references to ARF_RND_* in the generated C code. Isn't that the case?

I agree, there is no issue here. Cython doesn't care about the values.

5. RealBall.upper, RealBall.lower, RealBall.endpoints: missing INPUT: and OUTPUT: sections, document and test parameter rnd

The main options I see are:

• either to return the endpoints rounded (in which direction? we can't be 100% compatible with interval fields!) to the parent's precision,

I would go for: rounding endpoints in the obvious direction (i.e. up for the upper endpoint and down for the lower endpoint)

[jdemeyer]

Replying to mmezzarobba:

Replying to jdemeyer:

2. You can remove the include_dirs=[SAGE_INC + '/flint']) from src/module_list.py.

Are you sure? I get:

[2/2] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/marc/co/sage/local/include -I/home/marc/co/sage/local/include/python2.7 -I/home/marc/co/sage/local/lib/python2.7/site-packages/numpy/core/include -I/home/marc/co/sage/src -I/home/marc/co/sage/src/sage/ext -I/home/marc/co/sage/src/build/cythonized -I/home/marc/co/sage/src/build/cythonized/sage/ext -I/home/marc/co/sage/local/include/python2.7 -c /home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.c -o build/temp.linux-x86_64-2.7/home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.o -fno-strict-aliasing -w
In file included from /home/marc/co/sage/src/build/cythonized/sage/rings/complex_ball_acb.c:310:0:
/home/marc/co/sage/local/include/mag.h:36:19: fatal error: flint.h: No such file or directory
compilation terminated.
In file included from /home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.c:309:0:
/home/marc/co/sage/local/include/mag.h:36:19: fatal error: flint.h: No such file or directory

I must admit I didn't test it, but it seems the problem comes from arb (not the Sage library). I'll look into it later.

3. Ideally, the type declarations in the arb .pxd files should be in a different file: try to follow the model of src/sage/libs/gmp for example. Then you should add # distutils: libraries = arb to the files with library functions (i.e. not types).

A difference is that arb comes with several .h files (one for each module, with a more or less well-defined dependency graph): are you suggesting to put all the type definitions in a single .pxd file nevertheless?

Yes, put the type declarations all in types.pxd.

5. For CBF.__hash__, what's the point of >> 7?

The idea was just to avoid that the λ(1+i) all hash to zero...

??? I don't get it.

6. This is a very strange way to handle exceptions, what's wrong with raise?

           if arf_get_mpfr(rad.value, tmp, GMP_RNDN):
               abort()
   

I don't remember for sure. I guess I may have wanted to make sure that errors raised by arf_get_mpfr() itself and errors raised manually give the same exception, and to avoid repeating the error message.

You can use sig_error() for that, it just calls abort() but it's more explicit.

9. What's with this comment?

                   # FIXME: RBF is not even associative, but CompletionFunctor
                   # only works with rings, and other coercion features require
                   # methods like is_integral_domain() to be defined
                   category=category or sage.categories.fields.Fields().Infinite())
   

Your class models a mathematical field (real or complex numbers), so I don't see the problem. How would you want to fix this?

Well, people seem to disagree about the meaning of categories in Sage, and some people apparently assume that a parent that belongs to the category of fields will satisfy the axioms of fields, not just vaguely model a field. This is particularly harmful in the context of interval arithmetic.

In the long term, I'd want to fix this by (i) clarifying the “global conventions” that need to be used consistently across the whole sage codebase and (ii) depending on the outcome, perhaps creating categories for “inexact fields” and the like.

But that's not at all specific to arb right, so I'm just confused by this comment. I propose to remove it.

10. Now that all this is standard, can you add RBF, RealBallField, CBF and ComplexBallField to the global namespace and then remove all doctest lines of the form from sage.rings.real_arb import RBF.

I'm no fan of adding anything to the global namespace, but I'll do it if you insist.

Why not?

18. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

An arb ball has no intrinsic precision, and (more or less because of that), an element of RealBallField(p) can have a midpoint that doesn't fit on p bits. The round() method rounds such elements to the precision of their parent.

Now I'm even more confused... I still have no clue what you mean.

[jdemeyer]

Replying to mmezzarobba:

17. I don't understand the purpose of this comment: # TODO: once CBF is there, also wrap functions that only exist in acb*

There are special functions in arb that are only implemented for complex arguments, but that would make sense on sage real balls.

18. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

An arb ball has no intrinsic precision, and (more or less because of that), an element of RealBallField(p) can have a midpoint that doesn't fit on p bits. The round() method rounds such elements to the precision of their parent.

When asking these questions above, I'm not just asking for myself personally. It means that the code on the branch should be clarified.

[jdemeyer]

I consider the fact that include_dirs is needed an upstream arb bug. So you can leave it for now. See #19563.

[mmezzarobba]

Replying to jdemeyer:

Why not simply real_arb and complex_arb? At least something more consistent.

I agree, I already considered making that change several times. Actually, at some point, I'd like to reorganize the code of variants of real and complex numbers in order to make them more consistent with each other and reduce code duplication a little, and I think it would make sense to move them all to a new package sage.rings.numerical (or similar) then.

[mmezzarobba]

Replying to jdemeyer:

I would go for: rounding endpoints in the obvious direction (i.e. up for the upper endpoint and down for the lower endpoint)

Fine with me. The rnd argument will be a bit confusing, but I don't think that's too bad.

[mmezzarobba]

Replying to jdemeyer:

A difference is that arb comes with several .h files (one for each module, with a more or less well-defined dependency graph): are you suggesting to put all the type definitions in a single .pxd file nevertheless?

Yes, put the type declarations all in types.pxd.

Done.

5. For CBF.__hash__, what's the point of >> 7?

The idea was just to avoid that the λ(1+i) all hash to zero...

??? I don't get it.

I'm sorry, I don't understand what you don't understand. (You are talking about ComplexBall.__hash__(), not ComplexBallField.__hash__(), right?)

You can use sig_error() for that, it just calls abort() but it's more explicit.

Changed (though to me this is “strange sage-specific stuff” vs “standard C/POSIX”, so less explicit...)

But that's not at all specific to arb right, so I'm just confused by this comment. I propose to remove it.

Fine.

10. Now that all this is standard, can you add RBF, RealBallField, CBF and ComplexBallField to the global namespace and then remove all doctest lines of the form from sage.rings.real_arb import RBF.

I'm no fan of adding anything to the global namespace, but I'll do it if you insist.

Why not?

Because it doesn't make any sense to me to import tons of specialized stuff on startup. It is so much better to import the things you use in your personal init.sage... (And sage -min, which would mitigate the problem, hasn't worked for years afaik.)

18. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

An arb ball has no intrinsic precision, and (more or less because of that), an element of RealBallField(p) can have a midpoint that doesn't fit on p bits. The round() method rounds such elements to the precision of their parent.

Now I'm even more confused... I still have no clue what you mean.

I added some documentation, please tell me if it is clear enough.

[git]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

​49b79f3	rename complex_ball_acb to complex_arb, 1/2
​361ae39	rename complex_ball_acb to complex_arb, 2/2
​88da8de	libs.arb: reorganize
​ab117da	complex_ball: abort() --> sig_error()
​1ee8c16	ref manual: rm reference to arb being optional
​865e6d5	{real,complex}_arb: more doc on precision issues
​d289538	{real,complex}_arb: move SEEALSO blocks after EXAMPLES
​fe1b071	RealBall: clarify doc of upper(), lower(), endpoints()
​a38574a	complex_arb is no longer experimental
​d5af324	{real,complex}_arb: minor doc fixes

[jdemeyer]

Replying to mmezzarobba:

Because it doesn't make any sense to me to import tons of specialized stuff on startup.

We have lazy_import for that.

[jdemeyer]

Replying to mmezzarobba:

I'm sorry, I don't understand what you don't understand. (You are talking about ComplexBall.__hash__(), not ComplexBallField.__hash__(), right?)

I now understand what you mean. I would prefer something like

hash(real_part) + 3*hash(complex_part)

then. By doing >> 7, you're throwing away 7 bits for no reason.

[mmezzarobba]

Replying to jdemeyer:

Replying to mmezzarobba:

Because it doesn't make any sense to me to import tons of specialized stuff on startup.

We have lazy_import for that.

I'm not talking only about the cost of the imports, but mainly about the namespace pollution.

[git]

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

​f43e94a

RealBall: minor change to __hash__

[jdemeyer]

In __hash__, you are still throwing away 1.58 bits, but it's an improvement :-)

The rnd argument will be a bit confusing, but I don't think that's too bad.

I don't mind deprecating the rnd argument, I don't think it's useful anyway.

[jdemeyer]

I was reading the real_arb documentation and this caught my eye:

Warning

Identical RealBall? objects are understood to give permission for algebraic simplification. This assumption is made to improve performance. For example, setting z = x*x sets z to a ball enclosing the set {t2:t∈x} and not the (generally larger) set {tu:t∈x,u∈x}.

I'm not sure that we should do that. It's not what intervals do: I would expect x*x to be different from x^2.

[jdemeyer]

I consider this a bug:

It is possible to construct a ball whose parent is the real ball field with precision p but whose midpoint does not fit on p bits. However, the results of operations involving such a ball will (usually) be rounded to its parent’s precision.

It causes too much confusion for example with the round() function. It's also too different from all other places in Sage which handle precision.

[jdemeyer]

This is a good reason to round when creating a ball:

sage: a = RealBallField(20)(1.3)
sage: b = RealBallField(53)(1.3)
sage: a == b
True
sage: a+0 == b+0
False

[jdemeyer]

Despite what the documentation says, we still have

sage: a = RealBallField(53)(RIF(-1,1))
sage: (a*(a+0)).identical(a^2)
True

[git]

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

​7e6b5d0

real_arb: minor doc fix

[mmezzarobba]

Replying to jdemeyer:

Identical RealBall? objects are understood to give permission for algebraic simplification. This assumption is made to improve performance. For example, setting z = x*x sets z to a ball enclosing the set {t2:t∈x} and not the (generally larger) set {tu:t∈x,u∈x}.

I'm not sure that we should do that. It's not what intervals do: I would expect x*x to be different from x^2.

Replying to jdemeyer:

I consider this a bug:

It is possible to construct a ball whose parent is the real ball field with precision p but whose midpoint does not fit on p bits. However, the results of operations involving such a ball will (usually) be rounded to its parent’s precision.

It causes too much confusion for example with the round() function. It's also too different from all other places in Sage which handle precision.

Both these design choices were discussed on previous arb-related tickets, in particular #17194. As far as I'm concerend, I'm happy with the way things currently work (even though I was a little bit skeptical at first), and I'm not interested in working on changing it. Incidentally, I'm not sure this ticket would be the right place.

[jdemeyer]

Even though I don't agree with everything in this branch, I'm willing to give it positive_review.

[jdemeyer]

Follow-ups: #19563 and #19568.

[cheuberg]

Replying to mmezzarobba:

Replying to jdemeyer:

Identical RealBall? objects are understood to give permission for algebraic simplification. This assumption is made to improve performance. For example, setting z = x*x sets z to a ball enclosing the set {t2:t∈x} and not the (generally larger) set {tu:t∈x,u∈x}.

I'm not sure that we should do that. It's not what intervals do: I would expect x*x to be different from x^2.

Both these design choices were discussed on previous arb-related tickets, in particular #17194. As far as I'm concerend, I'm happy with the way things currently work (even though I was a little bit skeptical at first), and I'm not interested in working on changing it. Incidentally, I'm not sure this ticket would be the right place.

In particular, the first issue is a property of arb, see its documentation. Therefore, working around that might have involved that we make copies ourselves.

What is now the status of this ticket? Is it still needs_work, in particular after Jeroens remark earlier today?

[jdemeyer]

Replying to cheuberg:

In particular, the first issue is a property of arb, see its documentation.

If that is the case, this absolutely should be mentioned in the Sage documentation for arb.

[jdemeyer]

I really would prefer to have a positive_review to this ticket together with a positive_review for #19568. Can somebody please (re)consider reviewing #19568?

[cheuberg]

Replying to jdemeyer:

Replying to cheuberg:

In particular, the first issue is a property of arb, see its documentation.

If that is the case, this absolutely should be mentioned in the Sage documentation for arb.

I do not understand what you mean.

​Arb's documentation says that "An important caveat applies to aliasing of input variables. Identical pointers are understood to give permission for algebraic simplification."

The ​documentation of the real_arb class has a "Warning" box stating "Identical RealBall objects are understood to give permission for algebraic simplification. This assumption is made to improve performance."

[jdemeyer]

I meant that the documentation should state this is done by the arb library, not the Sage interface to arb.

[git]

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

​9b9cfb8	Trac #19152: reformat OUTPUT section and remove typo
​1f8d390	Trac #19152: add OUTPUT blocks
​cc74b61	Trac #19152: fix one link and add SEEALSO

[cheuberg]

Replying to mmezzarobba:

Replying to cheuberg:

1. arf.pxd: You define arf_rnd_t as an enum and rely on the fact that the numeric values of ARF_RND_DOWN etc. will never change in the C header files. However, there, the values are macros, so I do not know whether we can safely assume the values never to change.

I fear I don't understand what you mean here, sorry. I thought that cython would insert textual references to ARF_RND_* in the generated C code. Isn't that the case?

ok. Forget it.

2. polynomial_element.pyx:
   • I am very surprised to see this change here in an arb ticket.

I tend to consider that what matters is commits, not tickets. So I don't have any problem with that as long as it doesn't make the review harder, and I actually feel if it makes reviews easier... But please tell me if you don't like that and I will try to create separate tickets in the future.

It makes reviewing harder as I do not really want to interfere with the polynomials guys ...

• The documentation of is_gen states: "Important - this function doesn’t return True if self equals the generator; it returns True if self is the generator." I.e., the outcome differs from the old behaviour.

Good catch! I'd say it is okay, because the operation that really should be fast IMO is x^n where x = Pol.gen(), not (2*x - x)^n, and the new version seems to be about 10% faster when n == 2. But I'm open to suggestions.

Well, I would have preferred to have somebody else to decide about that;

5. RealBall.upper, RealBall.lower, RealBall.endpoints: missing INPUT: and OUTPUT: sections, document and test parameter rnd

The problem here is that, unlike elements of RealIntervalField(p), elements of RealBallField(p) may have endpoints that are not be representable on p bits. I wasn't sure what to do in this case, so I implemented a quick wrapper for the interval version and then forgot to come back to it. (This is also the reason for the clumsy formulation “viewed as an interval” Jeroen asks about.)

The main options I see are:

• either to return the endpoints rounded (in which direction? we can't be 100% compatible with interval fields!) to the parent's precision,
• or return the exact endpoints, but then, the output parent will be hard to predict.

Which do you think is best?

current behaviour is fine for me.

6. RealBall.add_error: missing INPUT: and OUTPUT: sections

What would you suggest to say there?

I'd like to know what ampl shall be (any rational number? ball?) and that I'd prefer to state that OUTPUT:

A complex ball.

(the description seems to be worded as an inline operation, but it actually returns a new ball).

7. RealBall.is_nonzero: The note section is hard to understand because __nonzero__ and is_zero should do very different things.

I'm not sure I understand your comment. As far as I understand (but all this stuff is very badly documented!), in the rest of Sage, __nonzero__() means “syntactically nonzero” (for example, the degree() methods of polynomials look for the leading __nonzero__ term), which for balls is exactly the negation of is_zero().

I'd prefer something which separates the two messages of the one sentence, e.g. "Use __nonzero__() to check that a ball is not exactly the zero ball (for instance, to determine the “degree” of a polynomial with ball coefficients). In fact, __nonzero__() is the negation of is_zero(), whereas is_nonzero() only returns True if 0 is known not to be in the ball."

Further remark:

19. The AUTHORS section at the beginning of the files is not accurate: I started those files, but most of their contents has been written by you. So please either add your name or remove mine.

[cheuberg]

Replying to mmezzarobba:

Replying to jdemeyer:

Replying to mmezzarobba:

Because it doesn't make any sense to me to import tons of specialized stuff on startup.

We have lazy_import for that.

I'm not talking only about the cost of the imports, but mainly about the namespace pollution.

I would not mind having RBF, CBF, RealBallField and ComplexBallField in the global name space; this is how most of sage seems to work. But this could also happen in another ticket ...

[git]

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

​f51cd86	{Real,Complex}Ball: better document add_error()
​49a2e9b	{Real,Complex}Ball: improve doc of is_nonzero()
​154b65d	{real,complex}_arb: rm AUTHORS blocks
​e22f407	add RealBallField & friends to the global namespace
​c8873f0	real_ball: one more instance of [non]zero -> True/False

[mmezzarobba]

Replying to cheuberg:

It makes reviewing harder as I do not really want to interfere with the polynomials guys ...

Ok, sorry, I'll try to avoid doing that in tickets you are likely to review.

I'd like to know what ampl shall be (any rational number? ball?) and that I'd prefer to state that OUTPUT:

A complex ball.

(the description seems to be worded as an inline operation, but it actually returns a new ball).

Done.

I'd prefer something which separates the two messages of the one sentence, e.g. "Use __nonzero__() to check that a ball is not exactly the zero ball (for instance, to determine the “degree” of a polynomial with ball coefficients). In fact, __nonzero__() is the negation of is_zero(), whereas is_nonzero() only returns True if 0 is known not to be in the ball."

Done.

Further remark:

19. The AUTHORS section at the beginning of the files is not accurate: I started those files, but most of their contents has been written by you. So please either add your name or remove mine.

I'll be consistent with what I said on sage-devel a few weeks ago and simply remove the AUTHORS block :-). But of course please feel free to add us both back if you prefer.

Replying to cheuberg:

I would not mind having RBF, CBF, RealBallField? and ComplexBallField? in the global name space; this is how most of sage seems to work. But this could also happen in another ticket ...

Done — I still don't like that very much, but since everyone seems to agree...

[git]

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

​7920b9f

{real,complex}_arb: clarify that b == b in arb too

[mmezzarobba]

Oops, merge conflict will the last develop. I will overwrite the last few commits in a minute to avoid the conflict.

[git]

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

​dfccd23	e22f4074539b1833befb7f24bd700a7fe11589a
​73a928a	real_ball: one more instance of [non]zero -> True/False
​3749cbb	{real,complex}_arb: clarify that b == b in arb too

[git]

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

​5334c56	Trac #19152: minor ReSt formatting
​0a04e62	Trac #19152: delete empty "TESTS" section

[cheuberg]

Replying to mmezzarobba:

Replying to cheuberg:

I would not mind having RBF, CBF, RealBallField? and ComplexBallField? in the global name space; this is how most of sage seems to work. But this could also happen in another ticket ...

Done — I still don't like that very much, but since everyone seems to agree...

Is there a particular reason for not using lazy_import?

Apart from that question, all my concerns have been dealt with and as far as I can see, Jeroen's comments are also all taken into account.

I think that #19568 is not too related to the changes in this ticket (RealBallField was never marked as experimental, as that decorator did not yet exist when it was introduced).

Therefore, I intend to set this ticket to positive as soon as the question about lazy imports is settled.

[jdemeyer]

Replying to cheuberg:

I think that #19568 is not too related to the changes in this ticket

It's not related but it would be nice to fix #19568 before too many people start using RBF.

[mmezzarobba]

Replying to cheuberg:

Is there a particular reason for not using lazy_import?

Yes: I'm not sure that the modules are expensive enough to be worth importing with lazy_import, and it causes doctest failures that I didn't have the time or motivation to investigate...

[jdemeyer]

Replying to mmezzarobba:

it causes doctest failures that I didn't have the time or motivation to investigate...

Can you at least push your attempt such that it can be investigated?

[mmezzarobba]

Replying to jdemeyer:

Replying to mmezzarobba:

it causes doctest failures that I didn't have the time or motivation to investigate...

Can you at least push your attempt such that it can be investigated?

The following change:

diff --git a/src/sage/rings/all.py b/src/sage/rings/all.py
index b5d4db7..33f18a1 100644
--- a/src/sage/rings/all.py
+++ b/src/sage/rings/all.py
@@ -83,7 +83,8 @@ from real_double import RealDoubleField, RDF, RealDoubleElement
 
 from real_lazy import RealLazyField, RLF, ComplexLazyField, CLF
 
-from sage.rings.real_arb import RealBallField, RBF
+lazy_import("sage.rings.real_arb", "RealBallField")
+lazy_import("sage.rings.real_arb", "RBF")
 
 # Polynomial Rings and Polynomial Quotient Rings
 from polynomial.all import *

leads to:

$ ./sage -t src/sage/rings/real_arb.pyx src/sage/rings/complex_arb.pyx
too many failed tests, not using stored timings
Running doctests with ID 2015-11-16-16-34-41-63268156.
Git branch: arb-misc
Using --optional=mpir,python2,sage,scons
Doctesting 2 files.
sage -t src/sage/rings/real_arb.pyx
**********************************************************************
File "src/sage/rings/real_arb.pyx", line 314, in sage.rings.real_arb.RealBallField
Failed example:
    (1/2*RBF(1)) + AA(sqrt(2)) - 1 + polygen(QQ, x)
Exception raised:
    Traceback (most recent call last):
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.real_arb.RealBallField[2]>", line 1, in <module>
        (Integer(1)/Integer(2)*RBF(Integer(1))) + AA(sqrt(Integer(2))) - Integer(1) + polygen(QQ, x)
      File "sage/structure/element.pyx", line 1702, in sage.structure.element.RingElement.__add__ (build/cythonized/sage/structure/element.c:16575)
        return coercion_model.bin_op(left, right, add)
      File "sage/structure/coerce.pyx", line 1070, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:9739)
        raise TypeError(arith_error_message(x,y,op))
    TypeError: unsupported operand parent(s) for '+': 'Real ball field with 53 bits precision' and 'Univariate Polynomial Ring in x over Rational Field'
**********************************************************************
1 item had failures:
   1 of  14 in sage.rings.real_arb.RealBallField
    [454 tests, 1 failure, 0.23 s]
sage -t src/sage/rings/complex_arb.pyx
    [253 tests, 0.25 s]
----------------------------------------------------------------------
sage -t src/sage/rings/real_arb.pyx  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.7 seconds
    cpu time: 0.5 seconds
    cumulative wall time: 0.5 seconds
(exit 1) 

[git]

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

​b21aa6e

Trac #19152: lazy_import

[cheuberg]

Replying to mmezzarobba:

Replying to jdemeyer:

Replying to mmezzarobba: Can you at least push your attempt such that it can be investigated?

The following change: ... leads to: ...

    TypeError: unsupported operand parent(s) for '+': 'Real ball field with 53 bits precision' and 'Univariate Polynomial Ring in x over Rational Field'

I cannot reproduce this. I used your diff and had no problems with make ptestlong, neither on 6.10.beta0 where this branch is based on nor after merging with 6.10.beta5. Tested only on 64-bit linux though.

Let's see what the patchbots say.

[git]

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

[cheuberg]

So the patchbot could reproduce the problem (and in the meantime, I could also reproduce it). Thus I reset the branch to the earlier commit.

The commit with the lazy import is still available as branch u/cheuberg/19152-arb-misc-lazy-import if somebody wants to debug this.

My intention now is to set this ticket to positive review as it is in the next few days unless somebody objects.

[vbraun]

Fine with me; There seems to be something fishy with lazy import and coercion. Can you open a new ticket and push your lazy-import branch there?

[cheuberg]

Replying to vbraun:

Can you open a new ticket and push your lazy-import branch there?

This is now #19628.

Thus it seems time to set this to positive_review.

[mmezzarobba]

I just noticed a stupid bug in the new __hash__ (07e365c509fbbd6e54da9e172f1845f6d24971c9): I forgot to fmpz_init some variables. Would you prefer me to fix it here or to open a new ticket?

[cheuberg]

It's safer to do it in a new ticket (The release manager might already have pulled this ticket for inclusion).

[mmezzarobba]

Replying to cheuberg:

It's safer to do it in a new ticket (The release manager might already have pulled this ticket for inclusion).

Done in #19629.