Opened 5 years ago
Closed 5 years ago
#19063 closed enhancement (fixed)
Coercions and basic arithmetic for complex balls
Reported by:  mmezzarobba  Owned by:  

Priority:  major  Milestone:  sage6.10 
Component:  numerical  Keywords:  arb 
Cc:  cheuberg  Merged in:  
Authors:  Marc Mezzarobba, Clemens Heuberger  Reviewers:  Clemens Heuberger, Marc Mezzarobba 
Report Upstream:  N/A  Work issues:  
Branch:  fa41787 (Commits)  Commit:  fa41787b7f670f82ba79ad2fa9ebe67ef5c2eb24 
Dependencies:  #18546  Stopgaps: 
Description (last modified by )
Change History (40)
comment:1 Changed 5 years ago by
 Branch set to u/mmezzarobba/19063acb
 Commit set to 8732ecb7f610a5e3c2d2d350cd83a73180b5e8be
comment:2 Changed 5 years ago by
 Commit changed from 8732ecb7f610a5e3c2d2d350cd83a73180b5e8be to 448bf24a279abd46d4afdf6d9e0e2c01693da1f5
comment:3 Changed 5 years ago by
 Keywords arb added
 Status changed from new to needs_review
comment:4 Changed 5 years ago by
 Description modified (diff)
comment:5 Changed 5 years ago by
 Cc cheuberg added
comment:6 Changed 5 years ago by
I started reviewing this and just a few first comments ...
 SEEALSO block at the beginning of complex_ball_acb.html: show human readable names of the modules instead of sage.rings....
ComplexBallField.construction()
: onesentencedescription: Replace "Returns" by "Return" (see developper guide)ComplexBallField.gen()
,.gens()
,.ngens()
: Missing onesentencedescription
comment:7 Changed 5 years ago by
 Commit changed from 448bf24a279abd46d4afdf6d9e0e2c01693da1f5 to 0196f69eb2ab98bd6468bfd0152396d7c5d1e89d
Branch pushed to git repo; I updated commit sha1. New commits:
0196f69  ComplexBallField: minor documentation improvements

comment:8 Changed 5 years ago by
 Branch changed from u/mmezzarobba/19063acb to u/cheuberg/19063acb
comment:9 Changed 5 years ago by
 Commit changed from 0196f69eb2ab98bd6468bfd0152396d7c5d1e89d to a33ceb40f76cd57791ad9e950fec234373278057
Branch pushed to git repo; I updated commit sha1. New commits:
6f7bea0  Trac #19063: Fix indentation in doctest

80fcc47  Trac #19063: Simplify doctest

cacf2ce  Trac #19063: RESt formatting

d05b9a2  Trac #19063: Replace "nonzero" by "True"

e214cc9  fixup: true/false

b52f52a  Trac #19063: replace a few "int" by "bint"

a33ceb4  Trac #19063: Replace interval by ball in title

comment:10 followup: ↓ 11 Changed 5 years ago by
 Reviewers set to Clemens Heuberger
 Status changed from needs_review to needs_info
I have now reviewed this patch and added a few reviewer commits.
Questions:
ComplexBall.__init__
: whyElement.__init__
instead ofRingElement.__init__
.some_elements
: Is1/3
intentional (this is an elaborate way of writing 0 in python)
 Why is the representation
real  imag*I
restricted to exact negative imaginary parts and does not apply to1 + [0.333]*I
.__contains__
,.contains_exact
: why do we have two distinct methods here? Why is an integer or a rational as the argument of.__contains__
not handeled separately by the relevant .acb functions?
Apart from that, a few documentation issues:
_complex_mpfr_field_
: missing INPUT: section (parameterparent
)
_real_mpfi_
: missing INPUT: section (parameter parent), Onesentencedescription mentions "real number" instead of "real interval".
_mpfr_
: missing INPUT: section (parameterparent
)
.mid
,.squash
: missing OUTPUT: section
.mid
should have "see also" section (.squash
)
.identical
refers to a methodcontains
which does not exist.
.contains_exact
,.identical
,.overlaps
: missing INPUT: section (parameterother
)
comment:11 in reply to: ↑ 10 ; followup: ↓ 13 Changed 5 years ago by
Replying to cheuberg:
I have now reviewed this patch and added a few reviewer commits.
Thanks!
Questions:
ComplexBall.__init__
: whyElement.__init__
instead ofRingElement.__init__
Because I neglected to change it after changing the parent class. (I hate Python...)
.some_elements
: Is1/3
intentional (this is an elaborate way of writing 0 in python)
It wasn't intentional.
 Why is the representation
real  imag*I
restricted to exact negative imaginary parts and does not apply to1 + [0.333]*I
The old version would return things like 1.000000000000000 +  0.5000000000000000*I
for exactly representable imaginary parts. I only implemented a minimal fix, thinking that the version with ... + [...]
was clear enough and perhaps clearer otherwise (especially for intervals containing zero), but I have no strong feelings either way.
.__contains__
,.contains_exact
: why do we have two distinct methods here? Why is an integer or a rational as the argument of.__contains__
not handeled separately by the relevant .acb functions?
The idea was to have a version that first converts its argument to a ball (and thus can accept a wide range of argument types), but may not recognize that the argument is contained in the other ball if the conversion is not precise enough, and a version for which a result of False
always guarantees that the argument is not in the ball. We could just document for which argument types the result of in
is guaranteed to be accurate, but I felt it'd be clearer that way. Here too, I'm open to discussion if you don't like that choice.
Apart from that, a few documentation issues:
_complex_mpfr_field_
: missing INPUT: section (parameterparent
)
_real_mpfi_
: missing INPUT: section (parameter parent), Onesentencedescription mentions "real number" instead of "real interval".
_mpfr_
: missing INPUT: section (parameterparent
)
.mid
,.squash
: missing OUTPUT: section
.mid
should have "see also" section (.squash
)
.identical
refers to a methodcontains
which does not exist.
.contains_exact
,.identical
,.overlaps
: missing INPUT: section (parameterother
)
Thanks. But note that the formal requirement of pointless INPUT/OUTPUT sections in docstrings is apparently going to be relaxed (#19041), so I'm not sure we need them INPUT sections every time you noted they were missing.
comment:12 Changed 5 years ago by
 Status changed from needs_info to needs_work
comment:13 in reply to: ↑ 11 Changed 5 years ago by
Replying to mmezzarobba:
Questions:
ComplexBall.__init__
: whyElement.__init__
instead ofRingElement.__init__
Because I neglected to change it after changing the parent class. (I hate Python...)
If I remember correctly, there is some reason not to use super
.
 Why is the representation
real  imag*I
restricted to exact negative imaginary parts and does not apply to1 + [0.333]*I
The old version would return things like
1.000000000000000 +  0.5000000000000000*I
for exactly representable imaginary parts. I only implemented a minimal fix, thinking that the version with... + [...]
was clear enough and perhaps clearer otherwise (especially for intervals containing zero), but I have no strong feelings either way.
No strong feelings from my side; I was just wondering reading the code. For intervals containing the zero, it might indeed be strange.
.__contains__
,.contains_exact
: why do we have two distinct methods here? Why is an integer or a rational as the argument of.__contains__
not handeled separately by the relevant .acb functions?The idea was to have a version that first converts its argument to a ball (and thus can accept a wide range of argument types), but may not recognize that the argument is contained in the other ball if the conversion is not precise enough, and a version for which a result of
False
always guarantees that the argument is not in the ball. We could just document for which argument types the result ofin
is guaranteed to be accurate, but I felt it'd be clearer that way. Here too, I'm open to discussion if you don't like that choice.
I think that rational in ball
and integer in ball
should give correct results in any case.
Apart from that, a few documentation issues:
_complex_mpfr_field_
: missing INPUT: section (parameterparent
)
_real_mpfi_
: missing INPUT: section (parameter parent), Onesentencedescription mentions "real number" instead of "real interval".
_mpfr_
: missing INPUT: section (parameterparent
)
.mid
,.squash
: missing OUTPUT: section
.mid
should have "see also" section (.squash
)
.identical
refers to a methodcontains
which does not exist.
.contains_exact
,.identical
,.overlaps
: missing INPUT: section (parameterother
)Thanks. But note that the formal requirement of pointless INPUT/OUTPUT sections in docstrings is apparently going to be relaxed (#19041), so I'm not sure we need them INPUT sections every time you noted they were missing.
I know; the question is what is obvious and what not. My feelings are not very strong here.
comment:14 Changed 5 years ago by
 Branch changed from u/cheuberg/19063acb to u/mmezzarobba/19063acb
 Commit changed from a33ceb40f76cd57791ad9e950fec234373278057 to ecc46368f1368266fdc438c3266a4fe7f488be5f
 Status changed from needs_work to needs_review
New commits:
ecc4636  #19063 Implement reviewer's comments

comment:16 Changed 5 years ago by
Thanks for the review!
comment:17 Changed 5 years ago by
 Description modified (diff)
comment:18 Changed 5 years ago by
 Milestone changed from sage6.9 to sagepending
comment:19 Changed 5 years ago by
 Branch changed from u/mmezzarobba/19063acb to u/cheuberg/19063acb
comment:20 followup: ↓ 22 Changed 5 years ago by
 Commit changed from ecc46368f1368266fdc438c3266a4fe7f488be5f to 8479dd21f1709c0a9f4e234eb7b280dc6e39ca78
I pushed a fix for the doctest issue on 32bit (cf. #18546) but have not yet tested it under 32 bit.
New commits:
91c5184  Trac #18546: remove optional  arb in real_mpfi

dbae43d  Trac #18546: make doctests 32bit aware

a872274  Trac #19063: Merge #18546 in preparation of doctest fix for 32 bit

8479dd2  Trac #19063: fix doctests on 32 bit

comment:21 Changed 5 years ago by
 Status changed from positive_review to needs_review
comment:22 in reply to: ↑ 20 Changed 5 years ago by
Replying to cheuberg:
but have not yet tested it under 32 bit.
tests on my (virtual) 32 bit machine pass.
comment:23 Changed 5 years ago by
 Milestone changed from sagepending to sage6.9
comment:24 Changed 5 years ago by
 Reviewers changed from Clemens Heuberger to Clemens Heuberger, Marc Mezzarobba
 Status changed from needs_review to positive_review
comment:25 Changed 5 years ago by
Just a comment for a followup: it would be better to use method names more consistent with RIF
/CIF
:
above_abs
andbelow_abs
should be renamedmagnitude
andmignitude
for analogy withRIF
and, after #19403, alsoCIF
.mid
should becenter
comment:26 followup: ↓ 27 Changed 5 years ago by
Or perhaps above_abs
is not exactly the same as magnitude
since the latter returns a real number instead of a ball. Still, it would be nice to implement magnitude
like RIF
.
comment:27 in reply to: ↑ 26 ; followup: ↓ 30 Changed 5 years ago by
Replying to jdemeyer:
Or perhaps
above_abs
is not exactly the same asmagnitude
since the latter returns a real number instead of a ball. Still, it would be nice to implementmagnitude
likeRIF
.
Yes.
Regarding center()
, I'd prefer to keep the name mid()
, if only to emphazise that the midpoint is part of the defining data of the ball (and thus always is exact, while mpfi_mid()
involves a rounding), but I have nothing against adding center()
as an alias.
comment:28 Changed 5 years ago by
 Branch changed from u/cheuberg/19063acb to u/mmezzarob/19063acb
 Commit 8479dd21f1709c0a9f4e234eb7b280dc6e39ca78 deleted
comment:29 followup: ↓ 32 Changed 5 years ago by
It looks like the trac server ignored my push for some reason, but as far as I can tell the branch merges cleanly.
comment:30 in reply to: ↑ 27 Changed 5 years ago by
Replying to mmezzarobba:
Replying to jdemeyer:
Or perhaps
above_abs
is not exactly the same asmagnitude
since the latter returns a real number instead of a ball. Still, it would be nice to implementmagnitude
likeRIF
.Yes.
Regarding
center()
, I'd prefer to keep the namemid()
, if only to emphazise that the midpoint is part of the defining data of the ball (and thus always is exact, whilempfi_mid()
involves a rounding), but I have nothing against addingcenter()
as an alias.
For most practical purposes, the rounding or not is just an implementation detail. Personally, I would prefer to use just one method name (for example, I don't like that some types implement prec()
and some implement precision()
but not all implement both: that's just confusing). Given that center()
has been around for a while, I would prefer to use center()
(and only that) also for arb.
comment:31 Changed 5 years ago by
By the way, I might be interesting in reviewing this ticket but only after the dependencies have been merged.
comment:32 in reply to: ↑ 29 ; followup: ↓ 33 Changed 5 years ago by
 Status changed from positive_review to needs_work
Replying to mmezzarobba:
It looks like the trac server ignored my push for some reason, but as far as I can tell the branch merges cleanly.
There is no branch...
comment:33 in reply to: ↑ 32 Changed 5 years ago by
 Branch changed from u/mmezzarob/19063acb to u/mmezzarobba/19063acb
 Commit set to d13e70facf6b65b6655c91a5f87a61f4faf1d02f
Replying to jdemeyer:
Replying to mmezzarobba:
It looks like the trac server ignored my push for some reason, but as far as I can tell the branch merges cleanly.
There is no branch...
Oops, now I understand. Thanks!
Last 10 new commits:
a7e94c2  ComplexBallField: minor documentation improvements

ea906fd  Trac #19063: Fix indentation in doctest

5886d73  Trac #19063: Simplify doctest

a2bf307  Trac #19063: RESt formatting

57a1c38  Trac #19063: Replace "nonzero" by "True"

2dccd9b  fixup: true/false

48c70da  Trac #19063: replace a few "int" by "bint"

91f9282  Trac #19063: Replace interval by ball in title

08d3d29  #19063 Implement reviewer's comments

d13e70f  Trac #19063: fix doctests on 32 bit

comment:34 Changed 5 years ago by
 Status changed from needs_work to positive_review
comment:35 Changed 5 years ago by
 Branch changed from u/mmezzarobba/19063acb to u/cheuberg/19063acb
comment:36 Changed 5 years ago by
 Commit changed from d13e70facf6b65b6655c91a5f87a61f4faf1d02f to 1091bfcc54fb9164a405ba0e1cb933037797bec7
 Milestone changed from sage6.9 to sage6.10
 Status changed from positive_review to needs_review
comment:37 Changed 5 years ago by
 Branch changed from u/cheuberg/19063acb to trac/u/mmezzarobba/19063acb
 Commit 1091bfcc54fb9164a405ba0e1cb933037797bec7 deleted
replacing the branch by my version of the merge because I also rebased the followups
comment:38 Changed 5 years ago by
 Branch changed from trac/u/mmezzarobba/19063acb to u/mmezzarobba/19063acb
 Commit set to fa41787b7f670f82ba79ad2fa9ebe67ef5c2eb24
comment:39 Changed 5 years ago by
 Status changed from needs_review to positive_review
content of both branches is equal. Thus back to positive.
comment:40 Changed 5 years ago by
 Branch changed from u/mmezzarobba/19063acb to fa41787b7f670f82ba79ad2fa9ebe67ef5c2eb24
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
ComplexBallField: implement some_elements()
Fix RealBall.mid(), implement ComplexBall.{mid(),squash()
}ComplexBall: round(), accuracy(), trim()
ComplexBall: conversions to ZZ, RR, RIF, CC, CIF
ComplexBallField: faster access to real field
{Real,Complex}Ball: implement conversions to QQ
ComplexBall: containment and overlapping predicates
ComplexBall: abs(), arg()
RealBall: implement abs()
{Real,Complex}Ball: implement {above,below}_abs()