Opened 4 years ago

Closed 4 years ago

#19152 closed enhancement (fixed)

{Real,Complex}Ball: Miscellaneous fixes and improvements

Reported by: mmezzarobba Owned by:
Priority: major Milestone: sage-6.10
Component: numerical Keywords:
Cc: cheuberg Merged in:
Authors: Marc Mezzarobba Reviewers: Clemens Heuberger
Report Upstream: N/A Work issues:
Branch: 0a04e62 (Commits) Commit: 0a04e627728c37d334c3576eb5243facebee0e80
Dependencies: #19063 Stopgaps:

Description (last modified by mmezzarobba)

Mostly for compatibility with RIF/CIF.

Change History (75)

comment:1 Changed 4 years ago by mmezzarobba

  • Branch set to u/mmezzarobba/19152-arb-misc
  • Commit set to d0e948b8e4c66eed2226178ffa1b0f1b4957f40c
  • Status changed from new to needs_review

Last 10 new commits:

f9e13d6ComplexBall: conversions to ZZ, RR, RIF, CC, CIF
4a7b5c4ComplexBallField: faster access to real field
b4e82aa{Real,Complex}Ball: implement conversions to QQ
5e1cbdaComplexBall: containment and overlapping predicates
d44c1b1ComplexBall: abs(), arg()
62af563RealBall: implement abs()
448bf24{Real,Complex}Ball: implement {above,below}_abs()
0196f69ComplexBallField: minor documentation improvements
f2d093eRealBall: upper(), lower(), endpoints()
d0e948b{Real,Complex}Ball: change __nonzero__ to check for syntactic !=0

comment:2 Changed 4 years ago by mmezzarobba

  • Dependencies set to #19063

comment:3 Changed 4 years ago by git

  • Commit changed from d0e948b8e4c66eed2226178ffa1b0f1b4957f40c to cfdffddc92d64218a2766b7c6c86117190af7e68

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

cfdffdd{Real,Complex}Ball: contains_zero()

comment:4 Changed 4 years ago by git

  • Commit changed from cfdffddc92d64218a2766b7c6c86117190af7e68 to a53da3999a233cf41b11f7e2bdeae96bd8ca7ced

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

a53da39{Real,Complex}Ball: <<, >> (of a ball by an integer)

comment:5 Changed 4 years ago by mmezzarobba

  • Description modified (diff)

comment:6 Changed 4 years ago by git

  • Commit changed from a53da3999a233cf41b11f7e2bdeae96bd8ca7ced to 50f4459b65e51e2539347fde5153863f1d1ef178

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

63e4b76{Real,Complex}Ball: add some tests
0db149f{Real,Complex}Ball: rename abs() to __abs__()
1ee9919RealBall: implement min(), max()
fa2c0b7{Real,Complex}Ball: __hash__()
1741bce{Real,Complex}Ball: bind arb's add_error
feb8384Implement ComplexBall.rad()
a724343real_arb: lone (and misplaced) sig_on()
9e3b251real_arb: work around weakness of arb_set_interval_mpfr
f30820eRealBall: union()
50f4459Implement CBF.complex_field()

comment:7 Changed 4 years ago by mmezzarobba

Rebased and extended.

comment:8 Changed 4 years ago by git

  • Commit changed from 50f4459b65e51e2539347fde5153863f1d1ef178 to 9097cd41160736353232c64351c010ef371c2d23

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

560fe0b{Real,Complex}Ball: add some tests
a1d9086{Real,Complex}Ball: rename abs() to __abs__()
0a8efddRealBall: implement min(), max()
8049aad{Real,Complex}Ball: __hash__()
e9700cb{Real,Complex}Ball: bind arb's add_error
761d53fImplement ComplexBall.rad()
ad628b8real_arb: lone (and misplaced) sig_on()
030652areal_arb: work around weakness of arb_set_interval_mpfr
3aa26c6RealBall: union()
9097cd4Implement CBF.complex_field()

comment:9 Changed 4 years ago by git

  • Commit changed from 9097cd41160736353232c64351c010ef371c2d23 to c802d70f17a70aff9f5600ae2d3a0f035eeafdd3

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

7b67b5e{Real,Complex}Ball: add some tests
202f64c{Real,Complex}Ball: rename abs() to __abs__()
8f1ddafRealBall: implement min(), max()
77f1174{Real,Complex}Ball: __hash__()
0d312da{Real,Complex}Ball: bind arb's add_error
11649d7Implement ComplexBall.rad()
638607ereal_arb: lone (and misplaced) sig_on()
eaf70fareal_arb: work around weakness of arb_set_interval_mpfr
23c34f8RealBall: union()
c802d70Implement CBF.complex_field()

comment:10 Changed 4 years ago by cheuberg

  • Milestone changed from sage-6.9 to sage-6.10

As discussed in #17220, the experimental decorator should be removed.

comment:11 Changed 4 years ago by cheuberg

  • Branch changed from u/mmezzarobba/19152-arb-misc to u/cheuberg/19152-arb-misc

comment:12 follow-ups: Changed 4 years ago by cheuberg

  • Commit changed from c802d70f17a70aff9f5600ae2d3a0f035eeafdd3 to 16a04a07c63349d3792ed8c538a7d60e5adfde5e
  • Reviewers set to Clemens Heuberger
  • Status changed from needs_review to needs_work

I read the modifications and have the following comments.

  1. arf.pxd: You define arf_rnd_t as an enum and rely on the fact that the numeric values of ARF_RND_DOWN etc. will never change in the C header files. However, there, the values are macros, so I do not know whether we can safely assume the values never to change.
  2. polynomial_element.pyx:
    • I am very surprised to see this change here in an arb ticket.
    • The documentation of is_gen states: "Important - this function doesn’t return True if self equals the generator; it returns True if self is the generator." I.e., the outcome differs from the old behaviour.
  3. power_series_ring_element.pyx: I am surprised to see this change here in an arb ticket, but I guess that it will not do any harm.
  4. RealBall.__hash__: I do not like the fact that the fractional part of the midpoint is ignored completely:
    sage: hash(RBF(3/4))
    1048577
    sage: hash(RBF(5/8))
    1048577
    sage: hash(RBF(7/8))
    1048577
    
  5. RealBall.upper, RealBall.lower, RealBall.endpoints: missing INPUT: and OUTPUT: sections, document and test parameter rnd
  6. RealBall.add_error: missing INPUT: and OUTPUT: sections
  7. RealBall.is_nonzero: The note section is hard to understand because __nonzero__ and is_zero should do very different things.
  8. RealBall.__lshift__, RealBall.__rshift__: why is the first parameter called val instead of self and how can it happen that it is not a RealBall if this is a method of RealBall?
  9. ComplexBall._is_atomic: self.is_real() or self.imag().is_zero() seems to be redundant. Do you want to say self.real().is_zero() instead?
  10. ComplexBall.add_error: missing INPUT: and OUTPUT: sections
  11. ComplexBall.is_nonzero: The note section is hard to understand because __nonzero__ and is_zero should do very different things.
  12. ComplexBall.__lshift__, ComplexBall.__rshift__: why is the first parameter called val instead of self and how can it happen that it is not a ComplexBall if this is a method of ComplexBall?

New commits:

a725299Trac #19152: fix error in module description
ada2759Trac #19152: Fix hyperlinks in documentation
32cc12aTrac #19152: add SEEALSO blocks
617b9a7Trac #19152: fix typo and spacing
59ab3cbTrac #19152: fix _repr_option
16a04a0Trac #19152: fix docstring

comment:13 in reply to: ↑ 12 ; follow-up: Changed 4 years ago by mmezzarobba

Replying to cheuberg:

I read the modifications and have the following comments.

Thank you very much for your comments. I'm a bit busy with other things right now, but I'll try to have a look next Friday, if that's okay with you.

comment:14 in reply to: ↑ 13 Changed 4 years ago by mmezzarobba

Replying to mmezzarobba:

Thank you very much for your comments. I'm a bit busy with other things right now, but I'll try to have a look next Friday, if that's okay with you.

I couldn't do much due to network problems, sorry. Hopefully Monday...

comment:15 follow-ups: Changed 4 years ago by jdemeyer

I am having a quick look though this branch, mostly to check for technicalities. It's a lot, so I'm not saying that you really must fix all these for positive_review.

  1. The naming of the files is inconsistent: why real_arb and complex_ball_acb?
  1. You can remove the include_dirs=[SAGE_INC + '/flint']) from src/module_list.py.
  1. Ideally, the type declarations in the arb .pxd files should be in a different file: try to follow the model of src/sage/libs/gmp for example. Then you should add # distutils: libraries = arb to the files with library functions (i.e. not types).
  1. The __hash__ for RBF is bad:
    sage: from sage.rings.real_arb import RBF
    sage: hash(RBF(0.6))
    1048577
    sage: hash(RBF(0.7))
    1048577
    sage: hash(RBF(0.8))
    1048577
    sage: hash(RBF(0.9))
    1048577
    
  1. For CBF.__hash__, what's the point of >> 7?
  1. This is a very strange way to handle exceptions, what's wrong with raise?
            if arf_get_mpfr(rad.value, tmp, GMP_RNDN):
                abort()
    
  1. Replace the GMP_RND macros by MPFR_RND.
  1. You can replace
    cdef extern from "math.h":
        long lrint(double)
    

by from libc.math cimport lrint (although I would prefer to not use it in the hash).

  1. What's with this comment?
                    # FIXME: RBF is not even associative, but CompletionFunctor
                    # only works with rings, and other coercion features require
                    # methods like is_integral_domain() to be defined
                    category=category or sage.categories.fields.Fields().Infinite())
    

