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

Remove _derivative from Polynomial_template #28147

Closed
bgrenet opened this issue Jul 9, 2019 · 14 comments
Closed

Remove _derivative from Polynomial_template #28147

bgrenet opened this issue Jul 9, 2019 · 14 comments

Comments

@bgrenet
Copy link

bgrenet commented Jul 9, 2019

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

Component: basic arithmetic

Keywords: polynomial

Author: Bruno Grenet

Branch/Commit: 9543c20

Reviewer: Marc Mezzarobba

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

@bgrenet bgrenet added this to the sage-8.9 milestone Jul 9, 2019
@bgrenet

This comment has been minimized.

@bgrenet
Copy link
Author

bgrenet commented Jul 10, 2019

Branch: u/bruno/remove_derivative

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2019

Commit: dcba409

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2019

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

6384e2e28147: Remove _derivative
dcba40928147: Doctests

@bgrenet
Copy link
Author

bgrenet commented Jul 10, 2019

comment:3

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!

@bgrenet

This comment has been minimized.

@mezzarobba
Copy link
Member

comment:5

Hi Bruno,

I think you may want to adapt some of the doctests you are removing. Otherwise lgtm...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 19, 2019

Changed commit from dcba409 to 9543c20

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 19, 2019

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

9543c2028147: Better error messages

@bgrenet
Copy link
Author

bgrenet commented Aug 19, 2019

comment:7

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

@mezzarobba
Copy link
Member

comment:8

Thank you!

@mezzarobba
Copy link
Member

Reviewer: Marc Mezzarobba

@bgrenet
Copy link
Author

bgrenet commented Aug 21, 2019

comment:9

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?

@vbraun
Copy link
Member

vbraun commented Aug 29, 2019

Changed branch from u/bruno/remove_derivative to 9543c20

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