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

Bug in derivatives of some functions to the index var #21644

Closed
rwst opened this issue Oct 5, 2016 · 17 comments
Closed

Bug in derivatives of some functions to the index var #21644

rwst opened this issue Oct 5, 2016 · 17 comments

Comments

@rwst
Copy link

rwst commented Oct 5, 2016

sage: gen_laguerre(n,a,x).diff(a)
-gen_laguerre(n - 1, a + 1, x)
sage: gen_laguerre(n,a,x).diff(x)
-gen_laguerre(n - 1, a + 1, x)

sage: derivative(hermite(n,x),x)
2*n*hermite(n - 1, x)
sage: derivative(hermite(n,x),n)
2*n*hermite(n - 1, x)

so something is badly wrong with recognizing the diff. parameter. Note only hermite is implemented in Pynac.

Component: symbolics

Author: Carlos R. Mafra

Branch/Commit: 2decb06

Reviewer: Ralf Stephan

Issue created by migration from https://trac.sagemath.org/ticket/21644

@rwst rwst added this to the sage-7.4 milestone Oct 5, 2016
@rwst

This comment has been minimized.

@sagetrac-mafra
Copy link
Mannequin

sagetrac-mafra mannequin commented Oct 5, 2016

comment:2

I have a patch for this issue (at least for gen_laguerre, my copy of sage doesn't have a derivative method for hermite).

The bug was in handling the diff_param choice, the 'else' branch had the return value for when diff_param was x, but that means that anything that didn't match
the ifs before would return the derivative wrt x from the 'else' branch.

The attached patch fixes this (and adds proper handling to laguerre as well)

@sagetrac-mafra
Copy link
Mannequin

sagetrac-mafra mannequin commented Oct 5, 2016

Patch to fix issue with gen_laguerre derivative

@tscrim
Copy link
Collaborator

tscrim commented Oct 5, 2016

comment:3

Attachment: 0001-Fix-derivative-of-gen_laguerre.patch.gz

@sagetrac-mafra you should create and post a git branch of your patch to this ticket. Then one that is done, you can put your (real) name as the author and set it to needs review.

@sagetrac-mafra
Copy link
Mannequin

sagetrac-mafra mannequin commented Oct 5, 2016

@sagetrac-mafra
Copy link
Mannequin

sagetrac-mafra mannequin commented Oct 5, 2016

Author: Carlos R. Mafra

@sagetrac-mafra
Copy link
Mannequin

sagetrac-mafra mannequin commented Oct 5, 2016

New commits:

e6311e0Fix derivative of gen_laguerre wrt non-implemented variables

@sagetrac-mafra
Copy link
Mannequin

sagetrac-mafra mannequin commented Oct 5, 2016

Commit: e6311e0

@sagetrac-mafra sagetrac-mafra mannequin added the s: needs review label Oct 5, 2016
@rwst
Copy link
Author

rwst commented Oct 6, 2016

comment:6

That is fine, thanks. Can you please add a doctest for the fixed behaviour? We add tests for every branch of possible computation.

BTW Pynac fixes also get Python doctests, partly in the https://github.com/pynac/sage branches (which get merged with the Pynac upgrade ticket into Sage), and partly with separate Sage tickets.

@rwst
Copy link
Author

rwst commented Oct 6, 2016

comment:7

The hermite fix is in pynac/pynac@20222d0

@rwst
Copy link
Author

rwst commented Oct 6, 2016

comment:8

However, see #21655 for a later enhancement.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 6, 2016

Changed commit from e6311e0 to 2decb06

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 6, 2016

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

5b1b51bFix derivative of gen_laguerre wrt non-implemented variables
24b25a4Add doctest to trac 21644
2decb06Merge branch 'u/mafra/bug_in_derivatives_of_some_functions_to_the_index_var' of git://trac.sagemath.org/sage into t/21644/bug_in_derivatives_of_some_functions_to_the_index_var

@sagetrac-mafra
Copy link
Mannequin

sagetrac-mafra mannequin commented Oct 6, 2016

comment:10

Replying to @rwst:

That is fine, thanks. Can you please add a doctest for the fixed behaviour? We add tests for every branch of possible computation.

Ok, I did that now. But I was not sure if I should have rebased the new patch into the previous one or not, and when I decided to push I got an error message saying that I needed to 'pull'
first. After pulling an automatic merge was done, so now there are three commits in this branch.

Should I have rebased the doctest patch into the previous one? Would that require a 'forced' git push?

@rwst
Copy link
Author

rwst commented Oct 6, 2016

comment:11

The merge commit would not have been necessary if you had done git co t/21644/bug_in_derivatives_of_some_functions_to_the_index_var on your machine, added the doctest commt, and finally git trac push. But it's no heavy disaster.

@rwst
Copy link
Author

rwst commented Oct 6, 2016

Reviewer: Ralf Stephan

@vbraun
Copy link
Member

vbraun commented Oct 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants