Opened 4 years ago

Closed 4 years ago

#20051 closed defect (fixed)

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

Reported by: cheuberg Owned by:
Priority: major Milestone: sage-7.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 cheuberg)

#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 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 4 years ago by cheuberg

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

comment:3 Changed 4 years ago by git

  • Commit set to 4f32094ab2f8178738eca5049580e82dcd0d1900

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

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

comment:4 Changed 4 years ago by cheuberg

  • Status changed from new to needs_review

comment:5 Changed 4 years ago by cheuberg

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

comment:6 follow-up: Changed 4 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: Changed 4 years ago by 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.

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 4 years ago by behackl

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.

comment:9 Changed 4 years ago by git

  • Commit changed from 4f32094ab2f8178738eca5049580e82dcd0d1900 to c80933da15af6a60fa2b1e6cef7f92f755dded01

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

c80933dTrac #20051: move formula for normalized

comment:10 Changed 4 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: Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

merge conflict

comment:12 Changed 4 years ago by git

  • Commit changed from c80933da15af6a60fa2b1e6cef7f92f755dded01 to e95cd2e98255ac3acdc8437c109ada3ead9467c5

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

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

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

Replying to vbraun:

merge conflict

sorry, fixed now.

comment:14 Changed 4 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.

comment:15 Changed 4 years ago by vbraun

  • Branch changed from u/cheuberg/asy/improve-singularity-analysis-log-not-normalized to e95cd2e98255ac3acdc8437c109ada3ead9467c5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.