Your class models a mathematical field (real or complex numbers), so I don't see the problem. How would you want to fix this?

  1. Now that all this is standard, can you add RBF, RealBallField, CBF and ComplexBallField to the global namespace and then remove all doctest lines of the form from sage.rings.real_arb import RBF.
  1. You should really avoid this: include "sage/ext/python.pxi", just cimport what you need.
  1. What's the difference between contains_exact and __contains__? I don't understand why you need both these methods.
  1. When raising exceptions, add a string: don't just raise TypeError, but raise TypeError("useful message here")
  1. You write .. SEEALSO:: :meth:abs` but there is no such method.
  1. I still don't understand why below_abs and above_abs return a ball instead of a real number. At the very least, this should be documentated in an OUTPUT: block in those methods.
  1. This is confusing documentation: Return the right endpoint of this ball, viewed as an interval. I would read it like this method returns an interval. Why not Return the right endpoint of this ball.?
  1. I don't understand the purpose of this comment: # TODO: once CBF is there, also wrap functions that only exist in acb*
  1. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by cheuberg

Replying to jdemeyer:

  1. The naming of the files is inconsistent: why real_arb and complex_ball_acb?

real_arb followed the pattern of real_mpfi; whereas complex_ball_acb somehow followed complex_interval. What names would you suggest?

comment:17 in reply to: ↑ 12 ; follow-ups: Changed 4 years ago by mmezzarobba

Hello Clemens,

Thanks again for your comments and the changes you made.

Replying to cheuberg:

I read the modifications and have the following comments.

  1. arf.pxd: You define arf_rnd_t as an enum and rely on the fact that the numeric values of ARF_RND_DOWN etc. will never change in the C header files. However, there, the values are macros, so I do not know whether we can safely assume the values never to change.

I fear I don't understand what you mean here, sorry. I thought that cython would insert textual references to ARF_RND_* in the generated C code. Isn't that the case?

  1. polynomial_element.pyx:
    • I am very surprised to see this change here in an arb ticket.

I tend to consider that what matters is commits, not tickets. So I don't have any problem with that as long as it doesn't make the review harder, and I actually feel if it makes reviews easier... But please tell me if you don't like that and I will try to create separate tickets in the future.

  • The documentation of is_gen states: "Important - this function doesn’t return True if self equals the generator; it returns True if self is the generator." I.e., the outcome differs from the old behaviour.

Good catch! I'd say it is okay, because the operation that really should be fast IMO is x^n where x = Pol.gen(), not (2*x - x)^n, and the new version seems to be about 10% faster when n == 2. But I'm open to suggestions.

  1. RealBall.__hash__: I do not like the fact that the fractional part of the midpoint is ignored completely:
    sage: hash(RBF(3/4))
    1048577
    sage: hash(RBF(5/8))
    1048577
    sage: hash(RBF(7/8))
    1048577
    

Yes, I don't know what I was doing with that hash!

  1. RealBall.upper, RealBall.lower, RealBall.endpoints: missing INPUT: and OUTPUT: sections, document and test parameter rnd

The problem here is that, unlike elements of RealIntervalField(p), elements of RealBallField(p) may have endpoints that are not be representable on p bits. I wasn't sure what to do in this case, so I implemented a quick wrapper for the interval version and then forgot to come back to it. (This is also the reason for the clumsy formulation “viewed as an interval” Jeroen asks about.)

The main options I see are:

  • either to return the endpoints rounded (in which direction? we can't be 100% compatible with interval fields!) to the parent's precision,
  • or return the exact endpoints, but then, the output parent will be hard to predict.

Which do you think is best?

  1. RealBall.add_error: missing INPUT: and OUTPUT: sections

What would you suggest to say there?

  1. RealBall.is_nonzero: The note section is hard to understand because __nonzero__ and is_zero should do very different things.

I'm not sure I understand your comment. As far as I understand (but all this stuff is very badly documented!), in the rest of Sage, __nonzero__() means “syntactically nonzero” (for example, the degree() methods of polynomials look for the leading __nonzero__ term), which for balls is exactly the negation of is_zero().

  1. RealBall.__lshift__, RealBall.__rshift__: why is the first parameter called val instead of self and how can it happen that it is not a RealBall if this is a method of RealBall?

That's how special methods work in Cython... See http://docs.cython.org/src/userguide/special_methods.html#arithmetic-methods

  1. ComplexBall._is_atomic: self.is_real() or self.imag().is_zero() seems to be redundant. Do you want to say self.real().is_zero() instead?

I think so, yes, thanks! But actually I don't understand what _is_atomic() really should do: for example, the documentation seems to suggest that -2 should not be atomic (since x + -2 is not correct), while it is atomic in various standard rings, and I think it has to in order to avoid redundant parentheses...

comment:18 in reply to: ↑ 15 ; follow-ups: Changed 4 years ago by mmezzarobba

Hello Jeroen,

Thank you very much for your remarks.

Replying to jdemeyer:

  1. You can remove the include_dirs=[SAGE_INC + '/flint']) from src/module_list.py.

Are you sure? I get:

[2/2] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/marc/co/sage/local/include -I/home/marc/co/sage/local/include/python2.7 -I/home/marc/co/sage/local/lib/python2.7/site-packages/numpy/core/include -I/home/marc/co/sage/src -I/home/marc/co/sage/src/sage/ext -I/home/marc/co/sage/src/build/cythonized -I/home/marc/co/sage/src/build/cythonized/sage/ext -I/home/marc/co/sage/local/include/python2.7 -c /home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.c -o build/temp.linux-x86_64-2.7/home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.o -fno-strict-aliasing -w
In file included from /home/marc/co/sage/src/build/cythonized/sage/rings/complex_ball_acb.c:310:0:
/home/marc/co/sage/local/include/mag.h:36:19: fatal error: flint.h: No such file or directory
compilation terminated.
In file included from /home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.c:309:0:
/home/marc/co/sage/local/include/mag.h:36:19: fatal error: flint.h: No such file or directory
  1. Ideally, the type declarations in the arb .pxd files should be in a different file: try to follow the model of src/sage/libs/gmp for example. Then you should add # distutils: libraries = arb to the files with library functions (i.e. not types).

A difference is that arb comes with several .h files (one for each module, with a more or less well-defined dependency graph): are you suggesting to put all the type definitions in a single .pxd file nevertheless?

  1. The __hash__ for RBF is bad:
    sage: from sage.rings.real_arb import RBF
    sage: hash(RBF(0.6))
    1048577
    sage: hash(RBF(0.7))
    1048577
    sage: hash(RBF(0.8))
    1048577
    sage: hash(RBF(0.9))
    1048577
    

Fixed (see above).

  1. For CBF.__hash__, what's the point of >> 7?

The idea was just to avoid that the λ(1+i) all hash to zero...

  1. This is a very strange way to handle exceptions, what's wrong with raise?
            if arf_get_mpfr(rad.value, tmp, GMP_RNDN):
                abort()
    

I don't remember for sure. I guess I may have wanted to make sure that errors raised by arf_get_mpfr() itself and errors raised manually give the same exception, and to avoid repeating the error message.

  1. Replace the GMP_RND macros by MPFR_RND.

Done.

  1. What's with this comment?
                    # FIXME: RBF is not even associative, but CompletionFunctor
                    # only works with rings, and other coercion features require
                    # methods like is_integral_domain() to be defined
                    category=category or sage.categories.fields.Fields().Infinite())
    

Your class models a mathematical field (real or complex numbers), so I don't see the problem. How would you want to fix this?

Well, people seem to disagree about the meaning of categories in Sage, and some people apparently assume that a parent that belongs to the category of fields will satisfy the axioms of fields, not just vaguely model a field. This is particularly harmful in the context of interval arithmetic.

In the long term, I'd want to fix this by (i) clarifying the “global conventions” that need to be used consistently across the whole sage codebase and (ii) depending on the outcome, perhaps creating categories for “inexact fields” and the like.

  1. Now that all this is standard, can you add RBF, RealBallField, CBF and ComplexBallField to the global namespace and then remove all doctest lines of the form from sage.rings.real_arb import RBF.

I'm no fan of adding anything to the global namespace, but I'll do it if you insist.

  1. You should really avoid this: include "sage/ext/python.pxi", just cimport what you need.

Done.

  1. What's the difference between contains_exact and __contains__? I don't understand why you need both these methods.

The difference is that contains_exact() works for fewer kinds of objects, but is guaranteed not to return false negatives, while __contains__() just does the check at the parent's precision. Of course, one could do with __contains__() alone, but then it would be too easy to call x in b by accident with an x for which it may not return the correct result.

  1. When raising exceptions, add a string: don't just raise TypeError, but raise TypeError("useful message here")

Done. (But I didn't include a message because I couldn't think of a useful message, so I doubt the new version is much better...)

  1. You write .. SEEALSO:: :meth:abs` but there is no such method.

Indeed, I forgot to remove these references, thanks!

  1. I still don't understand why below_abs and above_abs return a ball instead of a real number. At the very least, this should be documentated in an OUTPUT: block in those methods.

Why: because I often compute upper or lower bounds on quantities represented by balls and then do arithmetic operations on the bounds that may result in rounding errors. Representing the bounds themselves as thin intervals is pretty convenient then. I also find that choice more consistent with the rest of the interface of real/complex balls...

I added OUPUT blocks, please tell me if there is anything else you feel these blocks should say.

  1. This is confusing documentation: Return the right endpoint of this ball, viewed as an interval. I would read it like this method returns an interval. Why not Return the right endpoint of this ball.?

Indeed! See my answer to Clemens above.

  1. I don't understand the purpose of this comment: # TODO: once CBF is there, also wrap functions that only exist in acb*

There are special functions in arb that are only implemented for complex arguments, but that would make sense on sage real balls.

  1. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

An arb ball has no intrinsic precision, and (more or less because of that), an element of RealBallField(p) can have a midpoint that doesn't fit on p bits. The round() method rounds such elements to the precision of their parent.

comment:19 Changed 4 years ago by mmezzarobba

  • Branch changed from u/cheuberg/19152-arb-misc to public/19152-arb-misc
  • Commit changed from 16a04a07c63349d3792ed8c538a7d60e5adfde5e to ea08942380a0cfab4614de252bac9a8db340b3b4
  • Status changed from needs_work to needs_info

New commits:

07e365cRealBall: more robust __hash__
f0f4d6cComplexBall: fix _is_atomic()
d17f6daarb interface: use MPFR_RND? instead of GMP_RND?
31e3b4barb interface: avoid including python.pxi
ea08942{Real,Complex}Ball.*abs: add error message, fix crossrefs

comment:20 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by jdemeyer

Replying to cheuberg:

real_arb followed the pattern of real_mpfi; whereas complex_ball_acb somehow followed complex_interval.

I also find these names confusing, so I don't think it's a model to follow.

What names would you suggest?

Why not simply real_arb and complex_arb? At least something more consistent.

comment:21 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by jdemeyer

Replying to mmezzarobba:

Replying to cheuberg:

  1. arf.pxd: You define arf_rnd_t as an enum and rely on the fact that the numeric values of ARF_RND_DOWN etc. will never change in the C header files. However, there, the values are macros, so I do not know whether we can safely assume the values never to change.

I fear I don't understand what you mean here, sorry. I thought that cython would insert textual references to ARF_RND_* in the generated C code. Isn't that the case?

I agree, there is no issue here. Cython doesn't care about the values.

  1. RealBall.upper, RealBall.lower, RealBall.endpoints: missing INPUT: and OUTPUT: sections, document and test parameter rnd

The main options I see are:

  • either to return the endpoints rounded (in which direction? we can't be 100% compatible with interval fields!) to the parent's precision,

I would go for: rounding endpoints in the obvious direction (i.e. up for the upper endpoint and down for the lower endpoint)

comment:22 in reply to: ↑ 18 ; follow-up: Changed 4 years ago by jdemeyer

Replying to mmezzarobba:

Replying to jdemeyer:

  1. You can remove the include_dirs=[SAGE_INC + '/flint']) from src/module_list.py.

Are you sure? I get:

[2/2] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/marc/co/sage/local/include -I/home/marc/co/sage/local/include/python2.7 -I/home/marc/co/sage/local/lib/python2.7/site-packages/numpy/core/include -I/home/marc/co/sage/src -I/home/marc/co/sage/src/sage/ext -I/home/marc/co/sage/src/build/cythonized -I/home/marc/co/sage/src/build/cythonized/sage/ext -I/home/marc/co/sage/local/include/python2.7 -c /home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.c -o build/temp.linux-x86_64-2.7/home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.o -fno-strict-aliasing -w
In file included from /home/marc/co/sage/src/build/cythonized/sage/rings/complex_ball_acb.c:310:0:
/home/marc/co/sage/local/include/mag.h:36:19: fatal error: flint.h: No such file or directory
compilation terminated.
In file included from /home/marc/co/sage/src/build/cythonized/sage/rings/real_arb.c:309:0:
/home/marc/co/sage/local/include/mag.h:36:19: fatal error: flint.h: No such file or directory

I must admit I didn't test it, but it seems the problem comes from arb (not the Sage library). I'll look into it later.

  1. Ideally, the type declarations in the arb .pxd files should be in a different file: try to follow the model of src/sage/libs/gmp for example. Then you should add # distutils: libraries = arb to the files with library functions (i.e. not types).

A difference is that arb comes with several .h files (one for each module, with a more or less well-defined dependency graph): are you suggesting to put all the type definitions in a single .pxd file nevertheless?

Yes, put the type declarations all in types.pxd.

  1. For CBF.__hash__, what's the point of >> 7?

The idea was just to avoid that the λ(1+i) all hash to zero...

??? I don't get it.

  1. This is a very strange way to handle exceptions, what's wrong with raise?
            if arf_get_mpfr(rad.value, tmp, GMP_RNDN):
                abort()
    

I don't remember for sure. I guess I may have wanted to make sure that errors raised by arf_get_mpfr() itself and errors raised manually give the same exception, and to avoid repeating the error message.

You can use sig_error() for that, it just calls abort() but it's more explicit.

  1. What's with this comment?
                    # FIXME: RBF is not even associative, but CompletionFunctor
                    # only works with rings, and other coercion features require
                    # methods like is_integral_domain() to be defined
                    category=category or sage.categories.fields.Fields().Infinite())
    

Your class models a mathematical field (real or complex numbers), so I don't see the problem. How would you want to fix this?

Well, people seem to disagree about the meaning of categories in Sage, and some people apparently assume that a parent that belongs to the category of fields will satisfy the axioms of fields, not just vaguely model a field. This is particularly harmful in the context of interval arithmetic.

In the long term, I'd want to fix this by (i) clarifying the “global conventions” that need to be used consistently across the whole sage codebase and (ii) depending on the outcome, perhaps creating categories for “inexact fields” and the like.

But that's not at all specific to arb right, so I'm just confused by this comment. I propose to remove it.

  1. Now that all this is standard, can you add RBF, RealBallField, CBF and ComplexBallField to the global namespace and then remove all doctest lines of the form from sage.rings.real_arb import RBF.

I'm no fan of adding anything to the global namespace, but I'll do it if you insist.

Why not?

  1. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

An arb ball has no intrinsic precision, and (more or less because of that), an element of RealBallField(p) can have a midpoint that doesn't fit on p bits. The round() method rounds such elements to the precision of their parent.

