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

Implement field operations, basic complex number and ball methods (abs arg, conjugate, mid, rad...), and some conversions and coercions for complex balls. Also make a few changes to real balls for consistency.

Follow-up tickets: #19152, #19082.

Change History (40)

comment:1 Changed 5 years ago by mmezzarobba

  • Branch set to u/mmezzarobba/19063-acb
  • Commit set to 8732ecb7f610a5e3c2d2d350cd83a73180b5e8be
Last edited 5 years ago by mmezzarobba (previous) (diff)

comment:2 Changed 5 years ago by git

  • Commit changed from 8732ecb7f610a5e3c2d2d350cd83a73180b5e8be to 448bf24a279abd46d4afdf6d9e0e2c01693da1f5

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

9524687ComplexBallField: implement some_elements()
ad62199Fix RealBall.mid(), implement ComplexBall.{mid(),squash()}
c513c70ComplexBall: round(), accuracy(), trim()
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()

comment:3 Changed 5 years ago by mmezzarobba

  • Authors set to Marc Mezzarobba
  • Keywords arb added
  • Status changed from new to needs_review
Last edited 5 years ago by mmezzarobba (previous) (diff)

comment:4 Changed 5 years ago by mmezzarobba

  • Description modified (diff)

comment:5 Changed 5 years ago by mmezzarobba

  • Cc cheuberg added

comment:6 Changed 5 years ago by cheuberg

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(): one-sentence-description: Replace "Returns" by "Return" (see developper guide)
  • ComplexBallField.gen(), .gens(), .ngens(): Missing one-sentence-description

comment:7 Changed 5 years ago by git

  • Commit changed from 448bf24a279abd46d4afdf6d9e0e2c01693da1f5 to 0196f69eb2ab98bd6468bfd0152396d7c5d1e89d

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

0196f69ComplexBallField: minor documentation improvements

comment:8 Changed 5 years ago by cheuberg

  • Branch changed from u/mmezzarobba/19063-acb to u/cheuberg/19063-acb

comment:9 Changed 5 years ago by git

  • Commit changed from 0196f69eb2ab98bd6468bfd0152396d7c5d1e89d to a33ceb40f76cd57791ad9e950fec234373278057

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

