cleaning reall and complex balls

[videlec]

We perform some cleaning

• move the conversion number field -> ball in the files relative to number fields (creation of methds _arb_ and _acb_ for that purpose)
• remove c++ clumsy dependency (ntl for number fields)
• with X bits precision -> with X bits of precision in string representation

(follow up #24318)
(this is part of the task ticket #24289)

CC: @mezzarobba

Component: basic arithmetic

Author: Vincent Delecroix

Branch: 3396c3e

Reviewer: Marc Mezzarobba, Travis Scrimshaw

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

[videlec]

Commit: c949ad7

[videlec]

Branch: u/vdelecroix/24285

[videlec]

New commits:

82dcce2	24285: cleaning in real_mpfi
1a89009	24285: clean ball fields
c949ad7	24285: now ball fields do not need C++

[sagetrac-git]

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

53370da

24285: fixing doctests

[sagetrac-git]

Changed commit from c949ad7 to 53370da

[mezzarobba]

comment:3

Quick comments (no time for a full review before several weeks): I like some of the refactoring you did, especially the part related to the interface with number fields and the NTL dependency. However, I feel that using a Cython class for the parent is a big step backward. The only advantage I see is that it should make accessing the precision field much faster, but there are other ways to do that—even storing a copy of the precision in every ball would be a better option IMO—and to date it wasn't much of an issue in my benchmarks anyway. Do you have other reasons for making this change? I have mixed feelings about the change to _repr_(), which may break users' doctests just to fix a cosmetic issue, but I won't fight against it.

[videlec]

comment:4

Replying to @mezzarobba:

Quick comments (no time for a full review before several weeks):

Thanks for having a quick look already. After reading your comments I would propose to:

• make this ticket only about moving number field stuff and C++ care
• open a more general ticket about "convergence between real intervals and balls"

I like some of the refactoring you did, especially the part related to the interface with number fields and the NTL dependency.

Indeed. If you try to use Cython in a pyx file and use balls you are in troubles because there is no proper distutils directive in the headers. Moreover, it make more sense implemented that way.

However, I feel that using a Cython class for the parent is a big step backward. The only advantage I see is that it should make accessing the precision field much faster, but there are other ways to do that—even storing a copy of the precision in every ball would be a better option IMO—and to date it wasn't much of an issue in my benchmarks anyway. Do you have other reasons for making this change?

Why would you prefer a Python class over an extension class inside a pyx file? To benefit from UniqueRepresentation?

The real point is: "How do you create a ball from nothing in Cython?". This is needed here in the code

def _arb_(self, R):
    cdef RealBall x = ---> what do I put here? <----

The way it is for interval is at least simple R._new(). And I just copied it for balls. I really dislike how people implementing balls just made their own coding conventions. I do not discuss the fact that they might be better (sometimes they do). Just the fact that they are different. The two interfaces should just be the same.

I have mixed feelings about the change to _repr_(), which may break users' doctests just to fix a cosmetic issue, but I won't fight against it.

Same comment applies: uniformity with real intervals. (BTW, a docstring is not any serious doctest breaking)

[tscrim]

comment:6

Replying to @videlec:

Replying to @mezzarobba:

However, I feel that using a Cython class for the parent is a big step backward. The only advantage I see is that it should make accessing the precision field much faster, but there are other ways to do that—even storing a copy of the precision in every ball would be a better option IMO—and to date it wasn't much of an issue in my benchmarks anyway. Do you have other reasons for making this change?

Why would you prefer a Python class over an extension class inside a pyx file? To benefit from UniqueRepresentation?

Yes, and there are real benefits. The first is that by using a custom cache, you are making a strong reference to each parent. With UniqueRepresentation, it is only a weak reference, so you do not get the parents nailed in memory. It also is far less clean code:

• You lose localization of the input and code; in particular, you either have duplicate documentation or either the creation function or object is lacking in documentation.

• (Essentially) Duplicate code:

  • You have to maintain the cache.
  • You do not have the guarantees of uniqueness unless you handle pickles, __copy__, etc.
  • You have to handle pickles.
  • You have to handle comparisons.

• Comparisons are now slower because they are not by id.

So I also think removing the UniqueRepresentation inheritance is a big step backwards. I do not see why you need the parent to be a cdef class. This does not seem to be a common optimization of creating an uninitialized ball, so just put <RealBall> (RealBall.__new__(RealBall, R)) the place you need it. Yes, readability suffers slightly, but I think that is a marginal concern for code that is being optimized to the extent that you care about creating an uninitialized object in contrast to RealBall(R).

[mezzarobba]

comment:7

Also, the category framework works better when the parents are plain Python classes. More generally, I thought there was a consensus that parents should always be Python classes, except perhaps in a few very special cases.

Regarding the convergence between interval fields and ball fields, I am not really convinced that they serve the same purpose and need to be interchangeable, though of course it is better if they are somewhat compatible. And I don't think we made up our own coding conventions when implementing the arb interface. Rather, the implementation of MPFI intervals is quite outdated (and their behavior sometimes plainly incorrect), while the arb interface is more modern and more careful about giving correct results—at the price that some things that rely on questionable behavior elsewhere in sage simply don't work.

The first step to improve the situation IMO would be to modernize the implementation of intervals. I have an old branch where I started doing that at u/mmezzarobba/wip/intervals, but I had to stop because it broke things elsewhere due to a deeper issue (probably something related to comparisons, as usual, but I don't remember for sure).

[sagetrac-git]

Changed commit from 53370da to 86c4400

[sagetrac-git]

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

d9469bc	24285: fix string representation
75bc752	24285: simplify real balls
86c4400	24285: now ball fields do not need C++

[videlec]

comment:9

Please move your interesting balls comments on #24289. This ticket stands for moving the quadratic number field conversion.

[videlec]

comment:10

Replying to @mezzarobba:

Regarding the convergence between interval fields and ball fields, I am not really convinced that they serve the same purpose and need to be interchangeable, though of course it is better if they are somewhat compatible.

You really need to explain me what is the difference in usage then.

The first step to improve the situation IMO would be to modernize the implementation of intervals. I have an old branch where I started doing that at u/mmezzarobba/wip/intervals, but I had to stop because it broke things elsewhere due to a deeper issue (probably something related to comparisons, as usual, but I don't remember for sure).

Indeed. It might be more reasonable starting in this direction. If you make your branch alive again I can review it.

[videlec]

comment:11

And patchbots are happy!

[tscrim]

comment:12

Since RealBallField is a UniqueRepresentation it shouldn't need a custom __reduce__ (left over from the previous changes?). Modulo that, I am happy. Marc?

[videlec]

comment:13

Right! Good catch.

[sagetrac-git]

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

9d2298f

24285: `__reduce__` not needed on RealBallField

[sagetrac-git]

Changed commit from 86c4400 to 9d2298f

[mezzarobba]

comment:15

Replying to @tscrim:

Modulo that, I am happy. Marc?

Looks reasonable to me, but I only skimmed through the code.

Why remove the ability to construct a complex ball from a machine integer? Since this is something supported by arb, ComplexBall.__cinit__ does look like the most natural place for it to me.

Typos: “that the map go through”, ”cancellationss”.

Vincent: Yes, I'll try to see if I can do something with my old branch(es) about intervals, post my comments on the other ticket, etc. ...but not now.

[sagetrac-git]

Changed commit from 9d2298f to 27ac94b

[sagetrac-git]

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

27ac94b

24285: too many s

[videlec]

comment:17

Replying to @mezzarobba:

Replying to @tscrim:

Modulo that, I am happy. Marc?

Why remove the ability to construct a complex ball from a machine integer?

It does work through int -> ZZ when you call RBF(14r) which is just fine. What would be the point of having one more special case in the init for something that is anyway slow because of Python and will be useless in Python3?

Since this is something supported by arb, ComplexBall.__cinit__ does look like the most natural place for it to me.

Note that it used to be in __init__ and not __cinit__. I does think that __cinit__ should do the minimal job (memory allocation) and not set any value. If the user wants quick initialization she uses arb functions directly as in

cdef RealBall a = RealBall.__new__(RealBall)
a._parent = TheParent      # can possibly move in __cinit__?
arb_set_si(a.value, 13)

Typos: “that the map go through”

is that a typo?

”cancellationss”.

right. Fixed in 27ac94b

[tscrim]

comment:18

Replying to @videlec:

Replying to @mezzarobba:

Typos: “that the map go through”

is that a typo?

Should be "that the map goes through".

[sagetrac-git]

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

3396c3e

24285: map go -> map goes

[sagetrac-git]

Changed commit from 27ac94b to 3396c3e

[videlec]

comment:20

thanks

[mezzarobba]

comment:21

Replying to @videlec:

It does work through int -> ZZ when you call RBF(14r) which is just fine. What would be the point of having one more special case in the init for something that is anyway slow because of Python and will be useless in Python3?

I'm not sure it needs to be that slow when called from Cython code, but I admit I didn't measure. I don't really care anyway.

Note that it used to be in __init__ and not __cinit__. I does think that __cinit__ should do the minimal job (memory allocation) and not set any value.

Yes, that's what I meant, sorry. As far as I can say, the general logic how how constructors are organized in the arb interface is that (i) __cinit__() does basic initialization, (ii) __init__() wraps the C-level intializers provided by arb (and should be reasonably fast), and (iii) _element_constructor_(), conversion morphisms and the like take care of everything else. This organization has pros and cons, but it sort of makes sense to me.

[tscrim]

comment:23

I am going to consider that as a positive review.

[tscrim]

Reviewer: Marc Mezzarobba, Travis Scrimshaw

[videlec]

comment:24

Thanks Travis and Marc. Indeed, remaining remarks mostly concern the parts of the ticket that have been moved to #24289.

[vbraun]

Changed branch from u/vdelecroix/24285 to 3396c3e

[mezzarobba]

Changed commit from 3396c3e to none

[mezzarobba]

comment:26

Actually this change:

-        elif isinstance(other, number_field.NumberField_quadratic):
[...]
+        from sage.rings.number_field.number_field_base import is_NumberField
+        if is_NumberField(other):
             emb = other.coerce_embedding()
-            if emb is not None:
-                return self.has_coerce_map_from(emb.codomain())
+            return emb is not None and self.has_coerce_map_from(emb.codomain())

is not correct, because the ball constructor only accepts quadratic NF elements. NF elements of higher degree can be converted to complex balls, but only through CLF or CIF.

Fixed at #24621.