Now I'm even more confused... I still have no clue what you mean.

comment:23 in reply to: ↑ 18 Changed 4 years ago by jdemeyer

Replying to mmezzarobba:

  1. I don't understand the purpose of this comment: # TODO: once CBF is there, also wrap functions that only exist in acb*

There are special functions in arb that are only implemented for complex arguments, but that would make sense on sage real balls.

  1. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

An arb ball has no intrinsic precision, and (more or less because of that), an element of RealBallField(p) can have a midpoint that doesn't fit on p bits. The round() method rounds such elements to the precision of their parent.

When asking these questions above, I'm not just asking for myself personally. It means that the code on the branch should be clarified.

comment:24 Changed 4 years ago by jdemeyer

I consider the fact that include_dirs is needed an upstream arb bug. So you can leave it for now. See #19563.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:25 in reply to: ↑ 20 Changed 4 years ago by mmezzarobba

Replying to jdemeyer:

Why not simply real_arb and complex_arb? At least something more consistent.

I agree, I already considered making that change several times. Actually, at some point, I'd like to reorganize the code of variants of real and complex numbers in order to make them more consistent with each other and reduce code duplication a little, and I think it would make sense to move them all to a new package sage.rings.numerical (or similar) then.

comment:26 in reply to: ↑ 21 Changed 4 years ago by mmezzarobba

Replying to jdemeyer:

I would go for: rounding endpoints in the obvious direction (i.e. up for the upper endpoint and down for the lower endpoint)

Fine with me. The rnd argument will be a bit confusing, but I don't think that's too bad.

comment:27 in reply to: ↑ 22 ; follow-ups: Changed 4 years ago by mmezzarobba

Replying to jdemeyer:

A difference is that arb comes with several .h files (one for each module, with a more or less well-defined dependency graph): are you suggesting to put all the type definitions in a single .pxd file nevertheless?

Yes, put the type declarations all in types.pxd.

Done.

  1. For CBF.__hash__, what's the point of >> 7?

The idea was just to avoid that the λ(1+i) all hash to zero...

??? I don't get it.

I'm sorry, I don't understand what you don't understand. (You are talking about ComplexBall.__hash__(), not ComplexBallField.__hash__(), right?)

You can use sig_error() for that, it just calls abort() but it's more explicit.

Changed (though to me this is “strange sage-specific stuff” vs “standard C/POSIX”, so less explicit...)

But that's not at all specific to arb right, so I'm just confused by this comment. I propose to remove it.

Fine.

  1. Now that all this is standard, can you add RBF, RealBallField, CBF and ComplexBallField to the global namespace and then remove all doctest lines of the form from sage.rings.real_arb import RBF.

I'm no fan of adding anything to the global namespace, but I'll do it if you insist.

Why not?

Because it doesn't make any sense to me to import tons of specialized stuff on startup. It is so much better to import the things you use in your personal init.sage... (And sage -min, which would mitigate the problem, hasn't worked for years afaik.)

  1. I don't understand what this means: Return a copy of this ball rounded to the precision of the parent.

An arb ball has no intrinsic precision, and (more or less because of that), an element of RealBallField(p) can have a midpoint that doesn't fit on p bits. The round() method rounds such elements to the precision of their parent.

Now I'm even more confused... I still have no clue what you mean.

I added some documentation, please tell me if it is clear enough.

comment:28 Changed 4 years ago by git

  • Commit changed from ea08942380a0cfab4614de252bac9a8db340b3b4 to d5af324cfc7c998a11c91b98d00521c2f664502c

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

49b79f3rename complex_ball_acb to complex_arb, 1/2
361ae39rename complex_ball_acb to complex_arb, 2/2
88da8delibs.arb: reorganize
ab117dacomplex_ball: abort() --> sig_error()
1ee8c16ref manual: rm reference to arb being optional
865e6d5{real,complex}_arb: more doc on precision issues
d289538{real,complex}_arb: move SEEALSO blocks after EXAMPLES
fe1b071RealBall: clarify doc of upper(), lower(), endpoints()
a38574acomplex_arb is no longer experimental
d5af324{real,complex}_arb: minor doc fixes

comment:29 Changed 4 years ago by mmezzarobba

  • Status changed from needs_info to needs_review

comment:30 in reply to: ↑ 27 ; follow-up: Changed 4 years ago by jdemeyer

Replying to mmezzarobba:

Because it doesn't make any sense to me to import tons of specialized stuff on startup.

We have lazy_import for that.

comment:31 in reply to: ↑ 27 Changed 4 years ago by jdemeyer

Replying to mmezzarobba:

I'm sorry, I don't understand what you don't understand. (You are talking about ComplexBall.__hash__(), not ComplexBallField.__hash__(), right?)

I now understand what you mean. I would prefer something like

hash(real_part) + 3*hash(complex_part)

then. By doing >> 7, you're throwing away 7 bits for no reason.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:32 in reply to: ↑ 30 ; follow-up: Changed 4 years ago by mmezzarobba

Replying to jdemeyer:

Replying to mmezzarobba:

Because it doesn't make any sense to me to import tons of specialized stuff on startup.

We have lazy_import for that.

I'm not talking only about the cost of the imports, but mainly about the namespace pollution.

comment:33 Changed 4 years ago by git

  • Commit changed from d5af324cfc7c998a11c91b98d00521c2f664502c to f43e94a46bc9c0f03ba63ee8538384180d89d306

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

f43e94aRealBall: minor change to __hash__

comment:34 Changed 4 years ago by jdemeyer

In __hash__, you are still throwing away 1.58 bits, but it's an improvement :-)

The rnd argument will be a bit confusing, but I don't think that's too bad.

I don't mind deprecating the rnd argument, I don't think it's useful anyway.

comment:35 Changed 4 years ago by jdemeyer

I was reading the real_arb documentation and this caught my eye:

Warning

Identical RealBall? objects are understood to give permission for algebraic simplification. This assumption is made to improve performance. For example, setting z = x*x sets z to a ball enclosing the set {t2:t∈x} and not the (generally larger) set {tu:t∈x,u∈x}.

I'm not sure that we should do that. It's not what intervals do: I would expect x*x to be different from x^2.

comment:36 follow-up: Changed 4 years ago by jdemeyer

I consider this a bug:

It is possible to construct a ball whose parent is the real ball field with precision p but whose midpoint does not fit on p bits. However, the results of operations involving such a ball will (usually) be rounded to its parent’s precision.

It causes too much confusion for example with the round() function. It's also too different from all other places in Sage which handle precision.

comment:37 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This is a good reason to round when creating a ball:

sage: a = RealBallField(20)(1.3)
sage: b = RealBallField(53)(1.3)
sage: a == b
True
sage: a+0 == b+0
False

comment:38 Changed 4 years ago by jdemeyer

Despite what the documentation says, we still have

sage: a = RealBallField(53)(RIF(-1,1))
sage: (a*(a+0)).identical(a^2)
True
Last edited 4 years ago by jdemeyer (previous) (diff)

comment:39 Changed 4 years ago by git

  • Commit changed from f43e94a46bc9c0f03ba63ee8538384180d89d306 to 7e6b5d049ce0107880171d156f3da2f16d480476

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

7e6b5d0real_arb: minor doc fix

comment:40 in reply to: ↑ 36 ; follow-ups: Changed 4 years ago by mmezzarobba

Replying to jdemeyer:

Identical RealBall? objects are understood to give permission for algebraic simplification. This assumption is made to improve performance. For example, setting z = x*x sets z to a ball enclosing the set {t2:t∈x} and not the (generally larger) set {tu:t∈x,u∈x}.

I'm not sure that we should do that. It's not what intervals do: I would expect x*x to be different from x^2.

Replying to jdemeyer:

I consider this a bug:

It is possible to construct a ball whose parent is the real ball field with precision p but whose midpoint does not fit on p bits. However, the results of operations involving such a ball will (usually) be rounded to its parent’s precision.

It causes too much confusion for example with the round() function. It's also too different from all other places in Sage which handle precision.

Both these design choices were discussed on previous arb-related tickets, in particular #17194. As far as I'm concerend, I'm happy with the way things currently work (even though I was a little bit skeptical at first), and I'm not interested in working on changing it. Incidentally, I'm not sure this ticket would be the right place.

comment:41 in reply to: ↑ 40 Changed 4 years ago by jdemeyer

Even though I don't agree with everything in this branch, I'm willing to give it positive_review.

comment:42 Changed 4 years ago by jdemeyer

Follow-ups: #19563 and #19568.

comment:43 in reply to: ↑ 40 ; follow-up: Changed 4 years ago by cheuberg

Replying to mmezzarobba:

Replying to jdemeyer:

Identical RealBall? objects are understood to give permission for algebraic simplification. This assumption is made to improve performance. For example, setting z = x*x sets z to a ball enclosing the set {t2:t∈x} and not the (generally larger) set {tu:t∈x,u∈x}.

I'm not sure that we should do that. It's not what intervals do: I would expect x*x to be different from x^2.

Both these design choices were discussed on previous arb-related tickets, in particular #17194. As far as I'm concerend, I'm happy with the way things currently work (even though I was a little bit skeptical at first), and I'm not interested in working on changing it. Incidentally, I'm not sure this ticket would be the right place.

In particular, the first issue is a property of arb, see its documentation. Therefore, working around that might have involved that we make copies ourselves.

What is now the status of this ticket? Is it still needs_work, in particular after Jeroens remark earlier today?

comment:44 in reply to: ↑ 43 ; follow-up: Changed 4 years ago by jdemeyer

Replying to cheuberg:

In particular, the first issue is a property of arb, see its documentation.

If that is the case, this absolutely should be mentioned in the Sage documentation for arb.

comment:45 Changed 4 years ago by jdemeyer

I really would prefer to have a positive_review to this ticket together with a positive_review for #19568. Can somebody please (re)consider reviewing #19568?

comment:46 in reply to: ↑ 44 Changed 4 years ago by cheuberg

Replying to jdemeyer:

Replying to cheuberg:

In particular, the first issue is a property of arb, see its documentation.

If that is the case, this absolutely should be mentioned in the Sage documentation for arb.

I do not understand what you mean.

Arb's documentation says that "An important caveat applies to aliasing of input variables. Identical pointers are understood to give permission for algebraic simplification."

The documentation of the real_arb class has a "Warning" box stating "Identical RealBall objects are understood to give permission for algebraic simplification. This assumption is made to improve performance."

comment:47 Changed 4 years ago by jdemeyer

I meant that the documentation should state this is done by the arb library, not the Sage interface to arb.

comment:48 Changed 4 years ago by git

  • Commit changed from 7e6b5d049ce0107880171d156f3da2f16d480476 to cc74b61498b0ed0e6e5a54e30df6b5cfbe33350a

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

9b9cfb8Trac #19152: reformat OUTPUT section and remove typo
1f8d390Trac #19152: add OUTPUT blocks
cc74b61Trac #19152: fix one link and add SEEALSO

comment:49 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by cheuberg

Replying to mmezzarobba:

Replying to cheuberg:

  1. arf.pxd: You define arf_rnd_t as an enum and rely on the fact that the numeric values of ARF_RND_DOWN etc. will never change in the C header files. However, there, the values are macros, so I do not know whether we can safely assume the values never to change.

I fear I don't understand what you mean here, sorry. I thought that cython would insert textual references to ARF_RND_* in the generated C code. Isn't that the case?

ok. Forget it.

  1. polynomial_element.pyx:
    • I am very surprised to see this change here in an arb ticket.

I tend to consider that what matters is commits, not tickets. So I don't have any problem with that as long as it doesn't make the review harder, and I actually feel if it makes reviews easier... But please tell me if you don't like that and I will try to create separate tickets in the future.

It makes reviewing harder as I do not really want to interfere with the polynomials guys ...

  • The documentation of is_gen states: "Important - this function doesn’t return True if self equals the generator; it returns True if self is the generator." I.e., the outcome differs from the old behaviour.

Good catch! I'd say it is okay, because the operation that really should be fast IMO is x^n where x = Pol.gen(), not (2*x - x)^n, and the new version seems to be about 10% faster when n == 2. But I'm open to suggestions.

Well, I would have preferred to have somebody else to decide about that;

  1. RealBall.upper, RealBall.lower, RealBall.endpoints: missing INPUT: and OUTPUT: sections, document and test parameter rnd

