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:  sage8.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: 
Description (last modified by )
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
 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
comment:2 followup: ↓ 3 Changed 4 years ago by
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 ; followup: ↓ 4 Changed 4 years ago by
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 ; followup: ↓ 5 Changed 4 years ago by
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 ; followup: ↓ 7 Changed 4 years ago by
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
 Commit changed from 39c78d1943bfb31bb0e60038c2380eb9ea6b12ce to c97bfcf8b06e3f8dd5ff412b9d2ac40a5a37d411
comment:7 in reply to: ↑ 5 ; followup: ↓ 8 Changed 4 years ago by
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.
comment:8 in reply to: ↑ 7 Changed 4 years ago by
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
 Cc cheuberg added
comment:10 Changed 4 years ago by
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:12 Changed 4 years ago by
 Commit changed from c97bfcf8b06e3f8dd5ff412b9d2ac40a5a37d411 to 08cd02d29429f1ad4c30f05969a8d327085809da
comment:14 followup: ↓ 15 Changed 4 years ago by
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 ; followup: ↓ 16 Changed 4 years ago by
Replying to vdelecroix:
You should be using
PY_NEW(Integer)
and notInteger.__new__(Integer)
(unless something changed around Cython abouttp_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 ; followup: ↓ 17 Changed 4 years ago by
Replying to mmezzarobba:
Replying to vdelecroix:
You should be using
PY_NEW(Integer)
and notInteger.__new__(Integer)
(unless something changed around Cython abouttp_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
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
 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

f3dedd1  complex_arb: sage.rings.integer.Integer → Integer

d8a698e  complex_arb: Integer.__new__ → PY_NEW

d4cb5b9  #24717 implement ComplexBall.pow(expo, analytic=...)

comment:19 Changed 4 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
comment:21 Changed 4 years ago by
 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

8164105  complex_arb: sage.rings.integer.Integer → Integer

aa833ee  complex_arb: Integer.__new__ → PY_NEW

93679c5  #24717 implement ComplexBall.pow(expo, analytic=...)

comment:22 Changed 4 years ago by
 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
 Status changed from positive_review to needs_work
Merge conflict (wait for the next beta)
comment:24 Changed 4 years ago by
 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

cb84329  complex_arb: sage.rings.integer.Integer → Integer

4b8df54  complex_arb: Integer.__new__ → PY_NEW

e174e59  #24717 implement ComplexBall.pow(expo, analytic=...)

comment:25 Changed 4 years ago by
 Status changed from needs_work to positive_review
Ok, it was a trivial conflict.
comment:26 Changed 4 years ago by
 Status changed from positive_review to needs_work
[sagelib8.3.beta0] Error compiling Cython file: [sagelib8.3.beta0]  [sagelib8.3.beta0] ... [sagelib8.3.beta0] sage: ZZ(CBF(i)) [sagelib8.3.beta0] Traceback (most recent call last): [sagelib8.3.beta0] ... [sagelib8.3.beta0] ValueError: 1.000000000000000*I does not contain a unique integer [sagelib8.3.beta0] """ [sagelib8.3.beta0] cdef Integer res [sagelib8.3.beta0] ^ [sagelib8.3.beta0]  [sagelib8.3.beta0] [sagelib8.3.beta0] sage/rings/complex_arb.pyx:1512:13: 'Integer' is not a type identifier
comment:27 Changed 4 years ago by
 Commit changed from e174e59c17404f5e326542fdcae41c7afa43a33a to c827491eb4926640cae329942155cf2308d049c7
comment:29 Changed 4 years ago by
 Branch changed from u/mmezzarobba/acb_cuts to c827491eb4926640cae329942155cf2308d049c7
 Resolution set to fixed
 Status changed from positive_review to closed
Let's hope I didn't get too many branch cuts wrong...
Last 10 new commits:
#24686 robustify, clean up, document rigorous integration using arb
doc markup fixes
another doc fix
add a few more doctest examples
#24686 minor fixes
real_arb: use arb 2.6+ comparison functions
complex_arb: use arb 2.6+ comparison functions
stop considering inexact balls equal to themselves
Merge branch 'arb_cmp' into cuts
#24717 support the "analytic" flag in some methods of complex balls