Opened 4 years ago
Closed 4 years ago
#20051 closed defect (fixed)
Singularity analysis: fix and speed up singularity analysis (logtype) without renormalization
Reported by:  cheuberg  Owned by:  

Priority:  major  Milestone:  sage7.1 
Component:  asymptotic expansions  Keywords:  singularity analysis 
Cc:  dkrenn, behackl  Merged in:  
Authors:  Clemens Heuberger  Reviewers:  Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  e95cd2e (Commits)  Commit:  e95cd2e98255ac3acdc8437c109ada3ead9467c5 
Dependencies:  #20020, #20056, #20049  Stopgaps: 
Description (last modified by )
#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.
Change History (15)
comment:1 Changed 4 years ago by
 Description modified (diff)
 Priority changed from minor to major
 Summary changed from Singularity analysis: speed up singularity analysis (logtype) without renormalization to Singularity analysis: fix and speed up singularity analysis (logtype) without renormalization
 Type changed from enhancement to defect
comment:2 Changed 4 years ago by
 Branch set to u/cheuberg/asy/improvesingularityanalysislognotnormalized
comment:3 Changed 4 years ago by
 Commit set to 4f32094ab2f8178738eca5049580e82dcd0d1900
comment:4 Changed 4 years ago by
 Status changed from new to needs_review
comment:5 Changed 4 years ago by
 Dependencies changed from #20020 to #20020, #20056
comment:6 followup: ↓ 7 Changed 4 years ago by
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 thenormalized
parameter. What about writing something like "determines, whether the normalization factorz^(\beta + \delta)
is taken into account"? The detailed description could either go into aNOTE
block or just be in the extended method description (beforeINPUT
).  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 ; followup: ↓ 8 Changed 4 years ago by
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 thenormalized
parameter. What about writing something like "determines, whether the normalization factorz^(\beta + \delta)
is taken into account"? The detailed description could either go into aNOTE
block or just be in the extended method description (beforeINPUT
).
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 nth 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 4 years ago by
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 thenormalized
parameter. What about writing something like "determines, whether the normalization factorz^(\beta + \delta)
is taken into account"? The detailed description could either go into aNOTE
block or just be in the extended method description (beforeINPUT
).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 nth coefficient of ... (if
normalize=True
, the default) or of ... (ifnormalize=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 4 years ago by
 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 4 years ago by
 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 followup: ↓ 13 Changed 4 years ago by
 Status changed from positive_review to needs_work
merge conflict
comment:12 Changed 4 years ago by
 Commit changed from c80933da15af6a60fa2b1e6cef7f92f755dded01 to e95cd2e98255ac3acdc8437c109ada3ead9467c5
comment:13 in reply to: ↑ 11 Changed 4 years ago by
 Dependencies changed from #20020, #20056 to #20020, #20056, #20049
 Status changed from needs_work to needs_review
comment:14 Changed 4 years ago by
 Status changed from needs_review to positive_review
Changes LGTM, merging with all other of our closed tickets also works now.
comment:15 Changed 4 years ago by
 Branch changed from u/cheuberg/asy/improvesingularityanalysislognotnormalized to e95cd2e98255ac3acdc8437c109ada3ead9467c5
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Trac #20051: rename 'renormalize' to 'normalized'