#28147 closed defect (fixed)

Remove _derivative from Polynomial_template

Reported by: bruno Owned by:
Priority: major Milestone: sage-8.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:

Status badges

Description (last modified by bruno)

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 bruno

  • Branch set to u/bruno/remove_derivative
  • Description modified (diff)

comment:2 Changed 23 months ago by git

  • Commit set to dcba4094c2910c1956413b1e310ca8465d2c7790

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

6384e2e28147: Remove _derivative
dcba40928147: Doctests

comment:3 Changed 23 months ago by bruno

  • 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 bruno

  • Description modified (diff)

comment:5 follow-up: Changed 22 months ago by mmezzarobba

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 git

  • Commit changed from dcba4094c2910c1956413b1e310ca8465d2c7790 to 9543c20b0992d55ae6ba99a8ace921025cf39ec7

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

9543c2028147: Better error messages

comment:7 in reply to: ↑ 5 Changed 21 months ago by bruno

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
Last edited 21 months ago by bruno (previous) (diff)

comment:8 Changed 21 months ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

Thank you!

comment:9 Changed 21 months ago by bruno

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 vbraun

  • Branch changed from u/bruno/remove_derivative to 9543c20b0992d55ae6ba99a8ace921025cf39ec7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.