Opened 5 years ago

Closed 5 years ago

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

Reported by: Owned by: cheuberg major sage-7.1 asymptotic expansions singularity analysis dkrenn, behackl Clemens Heuberger Benjamin Hackl N/A e95cd2e (Commits) e95cd2e98255ac3acdc8437c109ada3ead9467c5 #20020, #20056, #20049

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

### comment:1 Changed 5 years ago by cheuberg

• Authors set to Clemens Heuberger
• Description modified (diff)
• Priority changed from minor to major
• Summary changed from Singularity analysis: speed up singularity analysis (log-type) without renormalization to Singularity analysis: fix and speed up singularity analysis (log-type) without renormalization
• Type changed from enhancement to defect

### comment:2 Changed 5 years ago by cheuberg

• Branch set to u/cheuberg/asy/improve-singularity-analysis-log-not-normalized

### comment:3 Changed 5 years ago by git

• Commit set to 4f32094ab2f8178738eca5049580e82dcd0d1900

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

 ​4f32094 Trac #20051: rename 'renormalize' to 'normalized'

### comment:4 Changed 5 years ago by cheuberg

• Status changed from new to needs_review

### comment:5 Changed 5 years ago by cheuberg

• Dependencies changed from #20020 to #20020, #20056

### comment:6 follow-up: ↓ 7 Changed 5 years ago by 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 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?

### comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 5 years ago by cheuberg

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.

### comment:8 in reply to: ↑ 7 Changed 5 years ago by 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.

### comment:9 Changed 5 years ago by git

• Commit changed from 4f32094ab2f8178738eca5049580e82dcd0d1900 to c80933da15af6a60fa2b1e6cef7f92f755dded01

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

 ​c80933d Trac #20051: move formula for normalized

### comment:10 Changed 5 years ago by behackl

• Reviewers set to Benjamin Hackl
• Status changed from needs_review to positive_review

Thanks!

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

### comment:11 follow-up: ↓ 13 Changed 5 years ago by vbraun

• Status changed from positive_review to needs_work

merge conflict

### comment:12 Changed 5 years ago by git

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

 ​d3b48c7 Trac #20049: \zeta in GF in SingularityAnalysis-generator ​e95cd2e Trac #20051: Merge #20049 and fix singularity

### comment:13 in reply to: ↑ 11 Changed 5 years ago by cheuberg

• Dependencies changed from #20020, #20056 to #20020, #20056, #20049
• Status changed from needs_work to needs_review

merge conflict

sorry, fixed now.

### comment:14 Changed 5 years ago by behackl

• Status changed from needs_review to positive_review

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