Opened 2 years ago

Closed 2 years ago

Last modified 22 months ago

#24285 closed enhancement (fixed)

cleaning reall and complex balls

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.2
Component: basic arithmetic Keywords:
Cc: ​mmezzarobba Merged in:
Authors: Vincent Delecroix Reviewers: Marc Mezzarobba, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 3396c3e (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

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)

Change History (26)

comment:1 Changed 2 years ago by vdelecroix

  • Branch set to u/vdelecroix/24285
  • Commit set to c949ad7d66bdbdd8e6a8a818f2b0b86e957d2e69
  • Status changed from new to needs_review

New commits:

82dcce224285: cleaning in real_mpfi
1a8900924285: clean ball fields
c949ad724285: now ball fields do not need C++

comment:2 Changed 2 years ago by git

  • Commit changed from c949ad7d66bdbdd8e6a8a818f2b0b86e957d2e69 to 53370da4caa6f0c321658c6417b67c66932f94d8

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

53370da24285: fixing doctests

comment:3 follow-up: Changed 2 years ago by mmezzarobba

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.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 2 years ago by vdelecroix

Replying to mmezzarobba:

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)

comment:5 Changed 2 years ago by vdelecroix

  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:6 in reply to: ↑ 4 Changed 2 years ago by tscrim

Replying to vdelecroix:

Replying to mmezzarobba:

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).

comment:7 follow-up: Changed 2 years ago by mmezzarobba

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).

comment:8 Changed 2 years ago by git

  • Commit changed from 53370da4caa6f0c321658c6417b67c66932f94d8 to 86c440049e2d537adbb63ae6558cd4ef0496034e

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

d9469bc24285: fix string representation
75bc75224285: simplify real balls
86c440024285: now ball fields do not need C++

comment:9 Changed 2 years ago by vdelecroix

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

comment:10 in reply to: ↑ 7 Changed 2 years ago by vdelecroix

Replying to mmezzarobba:

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.

comment:11 Changed 2 years ago by vdelecroix

And patchbots are happy!

comment:12 follow-up: Changed 2 years ago by tscrim

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

comment:13 Changed 2 years ago by vdelecroix

Right! Good catch.

comment:14 Changed 2 years ago by git

  • Commit changed from 86c440049e2d537adbb63ae6558cd4ef0496034e to 9d2298fe35272320158f3e96cfc384067611ef59

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

9d2298f24285: __reduce__ not needed on RealBallField

comment:15 in reply to: ↑ 12 Changed 2 years ago by mmezzarobba

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.

comment:16 Changed 2 years ago by git

  • Commit changed from 9d2298fe35272320158f3e96cfc384067611ef59 to 27ac94bd2ff8b1e406e4da6ddb0bf93cb9b25f13

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

27ac94b24285: too many s

comment:17 follow-ups: Changed 2 years ago by vdelecroix

Replying to mmezzarobba:

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

comment:18 in reply to: ↑ 17 Changed 2 years ago by tscrim

Replying to vdelecroix:

Replying to mmezzarobba:

Typos: “that the map go through”

is that a typo?

Should be "that the map goes through".

comment:19 Changed 2 years ago by git

  • Commit changed from 27ac94bd2ff8b1e406e4da6ddb0bf93cb9b25f13 to 3396c3e648d6e56c6f87018a7ec808a26032d945

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

3396c3e24285: map go -> map goes

comment:20 Changed 2 years ago by vdelecroix

thanks

comment:21 in reply to: ↑ 17 Changed 2 years ago by mmezzarobba

Replying to vdelecroix:

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.

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

comment:22 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:23 Changed 2 years ago by tscrim

  • Reviewers set to Marc Mezzarobba, Travis Scrimshaw
  • Status changed from needs_review to positive_review

I am going to consider that as a positive review.

comment:24 Changed 2 years ago by vdelecroix

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

comment:25 Changed 2 years ago by vbraun

  • Branch changed from u/vdelecroix/24285 to 3396c3e648d6e56c6f87018a7ec808a26032d945
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:26 Changed 22 months ago by mmezzarobba

  • Commit 3396c3e648d6e56c6f87018a7ec808a26032d945 deleted

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.

Last edited 22 months ago by mmezzarobba (previous) (diff)
Note: See TracTickets for help on using tickets.