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:  sage7.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: 
Description (last modified by )
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
Cc:  Clemens Heuberger Benjamin Hackl added 

comment:2 Changed 7 years ago by
Branch:  → u/dkrenn/asy/exploginv_taylor 

comment:3 Changed 7 years ago by
Authors:  → Daniel Krenn 

Commit:  → 84568cf4a0f89cbf3bfe22e6fd2ed279c08f4831 
Description:  modified (diff) 
Status:  new → needs_review 
comment:4 Changed 7 years ago by
Commit:  84568cf4a0f89cbf3bfe22e6fd2ed279c08f4831 → 68378745a4e792e5c58bdce9bb351debb7b239f9 

Branch pushed to git repo; I updated commit sha1. New commits:
6837874  Trac #19423: fix ReST doc

comment:5 followup: 8 Changed 7 years ago by
Milestone:  sage6.10 → sage7.1 

Reviewers:  → Clemens Heuberger 
Status:  needs_review → needs_work 
 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 with1  geom
, but I'd prefer to read standard taylor coefficients. _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
 I am not convinced of the name of the method.
 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
Branch:  u/dkrenn/asy/exploginv_taylor → u/cheuberg/asy/exploginv_taylor 

comment:7 Changed 7 years ago by
Commit:  68378745a4e792e5c58bdce9bb351debb7b239f9 → 1a526b73630b5ac24ffcbe0d604b44cc067f877e 

Branch pushed to git repo; I updated commit sha1. New commits:
1a526b7  Trac #19423: Merge #19946 to fix merge conflict

comment:8 followup: 10 Changed 7 years ago by
Replying to cheuberg:
 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.
_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.
 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
Commit:  1a526b73630b5ac24ffcbe0d604b44cc067f877e → c56a0958527786854a93fabac586107fedc5af1a 

comment:10 followup: 12 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:11 Changed 7 years ago by
Branch:  u/cheuberg/asy/exploginv_taylor → u/dkrenn/asy/exploginv_taylor 

comment:12 Changed 7 years ago by
Authors:  Daniel Krenn → Clemens Heuberger, Daniel Krenn 

Commit:  c56a0958527786854a93fabac586107fedc5af1a → 96f1ea6c9e8ec4d195635a95cb055eff6a7cc4f2 
Reviewers:  Clemens Heuberger → Clemens Heuberger, Daniel Krenn 
Replying to cheuberg:
From my side, that's it. Please (cross)review my changes.
Crossreviewed and fine from my side, but I've put a punch of small commits on top. Please crossreview them.
New commits:
1b44d48  Trac #19423: change ZeroDivisionError message

ecbeb26  Trac #19423: rephrase OUTPUTblock of _main_term_relative_error_

f21ea78  Trac #19423: new parameter to return inverse of main term in _main_term_relative_error_

7974666  Trac #19423: use new parameter of previous commit

b091009  Trac #19423: minor code rewrite to improve readability

96f1ea6  Trac #19423: correct parent in __invert__

comment:13 Changed 7 years ago by
Status:  needs_review → positive_review 

comment:14 Changed 7 years ago by
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
Branch:  u/dkrenn/asy/exploginv_taylor → 96f1ea6c9e8ec4d195635a95cb055eff6a7cc4f2 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Trac #19423: helpermethod for computing taylor series
Trac #19423: use new _taylor_ method in existing methods
Trac #19423: two code simplifications