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

Singularity analysis: fix and speed up singularity analysis (log-type) without renormalization #20051

Closed
cheuberg opened this issue Feb 14, 2016 · 20 comments

Comments

@cheuberg
Copy link
Contributor

#20020 introduced asymptotic_expansions._SingularityAnalysis_non_normalized_.
For zeta != 1, its results are incorrect due to substitution:

sage: asymptotic_expansions._SingularityAnalysis_non_normalized_(
....:     'n', 1/2, alpha=0, beta=1, precision=3)
1/2*2^n*n^(-1) + O(2^n*n^(-2))

instead of the correct 2^n * n^(-1) + O(2^n * n^(-2)).

Furthermore, its performance is poor as it relies on substitution. Speed up is possible by introducing a flag to asymptotic_expansions.SingularityAnalysis to ignore the normalization factor.

See #17601 for the asymptotic expansions meta ticket.

Depends on #20020
Depends on #20056
Depends on #20049

CC: @dkrenn @behackl

Component: asymptotic expansions

Keywords: singularity analysis

Author: Clemens Heuberger

Branch/Commit: e95cd2e

Reviewer: Benjamin Hackl

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

@cheuberg

This comment has been minimized.

@cheuberg
Copy link
Contributor Author

Author: Clemens Heuberger

@cheuberg cheuberg changed the title Singularity analysis: speed up singularity analysis (log-type) without renormalization Singularity analysis: fix and speed up singularity analysis (log-type) without renormalization Feb 17, 2016
@cheuberg
Copy link
Contributor Author

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 17, 2016

Commit: 4f32094

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 17, 2016

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

4f32094Trac #20051: rename 'renormalize' to 'normalized'

@cheuberg
Copy link
Contributor Author

Changed dependencies from #20020 to #20020, #20056

@behackl
Copy link
Member

behackl commented Feb 17, 2016

comment:6

Hello! I've reviewed your changes, they look good to me in general. I only have some minor comments:

  • I'm not sure whether so much math should occur in the INPUT-block in the description of the normalized-parameter. What about writing something like "determines, whether the normalization factor z^-(\beta + \delta) is taken into account"? The detailed description could either go into a NOTE-block or just be in the extended method description (before INPUT).
  • I see that you introduced a doctest which checks for the (currently) erroneous output in b336046. Was there no such doctest before, or have I missed something?

@cheuberg
Copy link
Contributor Author

comment:7

Replying to @behackl:

Hello! I've reviewed your changes, they look good to me in general. I only have some minor comments:

  • I'm not sure whether so much math should occur in the INPUT-block in the description of the normalized-parameter. What about writing something like "determines, whether the normalization factor z^-(\beta + \delta) is taken into account"? The detailed description could either go into a NOTE-block or just be in the extended method description (before INPUT).

I do not like a description like "whether the normalization factor ... is taken into account" because this is not a very clear description. I prefer to clearly state the fact, so that no ambiguities can arise.

If you want me to move this second variant to the top, say "the n-th coefficient of ... (if normalize=True, the default) or of ... (if normalize=False), I can do that.

  • I see that you introduced a doctest which checks for the (currently) erroneous output in b336046. Was there no such doctest before, or have I missed something?

There was one incorrect doctest in the growth group log(x)^2 where the incorrect result was stated (apparently, nobody checked whether the result there is correct). In any case, I wanted to have a doctest checking the correct behaviour right at the source of the problem.

@behackl
Copy link
Member

behackl commented Feb 18, 2016

comment:8

Replying to @cheuberg:

Replying to @behackl:

Hello! I've reviewed your changes, they look good to me in general. I only have some minor comments:

  • I'm not sure whether so much math should occur in the INPUT-block in the description of the normalized-parameter. What about writing something like "determines, whether the normalization factor z^-(\beta + \delta) is taken into account"? The detailed description could either go into a NOTE-block or just be in the extended method description (before INPUT).

I do not like a description like "whether the normalization factor ... is taken into account" because this is not a very clear description. I prefer to clearly state the fact, so that no ambiguities can arise.

I agree that unabiguity is more important than brevity in this case.

If you want me to move this second variant to the top, say "the n-th coefficient of ... (if normalize=True, the default) or of ... (if normalize=False), I can do that.

Yes, I think that mentioning the parameter in the extended function description too would be a good idea. Could you add such a statement?

  • I see that you introduced a doctest which checks for the (currently) erroneous output in b336046. Was there no such doctest before, or have I missed something?

There was one incorrect doctest in the growth group log(x)^2 where the incorrect result was stated (apparently, nobody checked whether the result there is correct). In any case, I wanted to have a doctest checking the correct behaviour right at the source of the problem.

I see, I've missed these lines when comparing diffs. The output of the doctest is now mathematically correct.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2016

Changed commit from 4f32094 to c80933d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2016

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

c80933dTrac #20051: move formula for normalized

@behackl
Copy link
Member

behackl commented Feb 18, 2016

comment:10

Thanks!

Documentation builds and looks fine, doctests pass. => positive_review.

@behackl
Copy link
Member

behackl commented Feb 18, 2016

Reviewer: Benjamin Hackl

@vbraun
Copy link
Member

vbraun commented Feb 18, 2016

comment:11

merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2016

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

d3b48c7Trac #20049: \zeta in GF in SingularityAnalysis-generator
e95cd2eTrac #20051: Merge #20049 and fix singularity

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2016

Changed commit from c80933d to e95cd2e

@cheuberg
Copy link
Contributor Author

Changed dependencies from #20020, #20056 to #20020, #20056, #20049

@cheuberg
Copy link
Contributor Author

comment:13

Replying to @vbraun:

merge conflict

sorry, fixed now.

@behackl
Copy link
Member

behackl commented Feb 18, 2016

comment:14

Changes LGTM, merging with all other of our closed tickets also works now.

@vbraun
Copy link
Member

vbraun commented Feb 18, 2016

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

3 participants