Opened 4 years ago

Closed 4 years ago

#19423 closed enhancement (fixed)

AsymptoticExpansion: combine shared code of invert, log, exp

Reported by: dkrenn Owned by:
Priority: major Milestone: sage-7.1
Component: asymptotic expansions Keywords:
Cc: cheuberg, behackl Merged in:
Authors: Clemens Heuberger, Daniel Krenn Reviewers: Clemens Heuberger, Daniel Krenn
Report Upstream: N/A Work issues:
Branch: 96f1ea6 (Commits) Commit: 96f1ea6c9e8ec4d195635a95cb055eff6a7cc4f2
Dependencies: Stopgaps:

Description (last modified by dkrenn)

From #19083, comment 66, 33:

I think that there is shared code between __invert__, __log__, __exp__ and powers with rational exponents (#19316). In all those cases, it is important to split into main term and renormalized remainder. The main term is then processed according to the respective method, the remainder is inserted into a converging taylor series with certain coefficients (this could be handeled by one method getting the sequence of taylor coefficients as an argument).

Change History (15)

comment:1 Changed 4 years ago by cheuberg

  • Cc cheuberg behackl added

comment:2 Changed 4 years ago by dkrenn

  • Branch set to u/dkrenn/asy/exploginv_taylor

comment:3 Changed 4 years ago by dkrenn

  • Authors set to Daniel Krenn
  • Commit set to 84568cf4a0f89cbf3bfe22e6fd2ed279c08f4831
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

2e35be6Trac #19423: helper-method for computing taylor series
cd99cbcTrac #19423: use new _taylor_ method in existing methods
84568cfTrac #19423: two code simplifications

comment:4 Changed 4 years ago by git

  • Commit changed from 84568cf4a0f89cbf3bfe22e6fd2ed279c08f4831 to 68378745a4e792e5c58bdce9bb351debb7b239f9

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

6837874Trac #19423: fix ReST doc

comment:5 follow-up: Changed 4 years ago by cheuberg

  • Milestone changed from sage-6.10 to sage-7.1
  • Reviewers set to Clemens Heuberger
  • Status changed from needs_review to needs_work
  1. What I had in mind was more commonality between the three methods: Most of the methods deal with writing self = max_elem * (1 + geom). This part could be done by a common routine. (log and inversion a slightly more comfortable with 1 - geom, but I'd prefer to read standard taylor coefficients.
  2. _taylor_:
    • I am not convinced of the name of the method. _power_series_ ?
    • The role of precision is not explained.
    • There should be a doctest where a fixed point is reached before reaching precision
  3. logarithm: I think that it would be more readable to start with 0 and let the coefficients start at k=1.

comment:6 Changed 4 years ago by cheuberg

  • Branch changed from u/dkrenn/asy/exploginv_taylor to u/cheuberg/asy/exploginv_taylor

comment:7 Changed 4 years ago by git

  • Commit changed from 68378745a4e792e5c58bdce9bb351debb7b239f9 to 1a526b73630b5ac24ffcbe0d604b44cc067f877e

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

1a526b7Trac #19423: Merge #19946 to fix merge conflict

comment:8 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by cheuberg

Replying to cheuberg:

  1. What I had in mind was more commonality between the three methods: Most of the methods deal with writing self = max_elem * (1 + geom). This part could be done by a common routine.

done.

(log and inversion a slightly more comfortable with 1 - geom, but I'd prefer to read standard taylor coefficients.

I used -x and everything is fine.

  1. _taylor_:
    • I am not convinced of the name of the method. _power_series_ ?

done.

  • The role of precision is not explained.

still to do.

  • There should be a doctest where a fixed point is reached before reaching precision

still to do.

  1. logarithm: I think that it would be more readable to start with 0 and let the coefficients start at k=1.

I can live with the previous implementation for efficiency reasons.

comment:9 Changed 4 years ago by git

  • Commit changed from 1a526b73630b5ac24ffcbe0d604b44cc067f877e to c56a0958527786854a93fabac586107fedc5af1a

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

9a58142Trac #19423: document parameter precision
c56a095Trac #19423: doctests for precision

comment:10 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by cheuberg

  • Status changed from needs_work to needs_review

Replying to cheuberg:

Replying to cheuberg:

  • The role of precision is not explained.

done.

  • There should be a doctest where a fixed point is reached before reaching precision

done.

From my side, that's it. Please (cross-)review my changes.

comment:11 Changed 4 years ago by dkrenn

  • Branch changed from u/cheuberg/asy/exploginv_taylor to u/dkrenn/asy/exploginv_taylor

comment:12 in reply to: ↑ 10 Changed 4 years ago by dkrenn

  • Authors changed from Daniel Krenn to Clemens Heuberger, Daniel Krenn
  • Commit changed from c56a0958527786854a93fabac586107fedc5af1a to 96f1ea6c9e8ec4d195635a95cb055eff6a7cc4f2
  • Reviewers changed from Clemens Heuberger to Clemens Heuberger, Daniel Krenn

Replying to cheuberg:

From my side, that's it. Please (cross-)review my changes.

Cross-reviewed and fine from my side, but I've put a punch of small commits on top. Please cross-review them.


New commits:

1b44d48Trac #19423: change ZeroDivisionError message
ecbeb26Trac #19423: rephrase OUTPUT-block of _main_term_relative_error_
f21ea78Trac #19423: new parameter to return inverse of main term in _main_term_relative_error_
7974666Trac #19423: use new parameter of previous commit
b091009Trac #19423: minor code rewrite to improve readability
96f1ea6Trac #19423: correct parent in __invert__

comment:13 Changed 4 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:14 Changed 4 years ago by cheuberg

Dima, thanks for setting this to positive on my behalf.

Patchbot "findstat" shows errors which should be in no way related to this ticket.

comment:15 Changed 4 years ago by vbraun

  • Branch changed from u/dkrenn/asy/exploginv_taylor to 96f1ea6c9e8ec4d195635a95cb055eff6a7cc4f2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.