sagemath / sage

Main repository of SageMath. Now open for Issues and Pull Requests.
https://www.sagemath.org
Other
1.19k stars 412 forks source link

derivative of integer wrt to variable in polynomial ring should belong to that ring, not symbolic ring #20812

Open d57983b9-eb5e-4338-b5ae-e1c8061064af opened 8 years ago

d57983b9-eb5e-4338-b5ae-e1c8061064af commented 8 years ago

If I try to take the derivative of an integer (or nonzero rational, or integer mod n), then the result is an element of the symbolic ring:

sage: R.<x>=ZZ[]
sage: derivative(0,x).parent()
Symbolic Ring

It seems like it would be more natural for the returned value to belong to the ring containing x instead.

This may seem kind of pedantic, but it did trip me up when I was working with a list of polynomials, some of which were constant, and things were getting cast to Expression unexpectedly.

I am not particularly familiar with the Sage codebase, but I am attaching a patch that seems to fix the issue.

CC: @orlitzky

Component: calculus

Author: Daniel Gulotta, Vincent Delecroix

Branch/Commit: u/vdelecroix/20812 @ 04dbe4c

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

d57983b9-eb5e-4338-b5ae-e1c8061064af commented 8 years ago

Attachment: functional.py.patch.gz

fchapoton commented 7 years ago
comment:1

If you take care to use the correct zero, this just works:

sage: R.zero().derivative(x).parent()
Univariate Polynomial Ring in x over Integer Ring

But there is room for improvement, for sure.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 3 years ago
comment:2

The derivative of a symbolic expression is a symbolic expression :

sage: R1.<t>=ZZ[]
sage: derivative(x^2+x+1,t).parent()
Symbolic Ring

Therefore this :

sage: derivative(0,t).parent()
Symbolic Ring

is a special case, indicating that 0 is cast to a symbolic expression (probably by diff...).

However :

sage: derivative(R1(0),t).parent()
Univariate Polynomial Ring in t over Integer Ring

conforms to the requirement that the derivative of an object belongs its parent ring.

Pedantism works both ways...

==> marking as invalid and requesting review in order to close.

d57983b9-eb5e-4338-b5ae-e1c8061064af commented 3 years ago
comment:3

This behavior is confusing. I think it's reasonable to expect that the derivative(f,g) will lie in the smallest ring containing f and g. Why not fix this?

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 3 years ago
comment:4

Replying to @sagetrac-dgulotta:

This behavior is confusing. I think it's reasonable to expect that the derivative(f,g) will lie in the smallest ring containing f and g. Why not fix this?

In order

...even when this function, expression or polynomial happens to be a "constant" or degree-0 monomial, in which case the derivative can be taken to be the null "constant" or degree-0 monomial.

Your remark may be more relevant in the reverse case:

sage: R1.<t>=QQbar[]
sage: foo=t^2
sage: integral(foo,t).parent()
Univariate Polynomial Ring in t over Algebraic Field

So far, so good. But

sage: integral(0,t).parent()
Symbolic Ring

is nonsensical, unless we mean to do implicitly :

sage: integral(R1(0),t).parent()
Univariate Polynomial Ring in t over Algebraic Field

In other words, take note that

sage: R1(0) is 0
False

even if

sage: R1(0).is_zero()
True

HTH,

d57983b9-eb5e-4338-b5ae-e1c8061064af commented 3 years ago
comment:5

It is difficult to do the right thing in all cases but I think that the patch that I submitted improves the situation for derivatives. I could write something similar for integrals if there is agreement that this would be useful.

The reason why I don't like casting things into the symbolic ring is that it leads to errors that are very difficult to track down. For example:

sage: R.<x>=QQ[]
sage: l = [1,x,x*(x-1),x*(x-1)*(x-2)]
sage: [derivative(f,x).monomial_coefficient(x) for f in l]
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-16-ff4d7a390725> in <module>
----> 1 [derivative(f,x).monomial_coefficient(x) for f in l]

<ipython-input-16-ff4d7a390725> in <listcomp>(.0)
----> 1 [derivative(f,x).monomial_coefficient(x) for f in l]

/ext/sage/sage-9.2/local/lib/python3.8/site-packages/sage/structure/element.pyx in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4703)()
    491             AttributeError: 'LeftZeroSemigroup_with_category.element_class' object has no attribute 'blah_blah'
    492         """
--> 493         return self.getattr_from_category(name)
    494 
    495     cdef getattr_from_category(self, name):
/ext/sage/sage-9.2/local/lib/python3.8/site-packages/sage/structure/element.pyx in sage.structure.element.Element.getattr_from_category (build/cythonized/sage/structure/element.c:4815)()
    504         else:
    505             cls = P._abstract_element_class
--> 506         return getattr_from_other_class(self, cls, name)
    507 
    508     def __dir__(self):
/ext/sage/sage-9.2/local/lib/python3.8/site-packages/sage/cpython/getattr.pyx in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:2620)()
    370         dummy_error_message.cls = type(self)
    371         dummy_error_message.name = name
--> 372         raise AttributeError(dummy_error_message)
    373     attribute = <object>attr
    374     # Check for a descriptor (__get__ in Python)
AttributeError: 'sage.symbolic.expression.Expression' object has no attribute 'monomial_coefficient'

It is not at all clear from the error message that 1 needs to be replaced with R(1). And this is just a toy example; in real code the failure could occur much later down the line. So I think it is bad for a function like derivative that sometimes returns polynomials to implicitly cast things into the symbolic ring when none of the arguments live in the symbolic ring. The patch that I submitted should significantly reduce these types of errors.

In principle I think it is better to raise an error with a detailed message than to implicitly cast into the symbolic ring. I guess it may be too late to make that change since it might break existing code. Attempting to cast into a polynomial ring when possible seems like a reasonable compromise.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 3 years ago
comment:6

Replying to @sagetrac-dgulotta:

[ Snip... ]

The patch that I submitted should significantly reduce these types of errors.

Which patch ? I find no patch in the ticket.

More generally, I think that the problem is to define and compute "the smallest ring containing f and g".

To illustrate :

sage: R1.<t>=QQ[]
sage: t.derivative(t)
1
sage: t.derivative(t).parent()
Univariate Polynomial Ring in t over Rational Field
sage: 1.derivative(t).parent()
## [ Snip... ]
AttributeError: 'sage.rings.integer.Integer' object has no attribute 'derivative'
sage: t.parent()(1).derivative(t).parent()
Univariate Polynomial Ring in t over Rational Field

This cast is reasonable and might be expected (i. e. one can reasonably expect 1.differential(t) to return R1's 0.

Harder :

sage: R2.<u>=QQ[]
sage: t.derivative(u)
## [ Snip... ]
ValueError: cannot differentiate with respect to u

One might expect the 0 with :

I'm not sure that this can be expressed in Sage...

For the integrals :

sage: 1.integral(t)
## [ Snip]
AttributeError: 'sage.rings.integer.Integer' object has no attribute 'integral'
sage: t.parent()(1).integral(t)
t

Again, a "reasonable" cast.

The case t.integral(u) leads to the same conclusion as for t.differentiate(u).

And we might have worse difficulties : what should be the "smallest ring" containing Zmod(3)['v'] and QQbar['w'] ? Ditto for ring of matrices...

Casting to SR is, indeed, far from ideal, but it seems tome that the possible enhancements are fraught with more difficulties than they solve.

Your thoughts ?

d57983b9-eb5e-4338-b5ae-e1c8061064af commented 3 years ago
comment:7

There is an attachment to this ticket, which is a patch. The patch uses sage.structure.element.get_coercion_model. I don't claim to be an expert on this function but it seems to do the right thing in cases that would come up in practice.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 3 years ago
comment:8

Replying to @sagetrac-dgulotta:

There is an attachment to this ticket, which is a patch. The patch uses sage.structure.element.get_coercion_model. I don't claim to be an expert on this function but it seems to do the right thing in cases that would come up in practice.

Would you mind submitting a branch, as described in Sagemath's developer's guide ?

d57983b9-eb5e-4338-b5ae-e1c8061064af commented 3 years ago

Branch: u/dgulotta/derivative_of_integer_wrt_to_variable_in_polynomial_ring_should_belong_to_that_ring__not_symbolic_ring

videlec commented 3 years ago

Commit: 0dc171f

videlec commented 3 years ago
comment:10

It is a bad idea to put a lot of code inside the try block. Only keep there the minimal amount of code that could potentially raise an error. For example neither elts.append(f) nor cm = get_coercion_model() should be there.


New commits:

0dc171fderivative: try to find a common parent before casting to SR
tscrim commented 3 years ago
comment:11

This might be an interesting data point:

sage: R.<x> = ZZ[]
sage: S.<y> = ZZ[]
sage: derivative(S.zero(), x).parent()
Univariate Polynomial Ring in y over Integer Ring
videlec commented 2 years ago
comment:12

Setting in needs works because of [comment:10].

videlec commented 2 years ago
comment:13

Also [comment:2] makes a good point derivative(QQ['t'].gen(), SR.var('t')) is currently a polynomial. The proposed branch would change that behaviour and it is not clear whether this is desirable.

A more sensible change for derivative(f, v) would be to do in order

  1. try to convert v to parent(f) and take derivative (that would keep the behaviour noticed in [comment:2])
  2. try to find a common parent for f and v and take derivative (that would solve the issue in the ticket description and arguably improve the behaviour from [comment:11])
  3. go to SR
videlec commented 2 years ago

Changed branch from u/dgulotta/derivative_of_integer_wrt_to_variable_in_polynomial_ring_should_belong_to_that_ring__not_symbolic_ring to u/vdelecroix/20812

videlec commented 2 years ago

New commits:

3fd97b4derivative: try to find a common parent before casting to SR
04dbe4ccleaner implementation, doctest and explanations
videlec commented 2 years ago

Changed commit from 0dc171f to 04dbe4c

videlec commented 2 years ago

Author: Daniel Gulotta, Vincent Delecroix

orlitzky commented 2 years ago
comment:17

There's a typo in,

This behaviour of the derivaive function

But personally, instead of an example that says

the parent of the result might seem confusing... this... is a consequence of how derivatives are implemented for polynomials

I would prefer if the documentation just told me what the parent will be in the OUTPUT block: the derivative of a symbolic expression with respect to a symbolic expression will be a symbolic expression, and likewise for polynomials with respect to polynomials. If the function and the variable are of different types that share a common parent, then the result will live in that common parent. Otherwise, as a last resort, it will live in the symbolic ring.

videlec commented 2 years ago
comment:18

Currently

sage: derivative(0)
0
sage: derivative(0).parent()
Symbolic Ring

I don't like it... though I am not sure what it should be.

orlitzky commented 2 years ago
comment:19

Replying to @videlec:

I don't like it... though I am not sure what it should be.

It should be an error, because the integer zero is not a function.

But I guess that behavior is consistent with the usual abuse of notation. We convert constants to symbolic expressions so that they can be evaluated like a function. In this case, we convert ZZ(0) to SR(0), and if you think of that as being the const-zero function, its derivative is itself with respect to whatever you call the implicit argument.

videlec commented 2 years ago
comment:20

Replying to @orlitzky:

Replying to @videlec:

I don't like it... though I am not sure what it should be.

It should be an error, because the integer zero is not a function.

If this is your answer, then you should arguethat the derivative of any element of Integer Ring should result in an error.

But I guess that behavior is consistent with the usual abuse of notation. We convert constants to symbolic expressions so that they can be evaluated like a function. In this case, we convert ZZ(0) to SR(0), and if you think of that as being the const-zero function, its derivative is itself with respect to whatever you call the implicit argument.

Precisely, this ticket is changing that behaviour as it also make sense to take derivatives of polynomials, Laurent polynomials, power series, etc. If you find this behaviour consistent, then this ticket should not be merged at all.

nbruin commented 2 years ago
comment:21

I suspect the behaviour on the integers was installed to deal with some edge cases that happened interactively or perhaps internally in SR.

As far as I can see, x.derivative(...) is meant as an application of an operator to a ring element x and the argument ... is taken as an indication what operator is meant. In particular, I don't think there is normally any effort to coerce the element x into a ring derived from the arguments ....

For instance:

sage: R.<a>=QQ[]
sage: S.<b>=QQ[]
sage: a.derivative(b)
ValueError: cannot differentiate with respect to b
sage: (a^3).derivative(2) #  2 means a repeat count here
6*a

In fact,

sage: 0.derivative(a)
AttributeError: 'sage.rings.integer.Integer' object has no attribute 'derivative'

so the functionality here is just part of the top-level derivative convenience function. Indeed, it's behaviour is: first try method application. If that fails, try if coercing into SR works. I think the current behaviour is as intended: convenience for the casual user who isn't bothered by SR. Anyone who is worried about parents should be using method calls here, since they dispatch much better among types.

These convenience functions are just not a good way of dispatching on type data. You'd have to move to a different language then, that has full signature dispatch rather than python's "first argument" dispatch through method calls.

orlitzky commented 2 years ago
comment:22

Replying to @videlec:

Replying to @orlitzky:

Replying to @videlec:

I don't like it... though I am not sure what it should be.

It should be an error, because the integer zero is not a function.

If this is your answer, then you should argue that the derivative of any element of Integer Ring should result in an error.

That's right.

Precisely, this ticket is changing that behaviour as it also make sense to take derivatives of polynomials, Laurent polynomials, power series, etc. If you find this behaviour consistent, then this ticket should not be merged at all.

These top-level convenience functions that take any sage or python object are always a mess. The principled approach is to use x.derivative(), and if that method isn't there, then you can't differentiate the thing. We only run into a problem trying to teach the top-level function how to differentiate things that can't be differentiated. In that regard, asking for consistency from the result when our goals weren't consistent to begin with is too optimistic.

So I think derivative(ZZ(0)) = SR(0) is as good as any answer for the derivative of an integer. But I also think that having special cases for polynomials and series is fine too. Since polynomials overlap partially (but not completely) with things that can be coerced into SR, I don't think we'll get a nice solution that is consistent in all cases.