The problem here is that, unlike elements of RealIntervalField(p), elements of RealBallField(p) may have endpoints that are not be representable on p bits. I wasn't sure what to do in this case, so I implemented a quick wrapper for the interval version and then forgot to come back to it. (This is also the reason for the clumsy formulation “viewed as an interval” Jeroen asks about.)

The main options I see are:

  • either to return the endpoints rounded (in which direction? we can't be 100% compatible with interval fields!) to the parent's precision,
  • or return the exact endpoints, but then, the output parent will be hard to predict.

Which do you think is best?

current behaviour is fine for me.

  1. RealBall.add_error: missing INPUT: and OUTPUT: sections

What would you suggest to say there?

I'd like to know what ampl shall be (any rational number? ball?) and that I'd prefer to state that OUTPUT:

A complex ball.

(the description seems to be worded as an inline operation, but it actually returns a new ball).

  1. RealBall.is_nonzero: The note section is hard to understand because __nonzero__ and is_zero should do very different things.

I'm not sure I understand your comment. As far as I understand (but all this stuff is very badly documented!), in the rest of Sage, __nonzero__() means “syntactically nonzero” (for example, the degree() methods of polynomials look for the leading __nonzero__ term), which for balls is exactly the negation of is_zero().

I'd prefer something which separates the two messages of the one sentence, e.g. "Use __nonzero__() to check that a ball is not exactly the zero ball (for instance, to determine the “degree” of a polynomial with ball coefficients). In fact, __nonzero__() is the negation of is_zero(), whereas is_nonzero() only returns True if 0 is known not to be in the ball."

Further remark:

  1. The AUTHORS section at the beginning of the files is not accurate: I started those files, but most of their contents has been written by you. So please either add your name or remove mine.

comment:50 in reply to: ↑ 32 Changed 4 years ago by cheuberg

Replying to mmezzarobba:

Replying to jdemeyer:

Replying to mmezzarobba:

Because it doesn't make any sense to me to import tons of specialized stuff on startup.

We have lazy_import for that.

I'm not talking only about the cost of the imports, but mainly about the namespace pollution.

I would not mind having RBF, CBF, RealBallField and ComplexBallField in the global name space; this is how most of sage seems to work. But this could also happen in another ticket ...

comment:51 Changed 4 years ago by git

  • Commit changed from cc74b61498b0ed0e6e5a54e30df6b5cfbe33350a to c8873f0d1fac0deb01a75d2203907b5a77fecf0e

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

f51cd86{Real,Complex}Ball: better document add_error()
49a2e9b{Real,Complex}Ball: improve doc of is_nonzero()
154b65d{real,complex}_arb: rm AUTHORS blocks
e22f407add RealBallField & friends to the global namespace
c8873f0real_ball: one more instance of [non]zero -> True/False

comment:52 in reply to: ↑ 49 ; follow-up: Changed 4 years ago by mmezzarobba

Replying to cheuberg:

It makes reviewing harder as I do not really want to interfere with the polynomials guys ...

Ok, sorry, I'll try to avoid doing that in tickets you are likely to review.

I'd like to know what ampl shall be (any rational number? ball?) and that I'd prefer to state that OUTPUT:

A complex ball.

(the description seems to be worded as an inline operation, but it actually returns a new ball).

Done.

I'd prefer something which separates the two messages of the one sentence, e.g. "Use __nonzero__() to check that a ball is not exactly the zero ball (for instance, to determine the “degree” of a polynomial with ball coefficients). In fact, __nonzero__() is the negation of is_zero(), whereas is_nonzero() only returns True if 0 is known not to be in the ball."

Done.

Further remark:

  1. The AUTHORS section at the beginning of the files is not accurate: I started those files, but most of their contents has been written by you. So please either add your name or remove mine.

I'll be consistent with what I said on sage-devel a few weeks ago and simply remove the AUTHORS block :-). But of course please feel free to add us both back if you prefer.

Replying to cheuberg:

I would not mind having RBF, CBF, RealBallField? and ComplexBallField? in the global name space; this is how most of sage seems to work. But this could also happen in another ticket ...

Done — I still don't like that very much, but since everyone seems to agree...

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

comment:53 Changed 4 years ago by mmezzarobba

  • Status changed from needs_work to needs_review

comment:54 Changed 4 years ago by git

  • Commit changed from c8873f0d1fac0deb01a75d2203907b5a77fecf0e to 7920b9f9fd59f3271c45eb11b370eef3787b3242

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

7920b9f{real,complex}_arb: clarify that b == b in arb too

comment:55 Changed 4 years ago by mmezzarobba

  • Status changed from needs_review to needs_work

Oops, merge conflict will the last develop. I will overwrite the last few commits in a minute to avoid the conflict.

comment:56 Changed 4 years ago by git

  • Commit changed from 7920b9f9fd59f3271c45eb11b370eef3787b3242 to 3749cbb41597447edb6f33fd43a6da1a4aac01dd

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

dfccd23e22f4074539b1833befb7f24bd700a7fe11589a
73a928areal_ball: one more instance of [non]zero -> True/False
3749cbb{real,complex}_arb: clarify that b == b in arb too

comment:57 Changed 4 years ago by mmezzarobba

  • Status changed from needs_work to needs_review

comment:58 Changed 4 years ago by git

  • Commit changed from 3749cbb41597447edb6f33fd43a6da1a4aac01dd to 0a04e627728c37d334c3576eb5243facebee0e80

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

5334c56Trac #19152: minor ReSt formatting
0a04e62Trac #19152: delete empty "TESTS" section

comment:59 in reply to: ↑ 52 ; follow-ups: Changed 4 years ago by cheuberg

Replying to mmezzarobba:

Replying to cheuberg:

I would not mind having RBF, CBF, RealBallField? and ComplexBallField? in the global name space; this is how most of sage seems to work. But this could also happen in another ticket ...

Done — I still don't like that very much, but since everyone seems to agree...

Is there a particular reason for not using lazy_import?

Apart from that question, all my concerns have been dealt with and as far as I can see, Jeroen's comments are also all taken into account.

I think that #19568 is not too related to the changes in this ticket (RealBallField was never marked as experimental, as that decorator did not yet exist when it was introduced).

Therefore, I intend to set this ticket to positive as soon as the question about lazy imports is settled.

comment:60 Changed 4 years ago by cheuberg

  • Status changed from needs_review to needs_info

comment:61 in reply to: ↑ 59 Changed 4 years ago by jdemeyer

Replying to cheuberg:

I think that #19568 is not too related to the changes in this ticket

It's not related but it would be nice to fix #19568 before too many people start using RBF.

comment:62 in reply to: ↑ 59 ; follow-up: Changed 4 years ago by mmezzarobba

Replying to cheuberg:

Is there a particular reason for not using lazy_import?

Yes: I'm not sure that the modules are expensive enough to be worth importing with lazy_import, and it causes doctest failures that I didn't have the time or motivation to investigate...

comment:63 in reply to: ↑ 62 ; follow-up: Changed 4 years ago by jdemeyer

Replying to mmezzarobba:

it causes doctest failures that I didn't have the time or motivation to investigate...

Can you at least push your attempt such that it can be investigated?

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

Replying to jdemeyer:

Replying to mmezzarobba:

it causes doctest failures that I didn't have the time or motivation to investigate...

Can you at least push your attempt such that it can be investigated?

The following change:

diff --git a/src/sage/rings/all.py b/src/sage/rings/all.py
index b5d4db7..33f18a1 100644
--- a/src/sage/rings/all.py
+++ b/src/sage/rings/all.py
@@ -83,7 +83,8 @@ from real_double import RealDoubleField, RDF, RealDoubleElement
 
 from real_lazy import RealLazyField, RLF, ComplexLazyField, CLF
 
-from sage.rings.real_arb import RealBallField, RBF
+lazy_import("sage.rings.real_arb", "RealBallField")
+lazy_import("sage.rings.real_arb", "RBF")
 
 # Polynomial Rings and Polynomial Quotient Rings
 from polynomial.all import *

leads to:

$ ./sage -t src/sage/rings/real_arb.pyx src/sage/rings/complex_arb.pyx
too many failed tests, not using stored timings
Running doctests with ID 2015-11-16-16-34-41-63268156.
Git branch: arb-misc
Using --optional=mpir,python2,sage,scons
Doctesting 2 files.
sage -t src/sage/rings/real_arb.pyx
**********************************************************************
File "src/sage/rings/real_arb.pyx", line 314, in sage.rings.real_arb.RealBallField
Failed example:
    (1/2*RBF(1)) + AA(sqrt(2)) - 1 + polygen(QQ, x)
Exception raised:
    Traceback (most recent call last):
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.real_arb.RealBallField[2]>", line 1, in <module>
        (Integer(1)/Integer(2)*RBF(Integer(1))) + AA(sqrt(Integer(2))) - Integer(1) + polygen(QQ, x)
      File "sage/structure/element.pyx", line 1702, in sage.structure.element.RingElement.__add__ (build/cythonized/sage/structure/element.c:16575)
        return coercion_model.bin_op(left, right, add)
      File "sage/structure/coerce.pyx", line 1070, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:9739)
        raise TypeError(arith_error_message(x,y,op))
    TypeError: unsupported operand parent(s) for '+': 'Real ball field with 53 bits precision' and 'Univariate Polynomial Ring in x over Rational Field'
