Opened 4 years ago
Closed 4 years ago
#19152 closed enhancement (fixed)
{Real,Complex}Ball: Miscellaneous fixes and improvements
Reported by:  mmezzarobba  Owned by:  

Priority:  major  Milestone:  sage6.10 
Component:  numerical  Keywords:  
Cc:  cheuberg  Merged in:  
Authors:  Marc Mezzarobba  Reviewers:  Clemens Heuberger 
Report Upstream:  N/A  Work issues:  
Branch:  0a04e62 (Commits)  Commit:  0a04e627728c37d334c3576eb5243facebee0e80 
Dependencies:  #19063  Stopgaps: 
Description (last modified by )
Mostly for compatibility with RIF/CIF.
Change History (75)
comment:1 Changed 4 years ago by
 Branch set to u/mmezzarobba/19152arbmisc
 Commit set to d0e948b8e4c66eed2226178ffa1b0f1b4957f40c
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Dependencies set to #19063
comment:3 Changed 4 years ago by
 Commit changed from d0e948b8e4c66eed2226178ffa1b0f1b4957f40c to cfdffddc92d64218a2766b7c6c86117190af7e68
Branch pushed to git repo; I updated commit sha1. New commits:
cfdffdd  {Real,Complex}Ball: contains_zero()

comment:4 Changed 4 years ago by
 Commit changed from cfdffddc92d64218a2766b7c6c86117190af7e68 to a53da3999a233cf41b11f7e2bdeae96bd8ca7ced
Branch pushed to git repo; I updated commit sha1. New commits:
a53da39  {Real,Complex}Ball: <<, >> (of a ball by an integer)

comment:5 Changed 4 years ago by
 Description modified (diff)
comment:6 Changed 4 years ago by
 Commit changed from a53da3999a233cf41b11f7e2bdeae96bd8ca7ced to 50f4459b65e51e2539347fde5153863f1d1ef178
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()

comment:7 Changed 4 years ago by
Rebased and extended.
comment:8 Changed 4 years ago by
 Commit changed from 50f4459b65e51e2539347fde5153863f1d1ef178 to 9097cd41160736353232c64351c010ef371c2d23
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()

comment:9 Changed 4 years ago by
 Commit changed from 9097cd41160736353232c64351c010ef371c2d23 to c802d70f17a70aff9f5600ae2d3a0f035eeafdd3
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()

comment:10 Changed 4 years ago by
 Milestone changed from sage6.9 to sage6.10
As discussed in #17220, the experimental
decorator should be removed.
comment:11 Changed 4 years ago by
 Branch changed from u/mmezzarobba/19152arbmisc to u/cheuberg/19152arbmisc
comment:12 followups: ↓ 13 ↓ 17 Changed 4 years ago by
 Commit changed from c802d70f17a70aff9f5600ae2d3a0f035eeafdd3 to 16a04a07c63349d3792ed8c538a7d60e5adfde5e
 Reviewers set to Clemens Heuberger
 Status changed from needs_review to needs_work
I read the modifications and have the following comments.
arf.pxd
: You definearf_rnd_t
as an enum and rely on the fact that the numeric values ofARF_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.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.
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.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
RealBall.upper
,RealBall.lower
,RealBall.endpoints
: missing INPUT: and OUTPUT: sections, document and test parameterrnd
RealBall.add_error
: missing INPUT: and OUTPUT: sectionsRealBall.is_nonzero
: The note section is hard to understand because__nonzero__
andis_zero
should do very different things.RealBall.__lshift__
,RealBall.__rshift__
: why is the first parameter calledval
instead ofself
and how can it happen that it is not aRealBall
if this is a method ofRealBall
?ComplexBall._is_atomic
:self.is_real() or self.imag().is_zero()
seems to be redundant. Do you want to sayself.real().is_zero()
instead?ComplexBall.add_error
: missing INPUT: and OUTPUT: sectionsComplexBall.is_nonzero
: The note section is hard to understand because__nonzero__
andis_zero
should do very different things.ComplexBall.__lshift__
,ComplexBall.__rshift__
: why is the first parameter calledval
instead ofself
and how can it happen that it is not aComplexBall
if this is a method ofComplexBall
?
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

comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 4 years ago by
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.
comment:14 in reply to: ↑ 13 Changed 4 years ago by
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...
comment:15 followups: ↓ 16 ↓ 18 Changed 4 years ago by
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.
 The naming of the files is inconsistent: why
real_arb
andcomplex_ball_acb
?
 You can remove the
include_dirs=[SAGE_INC + '/flint'])
fromsrc/module_list.py
.
 Ideally, the type declarations in the arb
.pxd
files should be in a different file: try to follow the model ofsrc/sage/libs/gmp
for example. Then you should add# distutils: libraries = arb
to the files with library functions (i.e. not types).
 The
__hash__
forRBF
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
 For
CBF.__hash__
, what's the point of>> 7
?
 This is a very strange way to handle exceptions, what's wrong with
raise
?if arf_get_mpfr(rad.value, tmp, GMP_RNDN): abort()
 Replace the
GMP_RND
macros byMPFR_RND
.
 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).
 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?
 Now that all this is standard, can you add
RBF
,RealBallField
,CBF
andComplexBallField
to the global namespace and then remove all doctest lines of the formfrom sage.rings.real_arb import RBF
.
 You should really avoid this:
include "sage/ext/python.pxi"
, justcimport
what you need.
 What's the difference between
contains_exact
and__contains__
? I don't understand why you need both these methods.
 When raising exceptions, add a string: don't just
raise TypeError
, butraise TypeError("useful message here")
 You write
.. SEEALSO:: :meth:
abs` but there is no such method.
 I still don't understand why
below_abs
andabove_abs
return a ball instead of a real number. At the very least, this should be documentated in anOUTPUT:
block in those methods.
 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 notReturn the right endpoint of this ball.
?
 I don't understand the purpose of this comment:
# TODO: once CBF is there, also wrap functions that only exist in acb*
 I don't understand what this means:
Return a copy of this ball rounded to the precision of the parent.
comment:16 in reply to: ↑ 15 ; followup: ↓ 20 Changed 4 years ago by
Replying to jdemeyer:
 The naming of the files is inconsistent: why
real_arb
andcomplex_ball_acb
?
real_arb
followed the pattern of real_mpfi
; whereas complex_ball_acb
somehow followed complex_interval
. What names would you suggest?
comment:17 in reply to: ↑ 12 ; followups: ↓ 21 ↓ 49 Changed 4 years ago by
Hello Clemens,
Thanks again for your comments and the changes you made.
Replying to cheuberg:
I read the modifications and have the following comments.
arf.pxd
: You definearf_rnd_t
as an enum and rely on the fact that the numeric values ofARF_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?
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.
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!
RealBall.upper
,RealBall.lower
,RealBall.endpoints
: missing INPUT: and OUTPUT: sections, document and test parameterrnd
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?
RealBall.add_error
: missing INPUT: and OUTPUT: sections
What would you suggest to say there?
RealBall.is_nonzero
: The note section is hard to understand because__nonzero__
andis_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()
.
RealBall.__lshift__
,RealBall.__rshift__
: why is the first parameter calledval
instead ofself
and how can it happen that it is not aRealBall
if this is a method ofRealBall
?
That's how special methods work in Cython... See http://docs.cython.org/src/userguide/special_methods.html#arithmeticmethods
ComplexBall._is_atomic
:self.is_real() or self.imag().is_zero()
seems to be redundant. Do you want to sayself.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...
comment:18 in reply to: ↑ 15 ; followups: ↓ 22 ↓ 23 Changed 4 years ago by
Hello Jeroen,
Thank you very much for your remarks.
Replying to jdemeyer:
 You can remove the
include_dirs=[SAGE_INC + '/flint'])
fromsrc/module_list.py
.
Are you sure? I get:
[2/2] gcc fnostrictaliasing 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/sitepackages/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.linuxx86_642.7/home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.o fnostrictaliasing 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
 Ideally, the type declarations in the arb
.pxd
files should be in a different file: try to follow the model ofsrc/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 welldefined dependency graph): are you suggesting to put all the type definitions in a single .pxd
file nevertheless?
 The
__hash__
forRBF
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).
 For
CBF.__hash__
, what's the point of>> 7
?
The idea was just to avoid that the λ(1+i) all hash to zero...
 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.
 Replace the
GMP_RND
macros byMPFR_RND
.
Done.
 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.
 Now that all this is standard, can you add
RBF
,RealBallField
,CBF
andComplexBallField
to the global namespace and then remove all doctest lines of the formfrom 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.
 You should really avoid this:
include "sage/ext/python.pxi"
, justcimport
what you need.
Done.
 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.
 When raising exceptions, add a string: don't just
raise TypeError
, butraise 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...)
 You write
.. SEEALSO:: :meth:
abs` but there is no such method.
Indeed, I forgot to remove these references, thanks!
 I still don't understand why
below_abs
andabove_abs
return a ball instead of a real number. At the very least, this should be documentated in anOUTPUT:
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.
 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 notReturn the right endpoint of this ball.
?
Indeed! See my answer to Clemens above.
 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.
 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.
comment:19 Changed 4 years ago by
 Branch changed from u/cheuberg/19152arbmisc to public/19152arbmisc
 Commit changed from 16a04a07c63349d3792ed8c538a7d60e5adfde5e to ea08942380a0cfab4614de252bac9a8db340b3b4
 Status changed from needs_work to needs_info
comment:20 in reply to: ↑ 16 ; followup: ↓ 25 Changed 4 years ago by
Replying to cheuberg:
real_arb
followed the pattern ofreal_mpfi
; whereascomplex_ball_acb
somehow followedcomplex_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.
comment:21 in reply to: ↑ 17 ; followup: ↓ 26 Changed 4 years ago by
Replying to mmezzarobba:
Replying to cheuberg:
arf.pxd
: You definearf_rnd_t
as an enum and rely on the fact that the numeric values ofARF_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.
RealBall.upper
,RealBall.lower
,RealBall.endpoints
: missing INPUT: and OUTPUT: sections, document and test parameterrnd
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)
comment:22 in reply to: ↑ 18 ; followup: ↓ 27 Changed 4 years ago by
Replying to mmezzarobba:
Replying to jdemeyer:
 You can remove the
include_dirs=[SAGE_INC + '/flint'])
fromsrc/module_list.py
.Are you sure? I get:
[2/2] gcc fnostrictaliasing 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/sitepackages/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.linuxx86_642.7/home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.o fnostrictaliasing 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.
 Ideally, the type declarations in the arb
.pxd
files should be in a different file: try to follow the model ofsrc/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 welldefined 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
.
 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.
 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.
 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.
 Now that all this is standard, can you add
RBF
,RealBallField
,CBF
andComplexBallField
to the global namespace and then remove all doctest lines of the formfrom 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?
 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 onp
bits. Theround()
method rounds such elements to the precision of their parent.
Now I'm even more confused... I still have no clue what you mean.
comment:23 in reply to: ↑ 18 Changed 4 years ago by
Replying to mmezzarobba:
 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.
 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 onp
bits. Theround()
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.
comment:24 Changed 4 years ago by
I consider the fact that include_dirs
is needed an upstream arb
bug. So you can leave it for now. See #19563.
comment:25 in reply to: ↑ 20 Changed 4 years ago by
Replying to jdemeyer:
Why not simply
real_arb
andcomplex_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.
comment:26 in reply to: ↑ 21 Changed 4 years ago by
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.
comment:27 in reply to: ↑ 22 ; followups: ↓ 30 ↓ 31 Changed 4 years ago by
Replying to jdemeyer:
A difference is that arb comes with several
.h
files (one for each module, with a more or less welldefined 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.
 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 callsabort()
but it's more explicit.
Changed (though to me this is “strange sagespecific 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.
 Now that all this is standard, can you add
RBF
,RealBallField
,CBF
andComplexBallField
to the global namespace and then remove all doctest lines of the formfrom 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.)
 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 onp
bits. Theround()
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.
comment:28 Changed 4 years ago by
 Commit changed from ea08942380a0cfab4614de252bac9a8db340b3b4 to d5af324cfc7c998a11c91b98d00521c2f664502c
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

comment:29 Changed 4 years ago by
 Status changed from needs_info to needs_review
comment:30 in reply to: ↑ 27 ; followup: ↓ 32 Changed 4 years ago by
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.
comment:31 in reply to: ↑ 27 Changed 4 years ago by
Replying to mmezzarobba:
I'm sorry, I don't understand what you don't understand. (You are talking about
ComplexBall.__hash__()
, notComplexBallField.__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.
comment:32 in reply to: ↑ 30 ; followup: ↓ 50 Changed 4 years ago by
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.
comment:33 Changed 4 years ago by
 Commit changed from d5af324cfc7c998a11c91b98d00521c2f664502c to f43e94a46bc9c0f03ba63ee8538384180d89d306
Branch pushed to git repo; I updated commit sha1. New commits:
f43e94a  RealBall: minor change to __hash__

comment:34 Changed 4 years ago by
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.
comment:35 Changed 4 years ago by
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
.
comment:36 followup: ↓ 40 Changed 4 years ago by
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.
comment:37 Changed 4 years ago by
 Status changed from needs_review to needs_work
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
comment:38 Changed 4 years ago by
Despite what the documentation says, we still have
sage: a = RealBallField(53)(RIF(1,1)) sage: (a*(a+0)).identical(a^2) True
comment:39 Changed 4 years ago by
 Commit changed from f43e94a46bc9c0f03ba63ee8538384180d89d306 to 7e6b5d049ce0107880171d156f3da2f16d480476
Branch pushed to git repo; I updated commit sha1. New commits:
7e6b5d0  real_arb: minor doc fix

comment:40 in reply to: ↑ 36 ; followups: ↓ 41 ↓ 43 Changed 4 years ago by
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 fromx^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 arbrelated 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.
comment:41 in reply to: ↑ 40 Changed 4 years ago by
Even though I don't agree with everything in this branch, I'm willing to give it positive_review.
comment:42 Changed 4 years ago by
comment:43 in reply to: ↑ 40 ; followup: ↓ 44 Changed 4 years ago by
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 fromx^2
.Both these design choices were discussed on previous arbrelated 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?
comment:44 in reply to: ↑ 43 ; followup: ↓ 46 Changed 4 years ago by
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.
comment:45 Changed 4 years ago by
comment:46 in reply to: ↑ 44 Changed 4 years ago by
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."
comment:47 Changed 4 years ago by
I meant that the documentation should state this is done by the arb
library, not the Sage interface to arb
.
comment:48 Changed 4 years ago by
 Commit changed from 7e6b5d049ce0107880171d156f3da2f16d480476 to cc74b61498b0ed0e6e5a54e30df6b5cfbe33350a
comment:49 in reply to: ↑ 17 ; followup: ↓ 52 Changed 4 years ago by
Replying to mmezzarobba:
Replying to cheuberg:
arf.pxd
: You definearf_rnd_t
as an enum and rely on the fact that the numeric values ofARF_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.
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
wherex = Pol.gen()
, not(2*x  x)^n
, and the new version seems to be about 10% faster whenn == 2
. But I'm open to suggestions.
Well, I would have preferred to have somebody else to decide about that;
RealBall.upper
,RealBall.lower
,RealBall.endpoints
: missing INPUT: and OUTPUT: sections, document and test parameterrnd
The problem here is that, unlike elements of
RealIntervalField(p)
, elements ofRealBallField(p)
may have endpoints that are not be representable onp
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.
RealBall.add_error
: missing INPUT: and OUTPUT: sectionsWhat 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).
RealBall.is_nonzero
: The note section is hard to understand because__nonzero__
andis_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, thedegree()
methods of polynomials look for the leading__nonzero__
term), which for balls is exactly the negation ofis_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:
 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.
comment:50 in reply to: ↑ 32 Changed 4 years ago by
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 ...
comment:51 Changed 4 years ago by
 Commit changed from cc74b61498b0ed0e6e5a54e30df6b5cfbe33350a to c8873f0d1fac0deb01a75d2203907b5a77fecf0e
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

comment:52 in reply to: ↑ 49 ; followup: ↓ 59 Changed 4 years ago by
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 ofis_zero()
, whereasis_nonzero()
only returnsTrue
if0
is known not to be in the ball."
Done.
Further remark:
 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 sagedevel 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...
comment:53 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:54 Changed 4 years ago by
 Commit changed from c8873f0d1fac0deb01a75d2203907b5a77fecf0e to 7920b9f9fd59f3271c45eb11b370eef3787b3242
Branch pushed to git repo; I updated commit sha1. New commits:
7920b9f  {real,complex}_arb: clarify that b == b in arb too

comment:55 Changed 4 years ago by
 Status changed from needs_review to needs_work
Oops, merge conflict will the last develop
. I will overwrite the last few commits in a minute to avoid the conflict.
comment:56 Changed 4 years ago by
 Commit changed from 7920b9f9fd59f3271c45eb11b370eef3787b3242 to 3749cbb41597447edb6f33fd43a6da1a4aac01dd
comment:57 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:58 Changed 4 years ago by
 Commit changed from 3749cbb41597447edb6f33fd43a6da1a4aac01dd to 0a04e627728c37d334c3576eb5243facebee0e80
comment:59 in reply to: ↑ 52 ; followups: ↓ 61 ↓ 62 Changed 4 years ago by
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.
comment:60 Changed 4 years ago by
 Status changed from needs_review to needs_info
comment:61 in reply to: ↑ 59 Changed 4 years ago by
comment:62 in reply to: ↑ 59 ; followup: ↓ 63 Changed 4 years ago by
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...
comment:63 in reply to: ↑ 62 ; followup: ↓ 64 Changed 4 years ago by
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?
comment:64 in reply to: ↑ 63 ; followup: ↓ 67 Changed 4 years ago by
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 2015111616344163268156. Git branch: arbmisc 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/sitepackages/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/sitepackages/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)
comment:65 Changed 4 years ago by
 Status changed from needs_info to needs_review
comment:66 Changed 4 years ago by
 Commit changed from 0a04e627728c37d334c3576eb5243facebee0e80 to b21aa6e069182b08aca3cf7c24d6ed24e5926510
Branch pushed to git repo; I updated commit sha1. New commits:
b21aa6e  Trac #19152: lazy_import

comment:67 in reply to: ↑ 64 Changed 4 years ago by
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 64bit linux though.
Let's see what the patchbots say.
comment:68 Changed 4 years ago by
 Commit changed from b21aa6e069182b08aca3cf7c24d6ed24e5926510 to 0a04e627728c37d334c3576eb5243facebee0e80
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:69 Changed 4 years ago by
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/19152arbmisclazyimport
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.
comment:70 followup: ↓ 71 Changed 4 years ago by
Fine with me; There seems to be something fishy with lazy import and coercion. Can you open a new ticket and push your lazyimport branch there?
comment:71 in reply to: ↑ 70 Changed 4 years ago by
 Status changed from needs_review to positive_review
comment:72 Changed 4 years ago by
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?
comment:73 followup: ↓ 74 Changed 4 years ago by
It's safer to do it in a new ticket (The release manager might already have pulled this ticket for inclusion).
comment:74 in reply to: ↑ 73 Changed 4 years ago by
comment:75 Changed 4 years ago by
 Branch changed from public/19152arbmisc to 0a04e627728c37d334c3576eb5243facebee0e80
 Resolution set to fixed
 Status changed from positive_review to closed
Last 10 new commits:
ComplexBall: conversions to ZZ, RR, RIF, CC, CIF
ComplexBallField: faster access to real field
{Real,Complex}Ball: implement conversions to QQ
ComplexBall: containment and overlapping predicates
ComplexBall: abs(), arg()
RealBall: implement abs()
{Real,Complex}Ball: implement {above,below}_abs()
ComplexBallField: minor documentation improvements
RealBall: upper(), lower(), endpoints()
{Real,Complex}Ball: change __nonzero__ to check for syntactic !=0