Deprecate the misuse of symbolic variables as polynomial variable

[simon-king-jena]

Relatively often on sage-support, users run into a problem since they mix symbolics and polynomials. Typically, they do

sage: z = var('z')
sage: P = QQ[z]

and expect that z is the generator of P - which is of course
wrong and will soon mean trouble.

I find even worse that one can do

sage: QQ[x,gap,singular]
Multivariate Polynomial Ring in x, Gap, Singular over Rational Field

since the variable names are obtained from the string representation of the given objects.

It should be clear that the preferred way of constructing a polynomial (or quotient) ring together with its generators is

sage: R.<x,y,z> = QQ[]

Hence, it is quite amazing that in the documentation one occasionally finds the "wrong" usage

sage: x,y,z = var('x y z')
sage: P = QQ[x,y,z]

I suggest to deprecate the possibility of providing the variable names by anything but strings (potentially plus an integer, like PolynomialRing(QQ,'x',5)).

With my patch, one has

sage: QQ[x,gap,singular]
/mnt/local/king/SAGE/sage-4.6/local/bin/sage-ipython:1: DeprecationWarning: (Since Sage Version 4.6.1) Variable name 'x' should be a string, but we obtained <type 'sage.symbolic.expression.Expression'>.
In an interactive session, you should use a definition of the form 'R.<x,y,z>=QQ[]'.
  #!/usr/bin/env python
/mnt/local/king/SAGE/sage-4.6/local/bin/sage-ipython:1: DeprecationWarning: (Since Sage Version 4.6.1) Variable name 'Gap' should be a string, but we obtained <class 'sage.interfaces.gap.Gap'>.
In an interactive session, you should use a definition of the form 'R.<x,y,z>=QQ[]'.
  #!/usr/bin/env python
/mnt/local/king/SAGE/sage-4.6/local/bin/sage-ipython:1: DeprecationWarning: (Since Sage Version 4.6.1) Variable name 'Singular' should be a string, but we obtained <class 'sage.interfaces.singular.Singular'>.
In an interactive session, you should use a definition of the form 'R.<x,y,z>=QQ[]'.
  #!/usr/bin/env python
Multivariate Polynomial Ring in x, Gap, Singular over Rational Field

See also #13187 where the problem of this ticket had the wrong solution (allowing for other types than string).

Depends on #23337
Depends on #23338
Depends on #23343
Depends on #23377
Depends on #23638
Depends on #23640

Component: basic arithmetic

Keywords: deprecation symbolic polynomial variable

Author: Simon King, Ralf Stephan, Jeroen Demeyer

Branch/Commit: public/10483-4 @ 938beb0

Reviewer: Karl-Dieter Crisman, Jeroen Demeyer

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

[simon-king-jena]

Author: Simon King

[simon-king-jena]

comment:1

Attachment: 10483deprecate_symbolics_in_polynomial_rings.patch.gz

Now the patch is attached. By the way: It also adds some doctests...

[kcrisman]

comment:2

Just a question - are the changes in symbolics because before the variables might or might not have been strings?

Also, is there an easy way to show what the generator of P would have been in the "bad" old case? I think that's useful to point out, if one is going to make the remarks in the first place.

[simon-king-jena]

comment:3

Replying to @kcrisman:

Just a question - are the changes in symbolics because before the variables might or might not have been strings?

As much as I know, the changes that I did in symbolics are indeed "replace a variable v by repr(v) when it is used to define a polynomial ring.

Also, is there an easy way to show what the generator of P would have been in the "bad" old case? I think that's useful to point out, if one is going to make the remarks in the first place.

I am not quite sure what you mean. By "remark", you probably mean

In earlier versions of Sage, the names have not necessarily been 
strings. Some users provided a symbolic variable instead of a string, 
and expected that this symbolic variable is the same as the generator 
of the resulting polynomial ring, which is of course not the case. 
This common mistake is now deprecated. 

So, you suggest to write something like this:

In earlier versions of Sage, the names have not necessarily been 
strings. Some users provided a symbolic variable instead of a string, 
and expected that this symbolic variable is the same as the generator 
of the resulting polynomial ring, which is of course not the case.

The following might be surpising for a user expecting the variable `x`
to be a polynomial::

    sage: P = QQ['x'] # here, some users wrote QQ[x], which is now deprecated
    sage: x == P(x)
    x == x
    sage: type(x == P(x))
    <type 'sage.symbolic.expression.Expression'>
    sage: parent(x+P(x)) == P
    False

I am not so convinced that the above text is good. Do you have a better suggesion?

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

comment:4

I'm just saying that the "of course is not the case" is not at all "of course" to me. Current behavior:

sage: P = QQ[x]
sage: P.gen()
x
sage: P.gen() == x
x == x
sage: bool(P.gen() == x)
True
sage: P.gen() is x
False

So unless you think to do the last thing, that is pretty confusing - they have identical representation methods. Which is of course why you want to change this, and introduce the deprecation.

So maybe you could say

... resulting polynomial ring.  However, this was not the case; the generator of a polynomial ring is always a polynomial type, not a symbolic expression, so although they have the same representation, they are not the same.  This explains the following warning:

<deprecation warning doctest

Does that make any sense?

[simon-king-jena]

comment:5

Replying to @kcrisman:

So maybe you could say

... resulting polynomial ring.  However, this was not the case; the generator of a polynomial ring is always a polynomial type, not a symbolic expression, so although they have the same representation, they are not the same.  This explains the following warning:

<deprecation warning doctest

Does that make any sense?

Yes, your formulation sounds much better than mine! Could you make a reviewer patch, changing the formulation?

Best regards, Simon

[kcrisman]

comment:6

Replying to @simon-king-jena:

Replying to @kcrisman:

So maybe you could say

... resulting polynomial ring.  However, this was not the case; the generator of a polynomial ring is always a polynomial type, not a symbolic expression, so although they have the same representation, they are not the same.  This explains the following warning:

<deprecation warning doctest

Does that make any sense?

Yes, your formulation sounds much better than mine! Could you make a reviewer patch, changing the formulation?

Umm... that might not happen for a little while. I can do it, though. But if I'm going to give it a formal review, I have to make sure I check that all those changes involving making things strings really catch what they are supposed to, and I don't have time to do that right now, and then make the reviewer patch, and I am slow on such things... sorry.

Also, I seem to recall there being some discussion about this on sage-devel. It's enough of a change (particularly since there are doctests which use the old one) that it could inconvenience some heavy users to see the deprecation messages, so if you have a link to that discussion I'd be interested in reading through it.

[kcrisman]

comment:7

For reference, here is the sage-devel discussion. Not that much noise, thankfully.

[kcrisman]

comment:8

Upon further reflection, I think that one would have to change a lot more of the doctests where x is used. This is because even though changing things so that they use SR['x'] instead of SR[x] is good, the same potential for confusion remains. The very first example in the patch provides the opportunity for this:

sage: R = SR['x']
sage: f = R([1/sqrt(2), 1/(4*sqrt(2))])
sage: f
1/8*sqrt(2)*x + 1/2*sqrt(2)
sage: type(f)
<class 'sage.rings.polynomial.polynomial_element_generic.Polynomial_generic_dense_field'>
sage: type(1/8*sqrt(2)*x + 1/2*sqrt(2))
<type 'sage.symbolic.expression.Expression'>
sage: type(x)
<type 'sage.symbolic.expression.Expression'>
sage: type(R.gen())
<class 'sage.rings.polynomial.polynomial_element_generic.Polynomial_generic_dense_field'>

I don't think we want to actually prohibit this - x is so conventional - but the doctests should all definitely be changed so that some variable other than the one default variable actually pre-defined by Sage is used with polynomial rings! And we might want to put more warnings about this in the polynomial ring generator docs anyway. Not to mention the tutorial :)

So putting 'needs info' until we think about what the best course of action is. But I don't want to just change these doctests, because it hides the real ambiguity, since 'x' is predefined as a variable when Sage initializes - which it should.

[simon-king-jena]

comment:9

--bump--

Any new info in the last 5 months?

[rwst]

comment:11

Pardon my naivete, why not recreate the symbol as ring element (and discard the symbol) when the user, by using it in a polynomial context, expects it to behave as one?

[nbruin]

comment:12

Replying to @rwst:

Pardon my naivete, why not recreate the symbol as ring element (and discard the symbol) when the user, by using it in a polynomial context, expects it to behave as one?

The following two invocations cannot be distinguished by the code implementing polynomial ring constructions:

x,y,z=var('x,y,z'); R = Q[x,y,z]

R = Q[ SR('x'), SR('y'), SR('z')]

Hopefully you'll agree that in the second case there is no justification to try and reach into the global scope to rebind python names that happen to print as the corresponding strings 'x','y','z'.

But thanks for bumping this ticket. Can we merge it?

[kcrisman]

comment:13

Like I said above (3 years ago? wow), it would be best to think about making the default examples for polynomial rings not use x, since that is indeed already defined system-wide, and changing the examples to use the string won't help the confusion. But I have no objection to the change in principle, just the potential for continuing confusion.

[jdemeyer]

comment:14

I also agree with the patch in principle, except I would use str() instead of repr().

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

24e21d8

Trac #10483: deprecate the misuse of symbolic variables as polynomial variable

[sagetrac-git]

Changed commit from a0e3569 to 24e21d8

[jdemeyer]

comment:108

Since the deprecation has now moved to normalize_names, I removed the added doctests in linear_code.py and quotient_ring.py which test the deprecation.

[sagetrac-git]

Changed commit from 24e21d8 to 938beb0

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

938beb0

Trac #10483: deprecate the misuse of symbolic variables as polynomial variable

[jdemeyer]

New commits:

938beb0

Trac #10483: deprecate the misuse of symbolic variables as polynomial variable

[jdemeyer]

Changed dependencies from #23337, #23338, #23343, #23377, #23638 to #23337, #23338, #23343, #23377, #23638, #23640

[jdemeyer]

comment:111

I'm going to leave this ticket be for now. So, if other people want to fix things, please go ahead.

[mkoeppe]

comment:113

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:114

Setting a new milestone for this ticket based on a cursory review.