Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coercions and basic arithmetic for complex balls #19063

Closed
mezzarobba opened this issue Aug 20, 2015 · 51 comments
Closed

Coercions and basic arithmetic for complex balls #19063

mezzarobba opened this issue Aug 20, 2015 · 51 comments

Comments

@mezzarobba
Copy link
Member

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.

Depends on #18546

CC: @cheuberg

Component: numerical

Keywords: arb

Author: Marc Mezzarobba, Clemens Heuberger

Branch/Commit: fa41787

Reviewer: Clemens Heuberger, Marc Mezzarobba

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

@mezzarobba mezzarobba added this to the sage-6.9 milestone Aug 20, 2015
@mezzarobba
Copy link
Member Author

Branch: u/mmezzarobba/19063-acb

@mezzarobba
Copy link
Member Author

Commit: 8732ecb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2015

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2015

Changed commit from 8732ecb to 448bf24

@mezzarobba
Copy link
Member Author

Changed keywords from none to arb

@mezzarobba
Copy link
Member Author

Author: Marc Mezzarobba

@mezzarobba

This comment has been minimized.

@cheuberg
Copy link
Contributor

cheuberg commented Sep 4, 2015

comment:6

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2015

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

0196f69ComplexBallField: minor documentation improvements

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2015

Changed commit from 448bf24 to 0196f69

@cheuberg
Copy link
Contributor

cheuberg commented Sep 7, 2015

Changed branch from u/mmezzarobba/19063-acb to u/cheuberg/19063-acb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2015

Changed commit from 0196f69 to a33ceb4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2015

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

@cheuberg
Copy link
Contributor

cheuberg commented Sep 7, 2015

comment:10

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)

@cheuberg
Copy link
Contributor

cheuberg commented Sep 7, 2015

Reviewer: Clemens Heuberger

@mezzarobba
Copy link
Member Author

comment:11

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.

@cheuberg
Copy link
Contributor

cheuberg commented Sep 7, 2015

comment:13

Replying to @mezzarobba:

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.

@mezzarobba
Copy link
Member Author

Changed branch from u/cheuberg/19063-acb to u/mmezzarobba/19063-acb

@mezzarobba
Copy link
Member Author

New commits:

ecc4636#19063 Implement reviewer's comments

@mezzarobba
Copy link
Member Author

Changed commit from a33ceb4 to ecc4636

@jdemeyer
Copy link
Contributor

comment:30

Replying to @mezzarobba:

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.

@jdemeyer
Copy link
Contributor

comment:31

By the way, I might be interesting in reviewing this ticket but only after the dependencies have been merged.

@jdemeyer
Copy link
Contributor

comment:32

Replying to @mezzarobba:

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

@mezzarobba
Copy link
Member Author

Commit: d13e70f

@mezzarobba
Copy link
Member Author

Changed branch from u/mmezzarob/19063-acb to u/mmezzarobba/19063-acb

@mezzarobba
Copy link
Member Author

comment:33

Replying to @jdemeyer:

Replying to @mezzarobba:

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

@cheuberg
Copy link
Contributor

Changed branch from u/mmezzarobba/19063-acb to u/cheuberg/19063-acb

@cheuberg
Copy link
Contributor

comment:36

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

@cheuberg
Copy link
Contributor

Changed commit from d13e70f to 1091bfc

@mezzarobba
Copy link
Member Author

Changed commit from 1091bfc to none

@mezzarobba
Copy link
Member Author

Changed branch from u/cheuberg/19063-acb to trac/u/mmezzarobba/19063-acb

@mezzarobba
Copy link
Member Author

comment:37

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

@mezzarobba
Copy link
Member Author

Changed branch from trac/u/mmezzarobba/19063-acb to u/mmezzarobba/19063-acb

@mezzarobba
Copy link
Member Author

Commit: fa41787

@cheuberg
Copy link
Contributor

comment:39

content of both branches is equal. Thus back to positive.

@vbraun
Copy link
Member

vbraun commented Oct 30, 2015

Changed branch from u/mmezzarobba/19063-acb to fa41787

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants