Opened 23 months ago
Closed 21 months ago
#28147 closed defect (fixed)
Remove _derivative from Polynomial_template
Reported by:  bruno  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  basic arithmetic  Keywords:  polynomial 
Cc:  Merged in:  
Authors:  Bruno Grenet  Reviewers:  Marc Mezzarobba 
Report Upstream:  N/A  Work issues:  
Branch:  9543c20 (Commits, GitHub, GitLab)  Commit:  9543c20b0992d55ae6ba99a8ace921025cf39ec7 
Dependencies:  Stopgaps: 
Description (last modified by )
Right now, the class Polynomial_template
contains a method _derivative
, called from the generic derivative
(from Polynomial
) through the function multi_derivative
of misc/derivative.pyx
. This method is quite badly written, with at least the following consequences:
 It is really slow:
sage: R.<x> = GF(65537)[] sage: p = R.random_element(10^4) sage: %time _ = p.derivative() CPU times: user 6.91 s, sys: 3.7 ms, total: 6.91 s Wall time: 6.91 s
 It breaks the differentiation of rational functions, cf #26844:
sage: A = PolynomialRing(GF(3), name='t') sage: K = A.fraction_field() sage: t = K.gen() sage: t.derivative(t) Traceback (most recent call last): ... ValueError: cannot differentiate with respect to t
After removal:
sage: R.<x> = GF(65537)[] sage: p = R.random_element(10^4) sage: %time _ = p.derivative() CPU times: user 48.5 ms, sys: 145 µs, total: 48.7 ms Wall time: 48.3 ms
sage: A = PolynomialRing(GF(3), name='t') sage: K = A.fraction_field() sage: t = K.gen() sage: t.derivative(t) 1
Change History (10)
comment:1 Changed 23 months ago by
 Branch set to u/bruno/remove_derivative
 Description modified (diff)
comment:2 Changed 23 months ago by
 Commit set to dcba4094c2910c1956413b1e310ca8465d2c7790
comment:3 Changed 23 months ago by
 Status changed from new to needs_review
I tried to detect potential problems with the removal of this method, but I didn't find any. Feel free to suggest where to look at!
comment:4 Changed 23 months ago by
 Description modified (diff)
comment:5 followup: ↓ 7 Changed 22 months ago by
Hi Bruno,
I think you may want to adapt some of the doctests you are removing. Otherwise lgtm...
comment:6 Changed 21 months ago by
 Commit changed from dcba4094c2910c1956413b1e310ca8465d2c7790 to 9543c20b0992d55ae6ba99a8ace921025cf39ec7
Branch pushed to git repo; I updated commit sha1. New commits:
9543c20  28147: Better error messages

comment:7 in reply to: ↑ 5 Changed 21 months ago by
Hi Marc,
I imported the removed doctests, and had errors... I had to change the error message when one tries to differentiate with respect to something "weird". In my view, this improves the current situation. In particular I do not find the following doctest (that I changed) very satisfying:
In the following example, it doesn't recognise 2\*x as the generator, so it tries to differentiate each of the coefficients with respect to 2\*x, which doesn't work because the integer coefficients don't have a _derivative() method:: sage: f._derivative(2*x) Traceback (most recent call last): ... AttributeError: 'sage.rings.integer.Integer' object has no attribute '_derivative'
The new doctest seems more reasonable to me:
It is not possible to differentiate with respect to 2\*x for instance:: sage: f._derivative(2*x) Traceback (most recent call last): ... ValueError: cannot differentiate with respect to 2*x
comment:8 Changed 21 months ago by
 Reviewers set to Marc Mezzarobba
 Status changed from needs_review to positive_review
Thank you!
comment:9 Changed 21 months ago by
Thank you for the review. Could you have a quick look to #26844 to confirm that the bug is indeed fixed by the current ticket?
comment:10 Changed 21 months ago by
 Branch changed from u/bruno/remove_derivative to 9543c20b0992d55ae6ba99a8ace921025cf39ec7
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
28147: Remove _derivative
28147: Doctests