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: sage-6.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 cheuberg)

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 cheuberg

  • Status changed from new to needs_review

comment:2 Changed 5 years ago by fredrik.johansson

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 a RealBallField type?).

comment:3 Changed 5 years ago by fredrik.johansson

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 cheuberg

The question is how the relation to RealIntervalField and ComplexIntervalField should be.

comment:5 Changed 5 years ago by mmezzarobba

  • Cc mmezzarobba added

comment:6 Changed 5 years ago by git

  • Commit changed from 4a343229803c8b3dafec47fb13cb12e49e51bdb6 to 55bdc8b67439f5742c8a9fed70dfc795262377c2

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

eb8b584Trac #17194: Fix typo in one doctest
a6cb2caTrac #17194: declare return types
55bdc8bTrac #17194: mark all doctests in real_arb.pyx as "optional - arb"

comment:7 Changed 5 years ago by git

  • Commit changed from 55bdc8b67439f5742c8a9fed70dfc795262377c2 to 14f599fe6469378d9474fb52a71caabbcfe7eb52

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

14f599fTrac #17194: rename attribute precision to _precision_

comment:8 Changed 5 years ago by cheuberg

As a follow-up, minimal bindings for the acb type (complex balls) are provided in #17218.

comment:9 Changed 5 years ago by fredrik.johansson

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 git

  • Commit changed from 14f599fe6469378d9474fb52a71caabbcfe7eb52 to 11df4b42506bfeaa63796e6dff90085353f8a730

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

11df4b4Trac #17194: Clarify that precision >=2 is required

comment:11 Changed 5 years ago by git

  • Commit changed from 11df4b42506bfeaa63796e6dff90085353f8a730 to 141aca0b89aa89b5e4a810b219d1f6fb8c94f4f8

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

141aca0Trac #17194: >2 should have been >= 2 in previous commit

comment:12 follow-up: Changed 5 years ago by jdemeyer

  • 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 follow-up: Changed 5 years ago by 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.

comment:14 Changed 5 years ago by jdemeyer

And Arb should probably be renamed to RealBallElement.

comment:15 in reply to: ↑ 12 ; follow-ups: Changed 5 years ago by cheuberg

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 trying 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 ; follow-up: Changed 5 years ago by jdemeyer

Replying to cheuberg:

What alternative do you recommend?

Try the import and catch ImportError. Would that work?

comment:17 Changed 5 years ago by fredrik.johansson

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 ; follow-up: Changed 5 years ago by jdemeyer

Replying to cheuberg:

Do we want to stress the difference between RealBallElement and RealIntervalFieldElement?

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 about RealBallFieldElement for the current Arb?

I slightly prefer the shorter name RealBallElement (like RealDoubleElement), but I can live with RealBallFieldElement.

comment:19 Changed 5 years ago by git

  • Commit changed from 141aca0b89aa89b5e4a810b219d1f6fb8c94f4f8 to a0afebd76117cdaeae6d8a408a2b1071ec3b8cd2

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

dc23341Trac #17194: Renamed 'Arb' to 'RealBallElement'
a0afebdTrac #17194: Replace is_package_installed by try ... except ImportError

comment:20 in reply to: ↑ 16 Changed 5 years ago by cheuberg

Replying to jdemeyer:

Try the import and catch ImportError. Would that work?

Yes, it works, thanks.

comment:21 in reply to: ↑ 18 Changed 5 years ago by cheuberg

  • Description modified (diff)

Replying to jdemeyer:

I slightly prefer the shorter name RealBallElement (like RealDoubleElement), but I can live with RealBallFieldElement.

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 jdemeyer

  • 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 git

  • Commit changed from a0afebd76117cdaeae6d8a408a2b1071ec3b8cd2 to a194938dca3bed5878bfa85ab94d0e52425298d4

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

0ffa347Trac #17194: Implemented RealBallField
dd8933aTrac #17194: Implement RealBallElement._repr_
a194938Trac #17194: Mark all doctests as optional - arb

