Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AsymptoticExpansion: combine shared code of invert, log, exp #19423

Closed
dkrenn opened this issue Oct 16, 2015 · 24 comments
Closed

AsymptoticExpansion: combine shared code of invert, log, exp #19423

dkrenn opened this issue Oct 16, 2015 · 24 comments
Labels
c: asymptotic expansions p: major / 3 t: enhancement
Milestone
sage-7.1

Comments

@dkrenn
Copy link
Contributor

dkrenn commented Oct 16, 2015

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).

CC: @cheuberg @behackl

Component: asymptotic expansions

Author: Clemens Heuberger, Daniel Krenn

Branch/Commit: 96f1ea6

Reviewer: Clemens Heuberger, Daniel Krenn

Issue created by migration from https://trac.sagemath.org/ticket/19423
@dkrenn dkrenn added this to the sage-6.10 milestone Oct 16, 2015
@dkrenn
Copy link
Contributor Author

dkrenn commented Jan 30, 2016

Branch: u/dkrenn/asy/exploginv_taylor

@dkrenn
Copy link
Contributor Author

dkrenn commented Jan 30, 2016

New commits:

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

@dkrenn
Copy link
Contributor Author

dkrenn commented Jan 30, 2016

Author: Daniel Krenn

@dkrenn

This comment has been minimized.

Sign in to view
@dkrenn
Copy link
Contributor Author

dkrenn commented Jan 30, 2016

Commit: 84568cf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 30, 2016

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

6837874Trac #19423: fix ReST doc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 30, 2016

Changed commit from 84568cf to 6837874

@cheuberg
Copy link
Contributor

Reviewer: Clemens Heuberger

@cheuberg
Copy link
Contributor

comment:5
  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.

@cheuberg cheuberg modified the milestones: sage-6.10, sage-7.1 Jan 31, 2016
@cheuberg
Copy link
Contributor

cheuberg commented Feb 4, 2016

Changed branch from u/dkrenn/asy/exploginv_taylor to u/cheuberg/asy/exploginv_taylor

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 4, 2016

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 4, 2016

Changed commit from 6837874 to 1a526b7

@cheuberg
Copy link
Contributor

cheuberg commented Feb 4, 2016

comment:8

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 5, 2016

Changed commit from 1a526b7 to c56a095

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 5, 2016

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

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

@cheuberg
Copy link
Contributor

cheuberg commented Feb 5, 2016

comment:10

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.

@dkrenn
Copy link
Contributor Author

dkrenn commented Feb 8, 2016

Changed branch from u/cheuberg/asy/exploginv_taylor to u/dkrenn/asy/exploginv_taylor

@dkrenn
Copy link
Contributor Author

dkrenn commented Feb 8, 2016

Changed commit from c56a095 to 96f1ea6

@dkrenn
Copy link
Contributor Author

dkrenn commented Feb 8, 2016

Changed author from Daniel Krenn to Clemens Heuberger, Daniel Krenn

@dkrenn
Copy link
Contributor Author

dkrenn commented Feb 8, 2016

comment:12

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__

@dkrenn
Copy link
Contributor Author

dkrenn commented Feb 8, 2016

Changed reviewer from Clemens Heuberger to Clemens Heuberger, Daniel Krenn

@dimpase
Copy link
Member

dimpase commented Feb 8, 2016

comment:13

https://groups.google.com/d/msg/sage-devel/2aBJVdA5YoY/aOrL7ggZCgAJ

@cheuberg
Copy link
Contributor

cheuberg commented Feb 9, 2016

comment:14

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

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

@vbraun
Copy link
Member

vbraun commented Feb 9, 2016

Changed branch from u/dkrenn/asy/exploginv_taylor to 96f1ea6

@vbraun vbraun closed this as completed in da58a38 Feb 9, 2016
@behackl behackl mentioned this issue Dec 29, 2022
Open
@dkrenn dkrenn mentioned this issue Feb 10, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: asymptotic expansions p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-7.1
Development

No branches or pull requests
4 participants
@vbraun @dimpase @cheuberg @dkrenn