Opened 7 years ago

Closed 7 years ago

#19423 closed enhancement (fixed)

AsymptoticExpansion: combine shared code of invert, log, exp

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

Status badges

Description (last modified by Daniel Krenn)

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 7 years ago by Clemens Heuberger

Cc: Clemens Heuberger Benjamin Hackl added

comment:2 Changed 7 years ago by Daniel Krenn

Branch: u/dkrenn/asy/exploginv_taylor

comment:3 Changed 7 years ago by Daniel Krenn

Authors: Daniel Krenn
Commit: 84568cf4a0f89cbf3bfe22e6fd2ed279c08f4831
Description: modified (diff)
Status: newneeds_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 7 years ago by git

Commit: 84568cf4a0f89cbf3bfe22e6fd2ed279c08f483168378745a4e792e5c58bdce9bb351debb7b239f9

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

6837874Trac #19423: fix ReST doc

comment:5 Changed 7 years ago by Clemens Heuberger

Milestone: sage-6.10sage-7.1
Reviewers: Clemens Heuberger
Status: needs_reviewneeds_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 7 years ago by Clemens Heuberger

Branch: u/dkrenn/asy/exploginv_tayloru/cheuberg/asy/exploginv_taylor

comment:7 Changed 7 years ago by git

Commit: 68378745a4e792e5c58bdce9bb351debb7b239f91a526b73630b5ac24ffcbe0d604b44cc067f877e

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 ; Changed 7 years ago by Clemens Heuberger

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 7 years ago by git

Commit: 1a526b73630b5ac24ffcbe0d604b44cc067f877ec56a0958527786854a93fabac586107fedc5af1a

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 ; Changed 7 years ago by Clemens Heuberger

Status: needs_workneeds_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 7 years ago by Daniel Krenn

Branch: u/cheuberg/asy/exploginv_tayloru/dkrenn/asy/exploginv_taylor

comment:12 in reply to:  10 Changed 7 years ago by Daniel Krenn

Authors: Daniel KrennClemens Heuberger, Daniel Krenn
Commit: c56a0958527786854a93fabac586107fedc5af1a96f1ea6c9e8ec4d195635a95cb055eff6a7cc4f2
Reviewers: Clemens HeubergerClemens 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 7 years ago by Dima Pasechnik

Status: needs_reviewpositive_review

comment:14 Changed 7 years ago by Clemens Heuberger

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 7 years ago by Volker Braun

Branch: u/dkrenn/asy/exploginv_taylor96f1ea6c9e8ec4d195635a95cb055eff6a7cc4f2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.