**********************************************************************
1 item had failures:
   1 of  14 in sage.rings.real_arb.RealBallField
    [454 tests, 1 failure, 0.23 s]
sage -t src/sage/rings/complex_arb.pyx
    [253 tests, 0.25 s]
----------------------------------------------------------------------
sage -t src/sage/rings/real_arb.pyx  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.7 seconds
    cpu time: 0.5 seconds
    cumulative wall time: 0.5 seconds
(exit 1) 

comment:65 Changed 4 years ago by mmezzarobba

  • Status changed from needs_info to needs_review

comment:66 Changed 4 years ago by git

  • Commit changed from 0a04e627728c37d334c3576eb5243facebee0e80 to b21aa6e069182b08aca3cf7c24d6ed24e5926510

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

b21aa6eTrac #19152: lazy_import

comment:67 in reply to: ↑ 64 Changed 4 years ago by cheuberg

Replying to mmezzarobba:

Replying to jdemeyer:

Replying to mmezzarobba: Can you at least push your attempt such that it can be investigated?

The following change: ... leads to: ...

    TypeError: unsupported operand parent(s) for '+': 'Real ball field with 53 bits precision' and 'Univariate Polynomial Ring in x over Rational Field'

I cannot reproduce this. I used your diff and had no problems with make ptestlong, neither on 6.10.beta0 where this branch is based on nor after merging with 6.10.beta5. Tested only on 64-bit linux though.

Let's see what the patchbots say.

comment:68 Changed 4 years ago by git

  • Commit changed from b21aa6e069182b08aca3cf7c24d6ed24e5926510 to 0a04e627728c37d334c3576eb5243facebee0e80

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

comment:69 Changed 4 years ago by cheuberg

So the patchbot could reproduce the problem (and in the meantime, I could also reproduce it). Thus I reset the branch to the earlier commit.

The commit with the lazy import is still available as branch u/cheuberg/19152-arb-misc-lazy-import if somebody wants to debug this.

My intention now is to set this ticket to positive review as it is in the next few days unless somebody objects.

comment:70 follow-up: Changed 4 years ago by vbraun

Fine with me; There seems to be something fishy with lazy import and coercion. Can you open a new ticket and push your lazy-import branch there?

comment:71 in reply to: ↑ 70 Changed 4 years ago by cheuberg

  • Status changed from needs_review to positive_review

Replying to vbraun:

Can you open a new ticket and push your lazy-import branch there?

This is now #19628.

Thus it seems time to set this to positive_review.

comment:72 Changed 4 years ago by mmezzarobba

I just noticed a stupid bug in the new __hash__ (07e365c509fbbd6e54da9e172f1845f6d24971c9): I forgot to fmpz_init some variables. Would you prefer me to fix it here or to open a new ticket?

comment:73 follow-up: Changed 4 years ago by cheuberg

It's safer to do it in a new ticket (The release manager might already have pulled this ticket for inclusion).

comment:74 in reply to: ↑ 73 Changed 4 years ago by mmezzarobba

Replying to cheuberg:

It's safer to do it in a new ticket (The release manager might already have pulled this ticket for inclusion).

Done in #19629.

comment:75 Changed 4 years ago by vbraun

  • Branch changed from public/19152-arb-misc to 0a04e627728c37d334c3576eb5243facebee0e80
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.