comment:24 in reply to: ↑ 13 Changed 5 years ago by cheuberg

  • 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 cheuberg

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 fredrik.johansson

The radius could just be printed with 3-5 digits (it is never a high precision number).

comment:27 follow-up: Changed 5 years ago by fredrik.johansson

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 cheuberg

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 git

  • Commit changed from a194938dca3bed5878bfa85ab94d0e52425298d4 to 1ce0e90678ad06f6004be7fc8eb47f58ed9184ac

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

fbcac86Merge remote-tracking branch 'trac/develop' into rings/real_arb
e1683f0Trac #17194: Changes required after merge of #17267
6ef90e0Trac #17194: _coerce_map_from_ as cpdef instead of def
1ce0e90Trac #17194: use weak_cached_function for RealBallField

comment:30 Changed 5 years ago by cheuberg

The changes in #17267 were incompatible with this ticket. Thus, I merged 6.5.beta1 (containing #17267) and adapted the code.

I used this occasion to change to minor other issues.

comment:31 Changed 5 years ago by git

  • Commit changed from 1ce0e90678ad06f6004be7fc8eb47f58ed9184ac to f4393a997a6bdee1d5765fb5f7e8c0a9dff61e35

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

f4393a9Trac #17194: remove one obsolete import

comment:32 follow-ups: Changed 5 years ago by mmezzarobba

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 just RealBall? 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 like interval, or perhaps _interval since in the long run it should be accessible through coercion/conversion to RealIntervalField (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 git

  • Commit changed from f4393a997a6bdee1d5765fb5f7e8c0a9dff61e35 to 80850af551dcc6634c322e0f023515ecbe14d2d8

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

9705e09Trac #17194: Rename RealBallElement to RealBall
80850afTrac #17194: Rename RealBall.RealIntervalFieldElement -> RealBall._interval

comment:34 Changed 5 years ago by git

  • Commit changed from 80850af551dcc6634c322e0f023515ecbe14d2d8 to a45f691daf51970311b43190496104a774d6518b

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

ddf7e9fTrac #17194: Fix one Docstring
a45f691Trac #17194: Rename parameter "value" of RealBall.__init__ to "x"

comment:35 in reply to: ↑ 32 ; follow-up: Changed 5 years ago by cheuberg

  • Description modified (diff)

Replying to mmezzarobba:

  • Why call the element class RealBallElement and not just RealBall? 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 cheuberg

  • 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 git

  • Commit changed from a45f691daf51970311b43190496104a774d6518b to 8802939c14030e34ad81d7a6acea00c5a8e71672

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

0eb70b0Trac #17688: Upgrade arb spkg to 2.5.0
50eeecbMerge branch 'u/cheuberg/packages/arb-2.5' (#17688) of git://trac.sagemath.org/sage into rings/real_arb
6c41d27Trac #17194: Remove coercion from RealIntervalField
8802939Trac #17194: Use arb's arg_get_str for RealBall._repr_

comment:38 in reply to: ↑ 32 Changed 5 years ago by cheuberg

  • Milestone changed from sage-6.4 to sage-6.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 ; follow-up: Changed 5 years ago by mmezzarobba

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 ; follow-up: Changed 5 years ago by 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 sage-devel (<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 RealBallFields 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 mmezzarobba

Incidentally, with the current (non-cpdef, 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 follow-up: Changed 5 years ago by mmezzarobba

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 git

  • Commit changed from 8802939c14030e34ad81d7a6acea00c5a8e71672 to ec8d7062c18fde4ffa52191e866cb465a76b751f

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

676f8a2Make RealBallField a plain Python class
a61566fMinor doc improvements
7fa8fd1Modernize [PC]ython style
b7eaa0esig_on/sig_off
0aaf188Clarify that the precision is a default precision
5be8dfaclarify(?) error message
3e1ed11Trac #17194: Mark one doctest as #optional - arb
ec8d706Trac #17194: Language correction

comment:44 in reply to: ↑ 42 ; follow-up: Changed 5 years ago by cheuberg

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 one-line patches and pushed it on the ticket.

From the discussion on sage-devel, 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 ; follow-up: Changed 5 years ago by cheuberg

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 sage-devel (<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 RealBallFields can have elements balls with any radius and any midpoint "precision". Is that correct?

Yes.

comment:46 in reply to: ↑ 44 ; follow-up: Changed 5 years ago by mmezzarobba

Replying to cheuberg:

From the discussion on sage-devel, 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 cheuberg

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 mmezzarobba

Another question: is there a reason to declare precision as unsigned in your code? The precision parameter of arb (and mpfr) functions is signed.

Last edited 5 years ago by mmezzarobba (previous) (diff)

comment:49 Changed 5 years ago by cheuberg

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 fredrik.johansson

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 ; follow-up: Changed 5 years ago by 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 RealBallFields 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 100-bit 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 follow-up: Changed 5 years ago by 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).

comment:53 in reply to: ↑ 51 Changed 5 years ago by cheuberg

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 RealBallFields 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 a RealBall resulting from a 100-bit 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),

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 ; follow-up: Changed 5 years ago by 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.

comment:55 in reply to: ↑ 54 Changed 5 years ago by mmezzarobba

  • Branch changed from u/cheuberg/rings/real_arb to u/mmezzarobba/17194-arb
  • 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 RealBalls. See u/mmezzarobba/arb. But that's for another ticket, unless you feel like reviewing the first few commits here.


New commits:

555df20Revert "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
Last edited 5 years ago by mmezzarobba (previous) (diff)

comment:56 follow-up: Changed 5 years ago by mmezzarobba

Follow-up: #17786.

Last edited 5 years ago by mmezzarobba (previous) (diff)

comment:57 in reply to: ↑ 56 ; follow-up: Changed 5 years ago by cheuberg

Replying to mmezzarobba:

Follow-up: #11786.

The follow-up 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 mmezzarobba

Replying to cheuberg:

The follow-up is actually #17786.

Oops, thanks. I will edit my other post to avoid any confusion.

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.

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 cheuberg

  • Branch changed from u/mmezzarobba/17194-arb to u/cheuberg/17194-arb

comment:60 Changed 5 years ago by git

  • Commit changed from ce51f81a120e229bc082a16d03b855607335fba9 to 5e41fcbc08bf0e04ff5a41ea60866568c5bc8588

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

5e41fcbTrac #17194: remove imports of fmpr

comment:61 Changed 5 years ago by cheuberg

  • Authors changed from Clemens Heuberger to Clemens Heuberger, Marc Mezzarobba
  • 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 cross-reviewed 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 git

  • Commit changed from 5e41fcbc08bf0e04ff5a41ea60866568c5bc8588 to 195a6ee9c9efe97f24e806526cc3ec50c7b93363

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

4e7ec46Trac #17194: is_zero, is_nonzero, is_exact
9e26152Trac #17194: Implement comparisons
1196b57Trac #17194: Fix RealBallField._an_element_
195a6eeTrac #17194: Fix two broken references

comment:63 follow-up: Changed 5 years ago by cheuberg

The semantics of != differ from those of real intervals: A ball is considered non-zero if it does not contain 0.

So, != is not not ==.

I prefer this for two reasons:

  • We are wrapping arb here, and arb 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 cheuberg

  • Status changed from needs_work to needs_review
  • Work issues is_zero, is_nonzero, __richcmp__ deleted

comment:65 in reply to: ↑ 63 ; follow-up: Changed 5 years ago by mmezzarobba

Replying to cheuberg:

The semantics of != differ from those of real intervals: A ball is considered non-zero if it does not contain 0.

So, != is not not ==.

Hmm, if I read correctly, that's also how comparisons of RealIntervalFieldElements work. Can you please be more specific about the difference you're seeing? For me the only difference is that, in the case of RealIntervalFieldElements, __nonzero__ is not consistent with != 0.

I prefer this for two reasons:

  • We are wrapping arb here, and arb 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: If a is b, then a==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.

Last edited 5 years ago by mmezzarobba (previous) (diff)

comment:66 in reply to: ↑ 65 ; follow-up: Changed 5 years ago by cheuberg

Replying to mmezzarobba:

Replying to cheuberg:

The semantics of != differ from those of real intervals: A ball is considered non-zero if it does not contain 0.

So, != is not not ==.

Hmm, if I read correctly, that's also how comparisons of RealIntervalFieldElements work. Can you please be more specific about the difference you're seeing? For me the only difference is that, in the case of RealIntervalFieldElements, __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 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!

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: If a is b, then a==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 ; follow-ups: Changed 5 years ago by mmezzarobba

Replying to cheuberg:

Replying to mmezzarobba:

Replying to cheuberg:

The semantics of != differ from those of real intervals: A ball is considered non-zero if it does not contain 0.

So, != is not not ==.

Hmm, if I read correctly, that's also how comparisons of RealIntervalFieldElements work. Can you please be more specific about the difference you're seeing? For me the only difference is that, in the case of RealIntervalFieldElements, __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, a NotImplementedError 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
True

is 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 cheuberg

Replying to mmezzarobba:

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?

This is correct.

comment:69 in reply to: ↑ 67 ; follow-up: Changed 5 years ago by cheuberg

(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, a NotImplementedError 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 ; follow-up: Changed 5 years ago by cheuberg

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 if RBF(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 mmezzarobba

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 ; follow-ups: Changed 5 years ago by mmezzarobba

  • 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) and f is a function, I still believe that f(M) can be computed by RealBall.

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 x-y
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_functions or additional layers of abstraction.

A few more quick comments:

  • I think we will need fmpr again later on to implement rad();
  • _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 follow-up 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), use sig_on() or sig_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 ; follow-up: Changed 5 years ago by cheuberg

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), use sig_on() or sig_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 ; follow-up: Changed 5 years ago by fredrik.johansson

Replying to mmezzarobba:

A few more quick comments:

  • I think we will need fmpr again later on to implement rad();

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), use sig_on() or sig_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 out-of-range value within MPFR (setting MPFR overflow flags and so on).

Last edited 5 years ago by fredrik.johansson (previous) (diff)

comment:75 in reply to: ↑ 73 Changed 5 years ago by mmezzarobba

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.

Last edited 5 years ago by mmezzarobba (previous) (diff)

comment:76 in reply to: ↑ 74 ; follow-up: Changed 5 years ago by mmezzarobba

Replying to fredrik.johansson:

Replying to mmezzarobba:

A few more quick comments:

  • I think we will need fmpr again later on to implement rad();

Anything fmpr can be done with arf module.

What should we do to convert a mag_t to an mpfr_t? Using mag_get_fmprlooked like the simplest method.

comment:77 in reply to: ↑ 76 ; follow-up: Changed 5 years ago by fredrik.johansson

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 implement rad();

Anything fmpr can be done with arf module.

What should we do to convert a mag_t to an mpfr_t? Using mag_get_fmprlooked like the simplest method.

arf_set_mag, so-called because the arf module depends on the mag module and not vice versa.

comment:78 in reply to: ↑ 77 Changed 5 years ago by mmezzarobba

Replying to fredrik.johansson:

arf_set_mag, so-called because the arf module depends on the mag module and not vice versa.

Ok, I missed that function, thanks!

comment:79 Changed 5 years ago by mmezzarobba

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 fredrik.johansson

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 cheuberg

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), use sig_on() or sig_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 vbraun

  • Branch changed from u/cheuberg/17194-arb to 195a6ee9c9efe97f24e806526cc3ec50c7b93363
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.