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.

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

Add TaylorSeriesRationalFunction and improve Derivative for univariate rational functions #4104

Merged
merged 2 commits into from
Sep 4, 2020

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Aug 20, 2020

Includes better method for derivative of rational function.

Taylor series coefficients may be useful for Molien series or Growth series

@hulpke hulpke added kind: new feature topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Aug 20, 2020
@coveralls
Copy link

coveralls commented Aug 21, 2020

Coverage Status

Coverage increased (+0.006%) to 85.76% when pulling 8519e20 on hulpke:additions into 32ea105 on gap-system:master.

@fingolfin fingolfin changed the title ENHANCE: Added Taylor series for rational functions Add TaylorSeriesRationalFunction and improve Derivative for univariate rational functions Sep 3, 2020
@fingolfin fingolfin added release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes and removed release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Sep 3, 2020
@fingolfin
Copy link
Member

This is a new documented feature, so it does need release notes. I've changed the PR title to a suitable candidate for the release notes entry.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK in principle but I have some suggestions. But it's also OK to merge as-is shrug

lib/ratfunul.gi Outdated Show resolved Hide resolved
lib/ratfunul.gi Outdated Show resolved Hide resolved
lib/ratfun.gd Outdated Show resolved Hide resolved
lib/ratfunul.gi Show resolved Hide resolved
Comment on lines +1128 to +1124
t:=Zero(f);
for i in [0..deg] do
if i>0 then
f:=Derivative(f);
fi;
t:=t+Value(f,at)*(x-at)^i/Factorial(i);
od;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we require deg >= 0 (which is not clear to me from the description of the function), could also do this

Suggested change
t:=Zero(f);
for i in [0..deg] do
if i>0 then
f:=Derivative(f);
fi;
t:=t+Value(f,at)*(x-at)^i/Factorial(i);
od;
t:=Value(f,at)*x^0;
for i in [1..deg] do
f:=Derivative(f);
t:=t+Value(f,at)*(x-at)^i/Factorial(i);
od;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a wash in terms of cost or code readability, so I left it out. (Added test for deg\ge0)

@hulpke hulpke merged commit 9c05b45 into gap-system:master Sep 4, 2020
@PaulaHaehndel PaulaHaehndel self-assigned this Feb 16, 2021
@PaulaHaehndel PaulaHaehndel added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 16, 2021
@PaulaHaehndel PaulaHaehndel removed their assignment Feb 16, 2021
@fingolfin fingolfin changed the title Add TaylorSeriesRationalFunction and improve Derivative for univariate rational functions Add TaylorSeriesRationalFunction and improve Derivative for univariate rational functions Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: new feature release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants