Opened 4 years ago

Closed 4 years ago

#24717 closed enhancement (fixed)

Branch cuts of functions on ComplexBalls

Reported by: mmezzarobba Owned by:
Priority: major Milestone: sage-8.2
Component: numerical Keywords:
Cc: fredrik.johansson, vdelecroix, cheuberg Merged in:
Authors: Marc Mezzarobba Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: c827491 (Commits, GitHub, GitLab) Commit: c827491eb4926640cae329942155cf2308d049c7
Dependencies: #24627, #24686 Stopgaps:

Status badges

Description (last modified by mmezzarobba)

Add support for the analytic flag required by the rigorous integration code in some methods of complex balls.

Change History (29)

comment:1 Changed 4 years ago by mmezzarobba

  • Authors set to Marc Mezzarobba
  • Branch set to u/mmezzarobba/acb_cuts
  • Cc fredrik.johansson vdelecroix added
  • Commit set to 39c78d1943bfb31bb0e60038c2380eb9ea6b12ce
  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from Branch cuts of functions on ComplexBallFields to Branch cuts of functions on ComplexBalls

Let's hope I didn't get too many branch cuts wrong...


Last 10 new commits:

374a942#24686 robustify, clean up, document rigorous integration using arb
376811ddoc markup fixes
0902f14another doc fix
1d6ca9cadd a few more doctest examples
120bcb4#24686 minor fixes
16962e5real_arb: use arb 2.6+ comparison functions
ac4ba37complex_arb: use arb 2.6+ comparison functions
b55e3c2stop considering inexact balls equal to themselves
09f788cMerge branch 'arb_cmp' into cuts
39c78d1#24717 support the "analytic" flag in some methods of complex balls

comment:2 follow-up: Changed 4 years ago by fredrik.johansson

This looks great. I would just suggest adding a pow method with this flag as well, and perhaps documenting some examples for .integral() where these are used (examples with .integral() could also be used as doctests for the individual methods).

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

Replying to fredrik.johansson:

This looks great. I would just suggest adding a pow method with this flag as well,

Yes, and there are several other methods that could implement analytic=True--but I thought it would be better to have some easy ones now rather than all later. (Though, thinking of it, pow wouldn't be that hard to support.)

and perhaps documenting some examples for .integral() where these are used (examples with .integral() could also be used as doctests for the individual methods).

Either I don't understand what you mean, or there are already one or two such examples.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by fredrik.johansson

Replying to mmezzarobba:

Replying to fredrik.johansson:

This looks great. I would just suggest adding a pow method with this flag as well,

Yes, and there are several other methods that could implement analytic=True--but I thought it would be better to have some easy ones now rather than all later. (Though, thinking of it, pow wouldn't be that hard to support.)

Indeed, but pow is perhaps the most important of them all...

and perhaps documenting some examples for .integral() where these are used (examples with .integral() could also be used as doctests for the individual methods).

Either I don't understand what you mean, or there are already one or two such examples.

Thanks, I missed that.

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

Replying to fredrik.johansson:

Indeed, but pow is perhaps the most important of them all...

Yes, but it is also a bit harder to implement due to the need to share code with the __pow__, which participates in the coercion system... but ok, I'll try to see what I can do.

comment:6 Changed 4 years ago by git

  • Commit changed from 39c78d1943bfb31bb0e60038c2380eb9ea6b12ce to c97bfcf8b06e3f8dd5ff412b9d2ac40a5a37d411

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

c566ca7complex_arb: sage.rings.integer.Integer -> Integer
c97bfcf#24717 implement ComplexBall.pow(expo, analytic=...)

comment:7 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by tscrim

Replying to mmezzarobba:

Replying to fredrik.johansson:

Indeed, but pow is perhaps the most important of them all...

Yes, but it is also a bit harder to implement due to the need to share code with the __pow__, which participates in the coercion system... but ok, I'll try to see what I can do.

If you implement __pow__, you bypass the coercion system altogether (similar to if you implement __mul__). ADDENDUM - Unless of course you call the coercion framework yourself.

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

comment:8 in reply to: ↑ 7 Changed 4 years ago by mmezzarobba

Replying to tscrim:

ADDENDUM - Unless of course you call the coercion framework yourself.

Yes, that's what both the previous code and my patch are doing.

comment:9 Changed 4 years ago by mmezzarobba

  • Cc cheuberg added

comment:10 Changed 4 years ago by vdelecroix

I agree with comment:2 that this would better be tested in conjunction with numerical integration.

Moreover, does it work correctly with function compositions? Could I integrate cos(exp(x)) using

lambda z,f: z.exp(f).cos(f)

If so, we shoud add examples in CBF.integral.

comment:11 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

comment:12 Changed 4 years ago by git

  • Commit changed from c97bfcf8b06e3f8dd5ff412b9d2ac40a5a37d411 to 08cd02d29429f1ad4c30f05969a8d327085809da

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

3b26420#24717 support the "analytic" flag in some methods of complex balls
aad5a45complex_arb: sage.rings.integer.Integer -> Integer
08cd02d#24717 implement ComplexBall.pow(expo, analytic=...)

comment:13 Changed 4 years ago by mmezzarobba

  • Status changed from needs_work to needs_review

thanks, rebased!

comment:14 follow-up: Changed 4 years ago by vdelecroix

You should be using PY_NEW(Integer) and not Integer.__new__(Integer) (unless something changed around Cython about tp_new).

It would be nice to have usage example in the integral docstring.

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

Replying to vdelecroix:

You should be using PY_NEW(Integer) and not Integer.__new__(Integer) (unless something changed around Cython about tp_new).

Can you explain the difference? I thought Integer.__new__(Integer) had been working already for a long time, and

$ git grep 'Integer.__new__' | wc -l
95

It would be nice to have usage example in the integral docstring.

See above :-)

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

Replying to mmezzarobba:

Replying to vdelecroix:

You should be using PY_NEW(Integer) and not Integer.__new__(Integer) (unless something changed around Cython about tp_new).

Can you explain the difference? I thought Integer.__new__(Integer) had been working already for a long time, and

$ git grep 'Integer.__new__' | wc -l
95

On the other hand, this is the current PY_NEW documentation

    Return ``t.__new__(t)``.  This works even for types like
    :class:`Integer` where we change ``tp_new`` at runtime (Cython
    optimizations assume that ``tp_new`` doesn't change).

It would be nice to have usage example in the integral docstring.

See above :-)

Oups.

comment:17 in reply to: ↑ 16 Changed 4 years ago by mmezzarobba

Replying to vdelecroix:

On the other hand, this is the current PY_NEW documentation

Ok. This has nothing to do with this ticket (I'm just changing s.r.i.Integer to Integer in an existing call), but let's be safe.

comment:18 Changed 4 years ago by git

  • Commit changed from 08cd02d29429f1ad4c30f05969a8d327085809da to d4cb5b9ba60bcb7f83885d44917970e4784d6b36

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

9305c90#24717 support the "analytic" flag in some methods of complex balls
f3dedd1complex_arb: sage.rings.integer.Integer → Integer
d8a698ecomplex_arb: Integer.__new__ → PY_NEW
d4cb5b9#24717 implement ComplexBall.pow(expo, analytic=...)

comment:19 Changed 4 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

comment:20 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:21 Changed 4 years ago by git

  • Commit changed from d4cb5b9ba60bcb7f83885d44917970e4784d6b36 to 93679c501b239650ce8b3f58352b37a92ca90259

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

7023769#24717 support the "analytic" flag in some methods of complex balls
8164105complex_arb: sage.rings.integer.Integer → Integer
aa833eecomplex_arb: Integer.__new__ → PY_NEW
93679c5#24717 implement ComplexBall.pow(expo, analytic=...)

comment:22 Changed 4 years ago by mmezzarobba

  • Status changed from needs_work to positive_review

Rebased (but without conflict for me: are you seeing a merge conflict with develop, or was that with additional changes?).

comment:23 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict (wait for the next beta)

comment:24 Changed 4 years ago by git

  • Commit changed from 93679c501b239650ce8b3f58352b37a92ca90259 to e174e59c17404f5e326542fdcae41c7afa43a33a

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

c921c89#24717 support the "analytic" flag in some methods of complex balls
cb84329complex_arb: sage.rings.integer.Integer → Integer
4b8df54complex_arb: Integer.__new__ → PY_NEW
e174e59#24717 implement ComplexBall.pow(expo, analytic=...)

comment:25 Changed 4 years ago by mmezzarobba

  • Status changed from needs_work to positive_review

Ok, it was a trivial conflict.

comment:26 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work
[sagelib-8.3.beta0] Error compiling Cython file:
[sagelib-8.3.beta0] ------------------------------------------------------------
[sagelib-8.3.beta0] ...
[sagelib-8.3.beta0]             sage: ZZ(CBF(i))
[sagelib-8.3.beta0]             Traceback (most recent call last):
[sagelib-8.3.beta0]             ...
[sagelib-8.3.beta0]             ValueError: 1.000000000000000*I does not contain a unique integer
[sagelib-8.3.beta0]         """
[sagelib-8.3.beta0]         cdef Integer res
[sagelib-8.3.beta0]             ^
[sagelib-8.3.beta0] ------------------------------------------------------------
[sagelib-8.3.beta0] 
[sagelib-8.3.beta0] sage/rings/complex_arb.pyx:1512:13: 'Integer' is not a type identifier

comment:27 Changed 4 years ago by git

  • Commit changed from e174e59c17404f5e326542fdcae41c7afa43a33a to c827491eb4926640cae329942155cf2308d049c7

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

9395581complex_arb: sage.rings.integer.Integer → Integer
19005f5complex_arb: Integer.__new__ → PY_NEW
c827491#24717 implement ComplexBall.pow(expo, analytic=...)

comment:28 Changed 4 years ago by mmezzarobba

  • Status changed from needs_work to positive_review

Sorry.

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

comment:29 Changed 4 years ago by vbraun

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