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

residue: mathematically wrong output #20084

Closed
behackl opened this issue Feb 18, 2016 · 18 comments
Closed

residue: mathematically wrong output #20084

behackl opened this issue Feb 18, 2016 · 18 comments

Comments

@behackl
Copy link
Member

behackl commented Feb 18, 2016

The complex function f(s) = 1/(1 - 2^(-s)) has poles of residue 1/log(2) at s = 2*k*pi*I/log(2) for all integers k.

Currently sage recognize these poles just at s=0:

sage: f(s).residue(s==0)
1/log(2)
sage: f(s).residue(s==2*pi*I/log(2))
0

In essence, this happens because the series-method does not recognize the pole. The priority is critical because mathematically wrong output is produced.

CC: @rwst @cheuberg

Component: symbolics

Author: Benjamin Hackl

Branch/Commit: 68fea61

Reviewer: Frédéric Chapoton

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

@behackl behackl added this to the sage-7.1 milestone Feb 18, 2016
@behackl
Copy link
Member Author

behackl commented Feb 18, 2016

comment:1

The most elegant solution would of course be the automatic simplification of expressions like 2^(something/log(2)) --> exp(something), such that

sage: 2^(2*pi*I/log(2))
1

However, I'm not sure if that can be achieved so easily.

A second suggestion would be something like

diff --git a/src/sage/symbolic/expression.pyx b/src/sage/symbolic/expression.pyx
index 3a5c864..175bcad 100644
--- a/src/sage/symbolic/expression.pyx
+++ b/src/sage/symbolic/expression.pyx
@@ -4076,7 +4076,7 @@ cdef class Expression(CommutativeRingElement):
             a = 0
         if a == infinity:
             return (-self.subs({x: 1/x}) / x**2).residue(x == 0)
-        return self.subs({x: x+a}).series(x == 0, 0).coefficient(x, -1)
+        return self.subs({x: x+a}).canonicalize_radical().series(x == 0, 0).coefficient(x, -1)
 
     def taylor(self, *args):
         r"""

where there are several ways to refine this approach:

  • introduce an additional keyword that could disable this additional simplification such that performance does not suffer necessarily,
  • only try to simplify for complex arguments of a

and so on.

Thoughts?

@behackl
Copy link
Member Author

behackl commented Feb 18, 2016

@behackl
Copy link
Member Author

behackl commented Feb 18, 2016

comment:2

This is just the quick workaround from above.


New commits:

45d16e2simplify substituted expression before series expansion

@behackl
Copy link
Member Author

behackl commented Feb 18, 2016

Author: Benjamin Hackl

@behackl
Copy link
Member Author

behackl commented Feb 18, 2016

Commit: 45d16e2

@williamstein
Copy link
Contributor

comment:3

Please add an example doctest to illustrate what you're fixing.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2016

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

8719364add a doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2016

Changed commit from 45d16e2 to 8719364

@rwst
Copy link

rwst commented Mar 5, 2016

comment:5

Is it ready for review?

@mforets
Copy link
Mannequin

mforets mannequin commented Apr 13, 2017

comment:6

update from the future:

sage: (1/(1 - 2^-x)).residue(x == 2*pi*I/log(2))
1/log(2)
sage: version()
'SageMath version 7.6.beta6, Release Date: 2017-03-03'

some updates in series may affect that now it can recognize the pole without having to call canonicalize_radical()?

@behackl
Copy link
Member Author

behackl commented Apr 13, 2017

comment:7

Yes, I have faint memory of improving something with series itself some time ago; thanks for reminding me.

Actually, there are more problems with residue. Take, for example,

sage: (1/sqrt(x^2)).residue(x==0)
0
sage: (1/sqrt(x^2)).canonicalize_radical().residue(x==0)
1

(Note that this is, in some sense, a pathological example.)

The ideal fix in this case (IMHO) would be to throw an error that the residue could not be computed as the "correct" branch of the root could not be chosen automatically. I think that just applying canonicalize_radical before computing the residue just introduces more grief as the user looses all control over which branch is chosen.

In any case: the original problem reported with this ticket is solved, so this is probably just wontfix. Other opinions?

Replying to @mforets:

update from the future:

sage: (1/(1 - 2^-x)).residue(x == 2*pi*I/log(2))
1/log(2)
sage: version()
'SageMath version 7.6.beta6, Release Date: 2017-03-03'

some updates in series may affect that now it can recognize the pole without having to call canonicalize_radical()?

@mforets
Copy link
Mannequin

mforets mannequin commented Apr 17, 2017

comment:8

Replying to @behackl:

Yes, I have faint memory of improving something with series itself some time ago; thanks for reminding me.
...
In any case: the original problem reported with this ticket is solved, so this is probably just wontfix. Other opinions?

cool. for what i've been told from other tickets, i suppose this one should should still provide the test (not wontfix):

Check that :trac:`20084` is fixed::

    sage: (1/(1 - 2^-x)).residue(x == 2*pi*I/log(2))
    1/log(2)

Actually, there are more problems with residue.
...
(Note that this is, in some sense, a pathological example.)

yes. one could come up with other examples (e.g. examples involving log(z)). i agree that just applying canonicalize_radical is not a good idea for the reason you stated. but is there a simple way to identify these cases without much hurdle? does this belong to series method or is sth that can be done at the level of residue?

@mforets
Copy link
Mannequin

mforets mannequin commented May 5, 2017

comment:9

@behackl : if you remove the canonicalize_radical keeping the test, i can review it! (or the other way round).

wrt handling pathological examples, yes IMO it is a relevant future task, although as pointed out before i cannot help right now, since i'm not visualizing a workaround :/

@fchapoton
Copy link
Contributor

Changed branch from u/behackl/symbolics/residue/exp-complex-poles to public/20084

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:10

looks good to me


New commits:

311e284Merge branch 'u/behackl/symbolics/residue/exp-complex-poles' in 8.0.b6
68fea61trac 20084 back to just adding an example

@fchapoton
Copy link
Contributor

Changed commit from 8719364 to 68fea61

@vbraun
Copy link
Member

vbraun commented May 19, 2017

Changed branch from public/20084 to 68fea61

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

5 participants