Opened 5 years ago
Closed 5 years ago
#17194 closed enhancement (fixed)
Minimal bindings for optional arb package
Reported by:  cheuberg  Owned by:  

Priority:  major  Milestone:  sage6.5 
Component:  numerical  Keywords:  arb, digamma function, real interval field 
Cc:  alina, fredrik.johansson, ktkohl, skropf, jdemeyer, mmezzarobba  Merged in:  
Authors:  Clemens Heuberger, Marc Mezzarobba  Reviewers:  Marc Mezzarobba 
Report Upstream:  N/A  Work issues:  
Branch:  195a6ee (Commits)  Commit:  195a6ee9c9efe97f24e806526cc3ec50c7b93363 
Dependencies:  #16747, #17688  Stopgaps: 
Description (last modified by )
In #16747, the optional arb package is proposed for inclusion in sage. That ticket did not have any bindings for the Sage library.
In the present ticket, rather minimal bindings are provided.
As a proof of concept, the digamma function of a RealIntervalFieldElement
is implemented by converting it to an RealBall
, computing the digamma function via the arb library, and converting the result back.
Change History (82)
comment:1 Changed 5 years ago by
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
comment:3 Changed 5 years ago by
Sage should arguably have a RealBallField
and ComplexBallField
with concrete implementations RealBallField_arb
and ComplexBallField_arb
(it would be useful to provide alternative implementations, for example to provide actual circular complex balls).
Anyway, I took a quick look at the code, and it seems fine to me.
comment:4 Changed 5 years ago by
The question is how the relation to RealIntervalField
and ComplexIntervalField
should be.
comment:5 Changed 5 years ago by
 Cc mmezzarobba added
comment:6 Changed 5 years ago by
 Commit changed from 4a343229803c8b3dafec47fb13cb12e49e51bdb6 to 55bdc8b67439f5742c8a9fed70dfc795262377c2
comment:7 Changed 5 years ago by
 Commit changed from 55bdc8b67439f5742c8a9fed70dfc795262377c2 to 14f599fe6469378d9474fb52a71caabbcfe7eb52
Branch pushed to git repo; I updated commit sha1. New commits:
14f599f  Trac #17194: rename attribute precision to _precision_

comment:8 Changed 5 years ago by
As a followup, minimal bindings for the acb
type (complex balls) are provided in #17218.
comment:9 Changed 5 years ago by
The code looks ok to me. A small detail is that precision must be >= 2, not just positive (this is required by RealField
et al too).
comment:10 Changed 5 years ago by
 Commit changed from 14f599fe6469378d9474fb52a71caabbcfe7eb52 to 11df4b42506bfeaa63796e6dff90085353f8a730
Branch pushed to git repo; I updated commit sha1. New commits:
11df4b4  Trac #17194: Clarify that precision >=2 is required

comment:11 Changed 5 years ago by
 Commit changed from 11df4b42506bfeaa63796e6dff90085353f8a730 to 141aca0b89aa89b5e4a810b219d1f6fb8c94f4f8
Branch pushed to git repo; I updated commit sha1. New commits:
141aca0  Trac #17194: >2 should have been >= 2 in previous commit

comment:12 followup: ↓ 15 Changed 5 years ago by
 Status changed from needs_review to needs_work
Please don't use is_package_installed()
. That's extremely slow, you don't want to call it every time.
comment:13 followup: ↓ 24 Changed 5 years ago by
I disagree with
In its present minimal form, this is not part of the sage coercion world.
You should have a Parent
from the start, adding it afterwards is only going to make things more difficult.
comment:14 Changed 5 years ago by
And Arb
should probably be renamed to RealBallElement
.
comment:15 in reply to: ↑ 12 ; followups: ↓ 16 ↓ 18 Changed 5 years ago by
Replying to jdemeyer:
Please don't use
is_package_installed()
. That's extremely slow, you don't want to call it every time.
What alternative do you recommend? Calling is_package_installed()
once and storing the result in a module global variable? Or just try
ing to call the arb code and catching the exception?
Concerning renaming Arb
to RealBallElement
: real_mpfi
aka RealIntervalFieldElement
and arb
have some similarities in their goals; the implementation is different, there might be more functions in the arb
library, and there are the corresponding functions for complex balls. Do we want to stress the difference between RealBallElement
and RealIntervalFieldElement
? Or would RealBallElement
be some kind of special case of RealIntervalFieldElement
.
I'll try to implement a parent, but it will take some time. The parent would then be RealBallField
; what about RealBallFieldElement
for the current Arb
?
comment:16 in reply to: ↑ 15 ; followup: ↓ 20 Changed 5 years ago by
Replying to cheuberg:
What alternative do you recommend?
Try the import and catch ImportError
. Would that work?
comment:17 Changed 5 years ago by
You could possibly make it so that one can choose between two different implementations of RealIntervalField
and ComplexIntervalField
, just like one can choose between:
PolynomialRing(ZZ, implementation='NTL')
PolynomialRing(ZZ, implementation='FLINT')
So you might look up how those are implemented. I don't have an opinion on whether this is better than what you're doing right now.
comment:18 in reply to: ↑ 15 ; followup: ↓ 21 Changed 5 years ago by
Replying to cheuberg:
Do we want to stress the difference between
RealBallElement
andRealIntervalFieldElement
?
Yes, I would do that. The implementations are also quite different.
I'll try to implement a parent, but it will take some time. The parent would then be
RealBallField
; what aboutRealBallFieldElement
for the currentArb
?
I slightly prefer the shorter name RealBallElement
(like RealDoubleElement
), but I can live with RealBallFieldElement
.
comment:19 Changed 5 years ago by
 Commit changed from 141aca0b89aa89b5e4a810b219d1f6fb8c94f4f8 to a0afebd76117cdaeae6d8a408a2b1071ec3b8cd2
comment:20 in reply to: ↑ 16 Changed 5 years ago by
comment:21 in reply to: ↑ 18 Changed 5 years ago by
 Description modified (diff)
Replying to jdemeyer:
I slightly prefer the shorter name
RealBallElement
(likeRealDoubleElement
), but I can live withRealBallFieldElement
.
As we apparently do not have a systematic naming scheme here, I renamed it to the shorter RealBallElement
, as suggested.
comment:22 Changed 5 years ago by
 Description modified (diff)
 Summary changed from Minimal bindings for optional arb package; RealIntervalFieldElement.psi to Minimal bindings for optional arb package
comment:23 Changed 5 years ago by
 Commit changed from a0afebd76117cdaeae6d8a408a2b1071ec3b8cd2 to a194938dca3bed5878bfa85ab94d0e52425298d4
comment:24 in reply to: ↑ 13 Changed 5 years ago by
 Status changed from needs_work to needs_review
Replying to jdemeyer:
I disagree with
In its present minimal form, this is not part of the sage coercion world.
You should have a
Parent
from the start, adding it afterwards is only going to make things more difficult.
I have implemented a parent RealBallField
.
comment:25 Changed 5 years ago by
trac's automerge fails for unknown reasons; I have successfully tried to merge this branch with 6.5.beta0
.
comment:26 Changed 5 years ago by
The radius could just be printed with 35 digits (it is never a high precision number).
comment:27 followup: ↓ 28 Changed 5 years ago by
In fact, the case could be made for printing in the same format as RealIntervalField
.
comment:28 in reply to: ↑ 27 Changed 5 years ago by
Replying to fredrik.johansson:
In fact, the case could be made for printing in the same format as
RealIntervalField
.
jdemeyer was in favor of making a clear distinction between RealIntervalField
and RealBallField
. Therefore, I chose to implement the _repr_
method of a RealBallElement
to look different from that of a RealIntervalFieldElement
and close to the arb_printd
method.
Of course, doing it the RealIntervalFieldElement
way would be easy, because conversion to a RealIntervalFieldElement
is already implemented, so we could simply delegate the task.
Jeroen, do you have a comment on this?
comment:29 Changed 5 years ago by
 Commit changed from a194938dca3bed5878bfa85ab94d0e52425298d4 to 1ce0e90678ad06f6004be7fc8eb47f58ed9184ac
Branch pushed to git repo; I updated commit sha1. New commits:
fbcac86  Merge remotetracking branch 'trac/develop' into rings/real_arb

e1683f0  Trac #17194: Changes required after merge of #17267

6ef90e0  Trac #17194: _coerce_map_from_ as cpdef instead of def

1ce0e90  Trac #17194: use weak_cached_function for RealBallField

comment:30 Changed 5 years ago by
comment:31 Changed 5 years ago by
 Commit changed from 1ce0e90678ad06f6004be7fc8eb47f58ed9184ac to f4393a997a6bdee1d5765fb5f7e8c0a9dff61e35
Branch pushed to git repo; I updated commit sha1. New commits:
f4393a9  Trac #17194: remove one obsolete import

comment:32 followups: ↓ 35 ↓ 38 Changed 5 years ago by
A few random remarks (I would be interested in working more on this, but unfortunately I don't expect to have time to do it before a few weeks):
 Why call the element class
RealBallElement
and not justRealBall
? to me the former sounds more like a point in a real ball, not the ball itself...  Does the parent class really need to be a Cython class?
 I'd rename the
RealIntervalFieldElement
method to something likeinterval
, or perhaps_interval
since in the long run it should be accessible through coercion/conversion toRealIntervalField
(which is currently broken in several ways, but that's another story).  I'm not sure if setting up coercion maps from real interval fields is a good idea. It's probably sound from the mathematical point of view, but I seem to remember that bidirectional coercions are at least discouraged, and when computing, say, the sum of an interval and a ball, I'd tend to expect the result to be an interval. Or perhaps this should depend on the precisions of the arguments...?
comment:33 Changed 5 years ago by
 Commit changed from f4393a997a6bdee1d5765fb5f7e8c0a9dff61e35 to 80850af551dcc6634c322e0f023515ecbe14d2d8
comment:34 Changed 5 years ago by
 Commit changed from 80850af551dcc6634c322e0f023515ecbe14d2d8 to a45f691daf51970311b43190496104a774d6518b
comment:35 in reply to: ↑ 32 ; followup: ↓ 39 Changed 5 years ago by
 Description modified (diff)
Replying to mmezzarobba:
 Why call the element class
RealBallElement
and not justRealBall
? to me the former sounds more like a point in a real ball, not the ball itself...
renamed.
 Does the parent class really need to be a Cython class?
What are the disadvantages if it is a Cython class?
While working on #17222, I found that I spent most of the time in many, many calls of ComplexIntervalField.__call__
. Therefore, I wanted RealBallField
to be a Cython class in order to save any bit of performance possible.
 I'm not sure if setting up coercion maps from real interval fields is a good idea. It's probably sound from the mathematical point of view, but I seem to remember that bidirectional coercions are at least discouraged, and when computing, say, the sum of an interval and a ball, I'd tend to expect the result to be an interval. Or perhaps this should depend on the precisions of the arguments...?
I do not understand your last comment on the precisions of the arguments.
Apart from that, do you have an opinion on how the output of the balls should look like? See comments 27 and 28.
comment:36 Changed 5 years ago by
 Dependencies changed from #16747 to #16747, #17688
 Status changed from needs_review to needs_work
As Arb 2.5 has a function arb_get_str
, this should be used for the output. So, we should upgrade to 2.5 (#17688).
comment:37 Changed 5 years ago by
 Commit changed from a45f691daf51970311b43190496104a774d6518b to 8802939c14030e34ad81d7a6acea00c5a8e71672
Branch pushed to git repo; I updated commit sha1. New commits:
0eb70b0  Trac #17688: Upgrade arb spkg to 2.5.0

50eeecb  Merge branch 'u/cheuberg/packages/arb2.5' (#17688) of git://trac.sagemath.org/sage into rings/real_arb

6c41d27  Trac #17194: Remove coercion from RealIntervalField

8802939  Trac #17194: Use arb's arg_get_str for RealBall._repr_

comment:38 in reply to: ↑ 32 Changed 5 years ago by
 Milestone changed from sage6.4 to sage6.5
 Status changed from needs_work to needs_review
Replying to mmezzarobba:
 I'm not sure if setting up coercion maps from real interval fields is a good idea. It's probably sound from the mathematical point of view, but I seem to remember that bidirectional coercions are at least discouraged, and when computing, say, the sum of an interval and a ball, I'd tend to expect the result to be an interval. Or perhaps this should depend on the precisions of the arguments...?
This coercion is now removed. So we do not coerce from anywhere at the moment. Coercions may be added later as needed.
Furthermore, we use arb's arb_get_str
method for _repr_
now.
comment:39 in reply to: ↑ 35 ; followup: ↓ 40 Changed 5 years ago by
Replying to cheuberg:
 Does the parent class really need to be a Cython class?
What are the disadvantages if it is a Cython class?
I don't know exactly :)
. I think Nicolas Thiéry used to say that sticking to Python for parents when possible was better, perhaps because of the tricks the category framework plays. But if there is a performance reason for using Cython, I'm all for it!
I will try to have a closer look at this ticket in the next few days if I have time... but no guarantees at all!
comment:40 in reply to: ↑ 39 ; followup: ↓ 45 Changed 5 years ago by
Replying to mmezzarobba:
 Does the parent class really need to be a Cython class?
What are the disadvantages if it is a Cython class?
[...] But if there is a performance reason for using Cython, I'm all for it!
In view of the answers to my recent questions on sagedevel (<mbdlc7$bjo$2@ger.gmane.org>
and its thread), I guess we should indeed try moving to a Python class (still compiled by Cython) and see if, for example, just making the element constructor a Cython function suffices to solve your problem. (I can probably do that if you want.)
I have another general design question. In arb, there is no precision attached to a number or a ball, precisions are parameters of arithmetic operations. Hence, to some extent, it would make sense to have a single real ball field with no precision parameter. Except of course that we need at least a default precision for operations like coercion from exact rings and arithmetic on exact balls. (For operations with inexact balls, another, less flexible option would be to choose the default precision based on the input radii.) So I guess the model you have in mind is that the parent provides the precision parameter of arb functions, but otherwise all RealBallField
s can have elements balls with any radius and any midpoint "precision". Is that correct?
Finally, it is clear that I am not confortable enough with the issues involved to provide a good review by myself. I'd be happy to continue reviewing the ticket nonetheless, but it would be nice if someone more experienced could "review my review". (Jeroen, would you be willing to do that?)
comment:41 Changed 5 years ago by
Incidentally, with the current (noncpdef
, going through RIF
, etc., so this probably doesn't mean much) _element_constructor_
, a call to RBF(42)
even from Cython takes slightly less time with a Python version of the parent class...
comment:42 followup: ↓ 44 Changed 5 years ago by
I made a few changes and pushed them to u/mmezzarobba/arb
. Please tell me what you think. Jeroen, if you are reading this, your comments are also more than welcome.
comment:43 Changed 5 years ago by
 Commit changed from 8802939c14030e34ad81d7a6acea00c5a8e71672 to ec8d7062c18fde4ffa52191e866cb465a76b751f
Branch pushed to git repo; I updated commit sha1. New commits:
676f8a2  Make RealBallField a plain Python class

a61566f  Minor doc improvements

7fa8fd1  Modernize [PC]ython style

b7eaa0e  sig_on/sig_off

0aaf188  Clarify that the precision is a default precision

5be8dfa  clarify(?) error message

3e1ed11  Trac #17194: Mark one doctest as #optional  arb

ec8d706  Trac #17194: Language correction

comment:44 in reply to: ↑ 42 ; followup: ↓ 46 Changed 5 years ago by
Replying to mmezzarobba:
I made a few changes and pushed them to
u/mmezzarobba/arb
. Please tell me what you think.
I added up two oneline patches and pushed it on the ticket.
From the discussion on sagedevel, I also remember the recommendation to have the parent in a different .py
file. If I understand the reasoning correctly, this seems to be preferred in order to avoid circular C imports.
Now, the behaviour is that the precision is stored in the Python parent. If we end up needing the precision frequently in arithmetic operations, that might turn out to be a bottleneck.
comment:45 in reply to: ↑ 40 ; followup: ↓ 51 Changed 5 years ago by
Replying to mmezzarobba:
Replying to mmezzarobba:
 Does the parent class really need to be a Cython class?
What are the disadvantages if it is a Cython class?
[...] But if there is a performance reason for using Cython, I'm all for it!
In view of the answers to my recent questions on sagedevel (
<mbdlc7$bjo$2@ger.gmane.org>
and its thread), I guess we should indeed try moving to a Python class (still compiled by Cython) and see if, for example, just making the element constructor a Cython function suffices to solve your problem. (I can probably do that if you want.)
Thanks for doing it.
I now believe that my performance problems in #17222 with ComplexIntervalField
might be caused by the fact that in ComplexIntervalField
, the call
function is overridden in Python.
I have another general design question. In arb, there is no precision attached to a number or a ball, precisions are parameters of arithmetic operations. Hence, to some extent, it would make sense to have a single real ball field with no precision parameter. Except of course that we need at least a default precision for operations like coercion from exact rings and arithmetic on exact balls. (For operations with inexact balls, another, less flexible option would be to choose the default precision based on the input radii.) So I guess the model you have in mind is that the parent provides the precision parameter of arb functions, but otherwise all
RealBallField
s can have elements balls with any radius and any midpoint "precision". Is that correct?
Yes.
comment:46 in reply to: ↑ 44 ; followup: ↓ 47 Changed 5 years ago by
Replying to cheuberg:
From the discussion on sagedevel, I also remember the recommendation to have the parent in a different
.py
file. If I understand the reasoning correctly, this seems to be preferred in order to avoid circular C imports.
And/or to keep the parent interpreted (which has some benefits in terms of debugging, build time...), I guess? But I think we really want a compiled parent in the present case, even if it remains a Python class, so it doesn't matter, does it?
Now, the behaviour is that the precision is stored in the Python parent. If we end up needing the precision frequently in arithmetic operations, that might turn out to be a bottleneck.
Yes, that was my concern as well. But let's see, and if that's the case, we can always turn the parent back to a Cython class (or possibly keep a copy of the precision in the elements?). I will introduce a function to access the precision of an element so that we can go back to a Cython parent without changing every reference to the precision.
comment:47 in reply to: ↑ 46 Changed 5 years ago by
Replying to mmezzarobba:
Now, the behaviour is that the precision is stored in the Python parent. If we end up needing the precision frequently in arithmetic operations, that might turn out to be a bottleneck.
Yes, that was my concern as well. But let's see, and if that's the case, we can always turn the parent back to a Cython class (or possibly keep a copy of the precision in the elements?).
ok.
I will introduce a function to access the precision of an element so that we can go back to a Cython parent without changing every reference to the precision.
Thank you.
comment:48 Changed 5 years ago by
Another question: is there a reason to declare precision
as unsigned in your code? The precision parameter of arb (and mpfr) functions is signed.
comment:49 Changed 5 years ago by
Not really. I cannot imagine what a negative precision should be, but if it is signed in arb and mpfr, then let's be signed too.
comment:50 Changed 5 years ago by
Signed integers are used because of the problems mixing signed and unsigned types in C, and precisions above LONG_MAX would not work anyway. In fact there is a limit somewhat smaller than MPFR_PREC_MAX above which things will start breaking. For a sanity check in the parent constructor, something like LONG_MAX / 16 should be fine (this is a bit conservative, but there's currently no macro in arb that gives a firm bound).
comment:51 in reply to: ↑ 45 ; followup: ↓ 53 Changed 5 years ago by
Replying to cheuberg:
I have another general design question. In arb, there is no precision attached to a number or a ball, precisions are parameters of arithmetic operations. Hence, to some extent, it would make sense to have a single real ball field with no precision parameter. Except of course that we need at least a default precision for operations like coercion from exact rings and arithmetic on exact balls. (For operations with inexact balls, another, less flexible option would be to choose the default precision based on the input radii.) So I guess the model you have in mind is that the parent provides the precision parameter of arb functions, but otherwise all
RealBallField
s can have elements balls with any radius and any midpoint "precision". Is that correct?Yes.
Ok, thanks. And do you think operations on balls should take a precision argument (defaulting to the parent's precision)? Or is the ability to change the parent enough?
In other words, assuming b
is a RealBall
resulting from a 100bit operation, and thus living in RBF(100)
, which commands among the following do you think should work to compute the exponential of b
with 53 bits of precision:
RealBallField(53)(b).exp()
(yes by the discussion I am quoting, I guess),b.change_prec(53).exp()
(a possible shortcut for the above),b.exp(prec=53)
(that's my question)?
(All three would yield a result in RBF(53)
.)
Depending on that, I wonder if we shouldn't revert the commit where I changed "precision" to "default precision" in the messages.
comment:52 followup: ↓ 54 Changed 5 years ago by
I think precision should be handled identically for all versions of real and complex fields (from Sage's point of view, it doesn't really matter whether the precision is part of the underlying C structures).
comment:53 in reply to: ↑ 51 Changed 5 years ago by
Replying to mmezzarobba:
Replying to cheuberg:
I have another general design question. In arb, there is no precision attached to a number or a ball, precisions are parameters of arithmetic operations. Hence, to some extent, it would make sense to have a single real ball field with no precision parameter. Except of course that we need at least a default precision for operations like coercion from exact rings and arithmetic on exact balls. (For operations with inexact balls, another, less flexible option would be to choose the default precision based on the input radii.) So I guess the model you have in mind is that the parent provides the precision parameter of arb functions, but otherwise all
RealBallField
s can have elements balls with any radius and any midpoint "precision". Is that correct?Yes.
Ok, thanks. And do you think operations on balls should take a precision argument (defaulting to the parent's precision)? Or is the ability to change the parent enough?
IMHO, changing the parent is enough.
In other words, assuming
b
is aRealBall
resulting from a 100bit operation, and thus living inRBF(100)
, which commands among the following do you think should work to compute the exponential ofb
with 53 bits of precision:
RealBallField(53)(b).exp()
(yes by the discussion I am quoting, I guess),
yes, but not yet implemented.
I think that RealBallField(53)(b)
would not actually change anything in b
except for changing the parent.
b.change_prec(53).exp()
(a possible shortcut for the above),
I do not think that this is needed: we'd have to find the correct parent, after all.
b.exp(prec=53)
(that's my question)?
I do not think that this is necessary.
(All three would yield a result in
RBF(53)
.)Depending on that, I wonder if we shouldn't revert the commit where I changed "precision" to "default precision" in the messages.
The Arb documentation says "The precision parameter [...] roughly indicates the precision to which calculations on the midpoint are carried out".
Later on, it is also mentioned that "For more complex operations, the precision parameter indicates a minimum working precision".
So "default precision" is not really correct, because the user is not allowed to change the default precision. So perhaps "working precision" would be more correct, or simply "precision".
comment:54 in reply to: ↑ 52 ; followup: ↓ 55 Changed 5 years ago by
Replying to fredrik.johansson:
I think precision should be handled identically for all versions of real and complex fields (from Sage's point of view, it doesn't really matter whether the precision is part of the underlying C structures).
IMHO, that is a valid point.
comment:55 in reply to: ↑ 54 Changed 5 years ago by
 Branch changed from u/cheuberg/rings/real_arb to u/mmezzarobba/17194arb
 Commit changed from ec8d7062c18fde4ffa52191e866cb465a76b751f to ce51f81a120e229bc082a16d03b855607335fba9
Replying to cheuberg:
Replying to fredrik.johansson:
I think precision should be handled identically for all versions of real and complex fields (from Sage's point of view, it doesn't really matter whether the precision is part of the underlying C structures).
IMHO, that is a valid point.
Ok, I think we all agree. Thanks for your input!
So I made a few more changes as promised. As far as I am concerned I'd be happy to set the ticket to positive review, but (i) at the very least someone should review my commits, and (ii) it would be great if someone with more experience on writing (efficient) parent could check that we are not doing anything that will prove awfully wrong later on.
And (as a way to reassure myself regarding this last point ;)
) I started implementing some basic arithmetic (edit: and more...) on RealBall
s. See u/mmezzarobba/arb
. But that's for another ticket, unless you feel like reviewing the first few commits here.
New commits:
555df20  Revert "Clarify that the precision is a default precision"

5dbc48d  #17194 RBF: prec should be a signed long

dc1d64b  #17194 doc

aa39c10  #17194 RealBall: abstract away access to parent precision

d3e3998  #17194 RBF: add a precision() method

ce51f81  #17194 RBF: some more is_foo() and similar methods

comment:56 followup: ↓ 57 Changed 5 years ago by
Followup: #17786.
comment:57 in reply to: ↑ 56 ; followup: ↓ 58 Changed 5 years ago by
Replying to mmezzarobba:
Followup: #11786.
The followup is actually #17786.
I'd propose to move is_zero
and is_nonzero
from #17786 to this ticket in order to avoid a change of behaviour later on. Similarly, I think that rich comparison and equality checking should be implemented here.
comment:58 in reply to: ↑ 57 Changed 5 years ago by
Replying to cheuberg:
The followup is actually #17786.
Oops, thanks. I will edit my other post to avoid any confusion.
I'd propose to move
is_zero
andis_nonzero
from #17786 to this ticket in order to avoid a change of behaviour later on. Similarly, I think that rich comparison and equality checking should be implemented here.
Fine with me. Feel free to do it if you want, I will again be a bit busy in the next days.
comment:59 Changed 5 years ago by
 Branch changed from u/mmezzarobba/17194arb to u/cheuberg/17194arb
comment:60 Changed 5 years ago by
 Commit changed from ce51f81a120e229bc082a16d03b855607335fba9 to 5e41fcbc08bf0e04ff5a41ea60866568c5bc8588
Branch pushed to git repo; I updated commit sha1. New commits:
5e41fcb  Trac #17194: remove imports of fmpr

comment:61 Changed 5 years ago by
 Description modified (diff)
 Status changed from needs_review to needs_work
 Work issues set to is_zero, is_nonzero, __richcmp__
Replying to mmezzarobba:
So I made a few more changes as promised.
Thank you.
As far as I am concerned I'd be happy to set the ticket to positive review, but (i) at the very least someone should review my commits,
I crossreviewed your commits, they look good to me, doctests pass. For a test, I included real_arb
into the documentation, documentation built fine.
Furthermore, I removed fmpr.pxd
because it is no longer needed and deprecated in arb.
I'll take care of is_zero
, is_nonzero
, __richcmp__
.
comment:62 Changed 5 years ago by
 Commit changed from 5e41fcbc08bf0e04ff5a41ea60866568c5bc8588 to 195a6ee9c9efe97f24e806526cc3ec50c7b93363
comment:63 followup: ↓ 65 Changed 5 years ago by
The semantics of !=
differ from those of real intervals: A ball is considered nonzero if it does not contain 0
.
So, !=
is not not ==
.
I prefer this for two reasons:
 We are wrapping
arb
here, andarb
has this convention (is_nonzero
). __richcmp__
allows independent implementations for==
and!=
; I take this as a hint.
Furthermore, the semantics of ==
are different from real intervals: If a is b
, then a==b
, independently of whether they are exact or not. Otherwise, a==b
can only hold if a
and b
are exact.
comment:64 Changed 5 years ago by
 Status changed from needs_work to needs_review
 Work issues is_zero, is_nonzero, __richcmp__ deleted
comment:65 in reply to: ↑ 63 ; followup: ↓ 66 Changed 5 years ago by
Replying to cheuberg:
The semantics of
!=
differ from those of real intervals: A ball is considered nonzero if it does not contain0
.So,
!=
is notnot ==
.
Hmm, if I read correctly, that's also how comparisons of RealIntervalFieldElement
s work. Can you please be more specific about the difference you're seeing? For me the only difference is that, in the case of RealIntervalFieldElement
s, __nonzero__
is not consistent with != 0
.
I prefer this for two reasons:
 We are wrapping
arb
here, andarb
has this convention (is_nonzero
).
(I don't understand what you mean here, but I don't think it matters.)
__richcmp__
allows independent implementations for==
and!=
; I take this as a hint.
Yes, I agree. Unfortunately, this convention (having True
mean true and False
mean false or don't know, basically) is not observed consistently in sage. Besides, I am very unconfortable with comparisons in sage in general, mostly due to the fact that they try to apply coercions, which leads to all sorts of problems when different parents use different semantics... So I guess I'd be in favor of having methods that duplicate the behavior of comparisons but only work for balls, in order not to risk mixing balls with other object types by mistake!
Furthermore, the semantics of
==
are different from real intervals: Ifa is b
, thena==b
, independently of whether they are exact or not.
I don't know what to think about that. That's consistent with the semantics of arb functions, and up to now I have sticked to the convention in the bindings I wrote. But I fear this will lead to unexpected behaviors and inconsistencies wrt the rest of sage, and I really believe we should think it through more carefully before committing to the arb semantics.
comment:66 in reply to: ↑ 65 ; followup: ↓ 67 Changed 5 years ago by
Replying to mmezzarobba:
Replying to cheuberg:
The semantics of
!=
differ from those of real intervals: A ball is considered nonzero if it does not contain0
.So,
!=
is notnot ==
.Hmm, if I read correctly, that's also how comparisons of
RealIntervalFieldElement
s work. Can you please be more specific about the difference you're seeing? For me the only difference is that, in the case ofRealIntervalFieldElement
s,__nonzero__
is not consistent with!= 0
.
I hope that the following examples make my impression of the current behaviour clear.
sage: a = RIF(1, 1) sage: from sage.rings.real_arb import RealBallField sage: RBF = RealBallField() sage: b = RBF(a) sage: a.__nonzero__() True sage: b.__nonzero__() False sage: bool(a) True sage: bool(b) False sage: a == 0 False sage: b == 0 False sage: a != 0 False sage: b != 0 False
__richcmp__
allows independent implementations for==
and!=
; I take this as a hint.Yes, I agree. Unfortunately, this convention (having
True
mean true andFalse
mean false or don't know, basically) is not observed consistently in sage. Besides, I am very unconfortable with comparisons in sage in general, mostly due to the fact that they try to apply coercions, which leads to all sorts of problems when different parents use different semantics... So I guess I'd be in favor of having methods that duplicate the behavior of comparisons but only work for balls, in order not to risk mixing balls with other object types by mistake!
In commit 5e41fcb, prior to my implementation of comparison, I obtain
sage: b < RIF(3) True
on my system because, sadly, sage reverts to comparing types if it cannot compare elements.
So if I understand you correctly, you'd be in favor of having methods RealBall.lt
and leave the original methods untouched? I fear that a user may then mistakenly use the above example and get "surprising" results.
Perhaps we could overwrite __richcmp__
in such a way that, if no coercions to balls are available, a NotImplementedError
is raised?
Furthermore, the semantics of
==
are different from real intervals: Ifa is b
, thena==b
, independently of whether they are exact or not.I don't know what to think about that. That's consistent with the semantics of arb functions, and up to now I have sticked to the convention in the bindings I wrote. But I fear this will lead to unexpected behaviors and inconsistencies wrt the rest of sage, and I really believe we should think it through more carefully before committing to the arb semantics.
I do not understand what you mean. In my understanding, having the implication that identical objects are mathematically equal, should not lead to unexpected behavior within the rest of sage.
For me, the behavior of RealIntervalFieldElement
sage: a = RIF(1, 1) sage: from sage.rings.real_arb import RealBallField sage: RBF = RealBallField() sage: b = RBF(a) sage: a == a False sage: b == b True
is very unexpected. After all, equality is usually assumed to be reflexive.
comment:67 in reply to: ↑ 66 ; followups: ↓ 68 ↓ 69 ↓ 70 Changed 5 years ago by
Replying to cheuberg:
Replying to mmezzarobba:
Replying to cheuberg:
The semantics of
!=
differ from those of real intervals: A ball is considered nonzero if it does not contain0
.So,
!=
is notnot ==
.Hmm, if I read correctly, that's also how comparisons of
RealIntervalFieldElement
s work. Can you please be more specific about the difference you're seeing? For me the only difference is that, in the case ofRealIntervalFieldElement
s,__nonzero__
is not consistent with!= 0
.I hope that the following examples make my impression of the current behaviour clear.
sage: a = RIF(1, 1) sage: from sage.rings.real_arb import RealBallField sage: RBF = RealBallField() sage: b = RBF(a) sage: a.__nonzero__() True sage: b.__nonzero__() False sage: bool(a) True sage: bool(b) False sage: a == 0 False sage: b == 0 False sage: a != 0 False sage: b != 0 False
So I think we agree: ==
and !=
behave the same way for intervals and balls (with your implementation), except for the case of x == y
when x is y
. However, __nonzero__
behaves differently. (And, to be clear: I believe that your choice is the more sensible one, but I don't like the inconsistency.) Is that correct?
So if I understand you correctly, you'd be in favor of having methods
RealBall.lt
and leave the original methods untouched? I fear that a user may then mistakenly use the above example and get "surprising" results.
No, I guess I wasn't clear, sorry. Imho the fact that comparisons of sage objects (or at least of sage Elements) can fall back to comparing types at all is dreadful. I was saying (as an aside, not as a remark on your code) that I would be tempted to implement safer comparison methods in addition to your implementation of Python comparisons (and with the same semantics in the case where we compare balls to balls) because Sage comparisons are dangerous in general.
Perhaps we could overwrite
__richcmp__
in such a way that, if no coercions to balls are available, aNotImplementedError
is raised?
I don't know if it is possible, and I doubt it is a good idea to circumvent the coercion system even if it can lead to problems.
I do not understand what you mean. In my understanding, having the implication that identical objects are mathematically equal, should not lead to unexpected behavior within the rest of sage.
Forget it, I think I convinced myself that it was ok—as long as a ball is understood as representing a single real number, of course. (And never as a subset of the real line whose image by a given function needs to be bounded, say.)
For me, the behavior of
RealIntervalFieldElement
sage: a = RIF(1, 1) sage: from sage.rings.real_arb import RealBallField sage: RBF = RealBallField() sage: b = RBF(a) sage: a == a False sage: b == b Trueis very unexpected.
I wouldn't say that: after all, it does not hold true that for all x, y in [1,1], x=y...
After all, equality is usually assumed to be reflexive.
Yes, but that argument would be (almost) equally valid in the case of RBF(foo) == RBF(foo)
, and I don't think we want that to hold if RBF(foo)
is not a point ball!
(I will be offline for a few days. If anyone would like to take over the review, please feel free.)
comment:68 in reply to: ↑ 67 Changed 5 years ago by
Replying to mmezzarobba:
So I think we agree:
==
and!=
behave the same way for intervals and balls (with your implementation), except for the case ofx == y
whenx is y
. However,__nonzero__
behaves differently. (And, to be clear: I believe that your choice is the more sensible one, but I don't like the inconsistency.) Is that correct?
This is correct.
comment:69 in reply to: ↑ 67 ; followup: ↓ 71 Changed 5 years ago by
(Replying in several chunks due to trac timeouts when I try to reply in one go, sorry).
Replying to mmezzarobba:
No, I guess I wasn't clear, sorry. Imho the fact that comparisons of sage objects (or at least of sage Elements) can fall back to comparing types at all is dreadful.
I agree.
I was saying (as an aside, not as a remark on your code) that I would be tempted to implement safer comparison methods in addition to your implementation of Python comparisons (and with the same semantics in the case where we compare balls to balls) because Sage comparisons are dangerous in general.
We can always do that later because it would not change behaviour.
Perhaps we could overwrite
__richcmp__
in such a way that, if no coercions to balls are available, aNotImplementedError
is raised?I don't know if it is possible, and I doubt it is a good idea to circumvent the coercion system even if it can lead to problems.
So let's forget this idea.
comment:70 in reply to: ↑ 67 ; followup: ↓ 72 Changed 5 years ago by
Replying to mmezzarobba:
I do not understand what you mean. In my understanding, having the implication that identical objects are mathematically equal, should not lead to unexpected behavior within the rest of sage.
Forget it, I think I convinced myself that it was ok—as long as a ball is understood as representing a single real number, of course. (And never as a subset of the real line whose image by a given function needs to be bounded, say.)
I do not understand your point. If M
is a subset of the real line (represented by a ball) and f
is a function, I still believe that f(M)
can be computed by RealBall
.
I wouldn't say that: after all, it does not hold true that for all x, y in [1,1], x=y...
After all, equality is usually assumed to be reflexive.
Yes, but that argument would be (almost) equally valid in the case of
RBF(foo) == RBF(foo)
, and I don't think we want that to hold ifRBF(foo)
is not a point ball!
No, I do not want that.
For reference, I quote here from the Arb manual:
"Identical pointers are understood to give permission for algebraic simplification. This assumption is made to improve performance. For example, the call arb_mul(z, x, x, prec)
sets z
to a ball enclosing the set {t^2 : t ∈ x}
and not the (generally larger) set {tu : t ∈ x, u ∈ x}
."
comment:71 in reply to: ↑ 69 Changed 5 years ago by
Replying to cheuberg:
I was saying (as an aside, not as a remark on your code) that I would be tempted to implement safer comparison methods in addition to your implementation of Python comparisons (and with the same semantics in the case where we compare balls to balls) because Sage comparisons are dangerous in general.
We can always do that later because it would not change behaviour.
Yes.
comment:72 in reply to: ↑ 70 ; followups: ↓ 73 ↓ 74 ↓ 81 Changed 5 years ago by
 Reviewers set to Marc Mezzarobba
 Status changed from needs_review to positive_review
So despite what I said about being offline for a few days, I could have a look at your last changes before disappearing.
Replying to cheuberg:
Forget it, I think I convinced myself that it was ok—as long as a ball is understood as representing a single real number, of course. (And never as a subset of the real line whose image by a given function needs to be bounded, say.)
I do not understand your point. If
M
is a subset of the real line (represented by a ball) andf
is a function, I still believe thatf(M)
can be computed byRealBall
.
Ok, that was very badly expressed once again. Sorry about that, I'm trying to do too many things at once :(
.
All I was trying to say essentially is that if your think of balls as ranges, not approximate reals, then code like
def f(x, y): return xy def range(f, *args): return f(*args) unit_ball = RBF(0, rad=1) range(f, unit_ball, unit_ball)
looks correct, while it violates the rule about aliasing in arb that you quoted. I guess one could come up with more subtle examples with cached_function
s or additional layers of abstraction.
A few more quick comments:
 I think we will need
fmpr
again later on to implementrad()
; _an_element_
can be inherited, but perhaps this requires some of the coercion & category stuff I implemented on the other ticket, I don't remember;
(so several of your last changes will probably be reverted in the followup ticket)
 we should add a note about aliasing similar to that which you quoted to the python package's documentation too;
arb_to_mpfi
will probably crash sage if the ball's endpoints are not representable within the exponent range of MPFR FP numbers (but it is close to impossible to construct such balls at this point), usesig_on()
orsig_str()
(without_do_sig
) to prevent that; I personally find
return (lst is rt) or (arb_is_exact(...) and ... and ...)
more readable than your implementation of equality, but that's a matter of taste.
None of the above is really urgent or important and the current state of the code looks good to me, so I'm setting the ticket to positive review to avoid delaying it too much. But please feel free to make changes here if you want, and then I'll try to review them when I come back.
comment:73 in reply to: ↑ 72 ; followup: ↓ 75 Changed 5 years ago by
Replying to mmezzarobba:
So despite what I said about being offline for a few days, I could have a look at your last changes before disappearing.
Thank you.
 we should add a note about aliasing similar to that which you quoted to the python package's documentation too;
arb_to_mpfi
will probably crash sage if the ball's endpoints are not representable within the exponent range of MPFR FP numbers (but it is close to impossible to construct such balls at this point), usesig_on()
orsig_str()
(without_do_sig
) to prevent that; I personally find
return (lst is rt) or (arb_is_exact(...) and ... and ...)
more readable than your implementation of equality, but that's a matter of taste.None of the above is really urgent or important and the current state of the code looks good to me, so I'm setting the ticket to positive review to avoid delaying it too much. But please feel free to make changes here if you want, and then I'll try to review them when I come back.
While I think that your points above are valid, I do not want to make changes here any more.
I'll try to do that in a separate ticket. Moreover, I plan to adapt the ComplexBalls
(#17218) to the structure implemented here.
comment:74 in reply to: ↑ 72 ; followup: ↓ 76 Changed 5 years ago by
Replying to mmezzarobba:
A few more quick comments:
 I think we will need
fmpr
again later on to implementrad()
;
Anything fmpr
can be done with arf
module.
arb_to_mpfi
will probably crash sage if the ball's endpoints are not representable within the exponent range of MPFR FP numbers (but it is close to impossible to construct such balls at this point), usesig_on()
orsig_str()
(without_do_sig
) to prevent that;
Yes, unfortunately I haven't added any code yet to underflow or overflow gracefully when converting to MPFR. When developing, I deliberately wanted this to crash rather than fail silently.
I should probably fix arb_get_interval_mpfr
to set the endpoint to +/ 2^MPFR_MIN_EXP
on underflow and +/ inf
on overflow.
Rather than changing arf_get_mpfr
, I might add a separate arf_get_mpfr_overflow
that behaves as if you had computed an outofrange value within MPFR (setting MPFR overflow flags and so on).
comment:75 in reply to: ↑ 73 Changed 5 years ago by
Replying to cheuberg:
While I think that your points above are valid, I do not want to make changes here any more. I'll try to do that in a separate ticket. Moreover, I plan to adapt the
ComplexBalls
(#17218) to the structure implemented here.
Great, thanks for your work on these bindings! I'll do what I can to help.
comment:76 in reply to: ↑ 74 ; followup: ↓ 77 Changed 5 years ago by
Replying to fredrik.johansson:
Replying to mmezzarobba:
A few more quick comments:
 I think we will need
fmpr
again later on to implementrad()
;Anything
fmpr
can be done witharf
module.
What should we do to convert a mag_t
to an mpfr_t
? Using mag_get_fmpr
looked like the simplest method.
comment:77 in reply to: ↑ 76 ; followup: ↓ 78 Changed 5 years ago by
Replying to mmezzarobba:
Replying to fredrik.johansson:
Replying to mmezzarobba:
A few more quick comments:
 I think we will need
fmpr
again later on to implementrad()
;Anything
fmpr
can be done witharf
module.What should we do to convert a
mag_t
to anmpfr_t
? Usingmag_get_fmpr
looked like the simplest method.
arf_set_mag
, socalled because the arf
module depends on the mag
module and not vice versa.
comment:78 in reply to: ↑ 77 Changed 5 years ago by
Replying to fredrik.johansson:
arf_set_mag
, socalled because thearf
module depends on themag
module and not vice versa.
Ok, I missed that function, thanks!
comment:79 Changed 5 years ago by
Another question that we may want to think about is whether and how to expose exact (by which I mean ARF_PREC_EXACT
) operations. They would be useful to me, but perhaps being able to use them directly from cython is enough.
comment:80 Changed 5 years ago by
ARF_PREC_EXACT
is something of a hack, and not really safe in general. If your numbers are too large, you get undefined behavior.
comment:81 in reply to: ↑ 72 Changed 5 years ago by
Replying to mmezzarobba:
 we should add a note about aliasing similar to that which you quoted to the python package's documentation too;
This is now #17809.
arb_to_mpfi
will probably crash sage if the ball's endpoints are not representable within the exponent range of MPFR FP numbers (but it is close to impossible to construct such balls at this point), usesig_on()
orsig_str()
(without_do_sig
) to prevent that;
It was hard to provoke this with available functions, but it did crash sage. This is now #17811.
 I personally find
return (lst is rt) or (arb_is_exact(...) and ... and ...)
more readable than your implementation of equality, but that's a matter of taste.
This is #17810.
comment:82 Changed 5 years ago by
 Branch changed from u/cheuberg/17194arb to 195a6ee9c9efe97f24e806526cc3ec50c7b93363
 Resolution set to fixed
 Status changed from positive_review to closed
This probably does not matter as long as the bindings are not part of the sage coercion world, but I suppose then you'd want to handle precision in a similar way to
RealField
,RealIntervalField
etc. where it is an attribute of the parent (maybe with aRealBallField
type?).