6f7bea0Trac #19063: Fix indentation in doctest
80fcc47Trac #19063: Simplify doctest
cacf2ceTrac #19063: RESt formatting
d05b9a2Trac #19063: Replace "nonzero" by "True"
e214cc9fixup: true/false
b52f52aTrac #19063: replace a few "int" by "bint"
a33ceb4Trac #19063: Replace interval by ball in title

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

  • 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__: why Element.__init__ instead of RingElement.__init__
  • .some_elements: Is 1/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 to 1 + [-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 (parameter parent)
  • _real_mpfi_: missing INPUT: section (parameter parent), One-sentence-description mentions "real number" instead of "real interval".
  • _mpfr_: missing INPUT: section (parameter parent)
  • .mid, .squash: missing OUTPUT: section
  • .mid should have "see also" section (.squash)
  • .identical refers to a method contains which does not exist.
  • .contains_exact, .identical, .overlaps: missing INPUT: section (parameter other)

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

Replying to cheuberg:

I have now reviewed this patch and added a few reviewer commits.

Thanks!

Questions:

  • ComplexBall.__init__: why Element.__init__ instead of RingElement.__init__

Because I neglected to change it after changing the parent class. (I hate Python...)

  • .some_elements: Is 1/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 to 1 + [-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 (parameter parent)
  • _real_mpfi_: missing INPUT: section (parameter parent), One-sentence-description mentions "real number" instead of "real interval".
  • _mpfr_: missing INPUT: section (parameter parent)
  • .mid, .squash: missing OUTPUT: section
  • .mid should have "see also" section (.squash)
  • .identical refers to a method contains which does not exist.
  • .contains_exact, .identical, .overlaps: missing INPUT: section (parameter other)

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 mmezzarobba

  • Status changed from needs_info to needs_work

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

Replying to mmezzarobba:

Questions:

  • ComplexBall.__init__: why Element.__init__ instead of RingElement.__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 to 1 + [-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 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.

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 (parameter parent)
  • _real_mpfi_: missing INPUT: section (parameter parent), One-sentence-description mentions "real number" instead of "real interval".
  • _mpfr_: missing INPUT: section (parameter parent)
  • .mid, .squash: missing OUTPUT: section
  • .mid should have "see also" section (.squash)
  • .identical refers to a method contains which does not exist.
  • .contains_exact, .identical, .overlaps: missing INPUT: section (parameter other)

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 mmezzarobba

  • Branch changed from u/cheuberg/19063-acb to u/mmezzarobba/19063-acb
  • Commit changed from a33ceb40f76cd57791ad9e950fec234373278057 to ecc46368f1368266fdc438c3266a4fe7f488be5f
  • Status changed from needs_work to needs_review

New commits:

ecc4636#19063 Implement reviewer's comments

comment:15 Changed 5 years ago by cheuberg

  • Status changed from needs_review to positive_review

fine for me.

comment:16 Changed 5 years ago by mmezzarobba

Thanks for the review!

comment:17 Changed 5 years ago by mmezzarobba

  • Description modified (diff)

comment:18 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-6.9 to sage-pending

comment:19 Changed 5 years ago by cheuberg

  • Branch changed from u/mmezzarobba/19063-acb to u/cheuberg/19063-acb

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

  • Commit changed from ecc46368f1368266fdc438c3266a4fe7f488be5f to 8479dd21f1709c0a9f4e234eb7b280dc6e39ca78

I pushed a fix for the doctest issue on 32-bit (cf. #18546) but have not yet tested it under 32 bit.


New commits:

91c5184Trac #18546: remove optional - arb in real_mpfi
dbae43dTrac #18546: make doctests 32-bit aware
a872274Trac #19063: Merge #18546 in preparation of doctest fix for 32 bit
8479dd2Trac #19063: fix doctests on 32 bit

comment:21 Changed 5 years ago by cheuberg

  • Status changed from positive_review to needs_review

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

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 cheuberg

  • Milestone changed from sage-pending to sage-6.9

comment:24 Changed 5 years ago by mmezzarobba

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

Just a comment for a follow-up: it would be better to use method names more consistent with RIF/CIF:

  • above_abs and below_abs should be renamed magnitude and mignitude for analogy with RIF and, after #19403, also CIF.
  • mid should be center
Last edited 5 years ago by jdemeyer (previous) (diff)

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

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

Replying to jdemeyer:

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.

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 mmezzarobba

  • Branch changed from u/cheuberg/19063-acb to u/mmezzarob/19063-acb
  • Commit 8479dd21f1709c0a9f4e234eb7b280dc6e39ca78 deleted

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

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 jdemeyer

Replying to mmezzarobba:

Replying to jdemeyer:

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.

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.

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 jdemeyer

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

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

  • Branch changed from u/mmezzarob/19063-acb to u/mmezzarobba/19063-acb
  • 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:

a7e94c2ComplexBallField: minor documentation improvements
ea906fdTrac #19063: Fix indentation in doctest
5886d73Trac #19063: Simplify doctest
a2bf307Trac #19063: RESt formatting
57a1c38Trac #19063: Replace "nonzero" by "True"
2dccd9bfixup: true/false
48c70daTrac #19063: replace a few "int" by "bint"
91f9282Trac #19063: Replace interval by ball in title
08d3d29#19063 Implement reviewer's comments
d13e70fTrac #19063: fix doctests on 32 bit

comment:34 Changed 5 years ago by mmezzarobba

  • Status changed from needs_work to positive_review

comment:35 Changed 5 years ago by cheuberg

  • Branch changed from u/mmezzarobba/19063-acb to u/cheuberg/19063-acb

comment:36 Changed 5 years ago by cheuberg

  • Commit changed from d13e70facf6b65b6655c91a5f87a61f4faf1d02f to 1091bfcc54fb9164a405ba0e1cb933037797bec7
  • Milestone changed from sage-6.9 to sage-6.10
  • Status changed from positive_review to needs_review

Did no longer merge cleanly with latest version of dependency #18546. Please check the merge.


New commits:

03a7a4fMerge tag '6.10.beta0' into t/18546/18546-arb-std
1091bfcMerge branch 't/18546/18546-arb-std-rebased' into t/19063/19063-acb

comment:37 Changed 5 years ago by mmezzarobba

  • Branch changed from u/cheuberg/19063-acb to trac/u/mmezzarobba/19063-acb
  • Commit 1091bfcc54fb9164a405ba0e1cb933037797bec7 deleted

replacing the branch by my version of the merge because I also rebased the follow-ups

comment:38 Changed 5 years ago by mmezzarobba

  • Branch changed from trac/u/mmezzarobba/19063-acb to u/mmezzarobba/19063-acb
  • Commit set to fa41787b7f670f82ba79ad2fa9ebe67ef5c2eb24

comment:39 Changed 5 years ago by cheuberg

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

  • Branch changed from u/mmezzarobba/19063-acb to fa41787b7f670f82ba79ad2fa9ebe67ef5c2eb24
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.