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:  sage7.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 )
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
 Cc cheuberg behackl added
comment:2 Changed 4 years ago by
 Branch set to u/dkrenn/asy/exploginv_taylor
comment:3 Changed 4 years ago by
 Commit set to 84568cf4a0f89cbf3bfe22e6fd2ed279c08f4831
 Description modified (diff)
 Status changed from new to needs_review
comment:4 Changed 4 years ago by
 Commit changed from 84568cf4a0f89cbf3bfe22e6fd2ed279c08f4831 to 68378745a4e792e5c58bdce9bb351debb7b239f9
Branch pushed to git repo; I updated commit sha1. New commits:
6837874  Trac #19423: fix ReST doc

comment:5 followup: ↓ 8 Changed 4 years ago by
 Milestone changed from sage6.10 to sage7.1
 Reviewers set to Clemens Heuberger
 Status changed from needs_review to 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 4 years ago by
 Branch changed from u/dkrenn/asy/exploginv_taylor to u/cheuberg/asy/exploginv_taylor
comment:7 Changed 4 years ago by
 Commit changed from 68378745a4e792e5c58bdce9bb351debb7b239f9 to 1a526b73630b5ac24ffcbe0d604b44cc067f877e
Branch pushed to git repo; I updated commit sha1. New commits:
1a526b7  Trac #19423: Merge #19946 to fix merge conflict

comment:8 in reply to: ↑ 5 ; followup: ↓ 10 Changed 4 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 4 years ago by
 Commit changed from 1a526b73630b5ac24ffcbe0d604b44cc067f877e to c56a0958527786854a93fabac586107fedc5af1a
comment:10 in reply to: ↑ 8 ; followup: ↓ 12 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:11 Changed 4 years ago by
 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
 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.
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 4 years ago by
 Status changed from needs_review to positive_review
comment:14 Changed 4 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 4 years ago by
 Branch changed from u/dkrenn/asy/exploginv_taylor to 96f1ea6c9e8ec4d195635a95cb055eff6a7cc4f2
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac #19423: helpermethod for computing taylor series
Trac #19423: use new _taylor_ method in existing methods
Trac #19423: two code simplifications