Fix bad library uses of var()

[jdemeyer]

Sage library code should not use var()!

[nbruin]

I assume you mean sage.calculus.calculus.var.var. Searching the source for calculus.var shows that the only files at risk for doing so are:

calculus/calculus.py:1532:        sage: sage.calculus.calculus.var_cmp(x,x)
calculus/calculus.py:1534:        sage: sage.calculus.calculus.var_cmp(x,var('z'))
calculus/calculus.py:1536:        sage: sage.calculus.calculus.var_cmp(x,var('a'))
calculus/desolvers.py:1162:        from sage.calculus.var import var
misc/sageinspect.py:188:       'sage/calculus/var.pyx'
crypto/lwe.py:99:from sage.calculus.var import var
symbolic/random_tests.py:275:    vars = [(1.0, sage.calculus.calculus.var('v%d' % (n+1))) for n in range(nvars)]
matroids/catalog.py:46:from sage.calculus.var import var
geometry/riemannian_manifolds/parametrized_surface3d.py:1508:        from sage.calculus.var import var
geometry/riemannian_manifolds/parametrized_surface3d.py:1621:        from sage.calculus.var import var
combinat/finite_state_machine.py:820:from sage.calculus.var import var
combinat/sf/jack.py:38:from sage.calculus.var import var
combinat/sf/hall_littlewood.py:28:from sage.calculus.var import var
combinat/sf/llt.py:33:from sage.calculus.var import var
combinat/sf/macdonald.py:49:from sage.calculus.var import var

unless someone has gone out of their way to obfuscate the incantation.

[git]

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

​f680be3

Use SR.var()

[rws]

Looks good and passes tests in specific main categories.

[kcrisman]

             sage: Sym = SymmetricFunctions(FractionField(QQ['t'])).macdonald()
             Traceback (most recent call last):
             ...
-            ValueError: parameter q must be in the base ring
+            TypeError: unable to convert string

Is this a regression in terms of the error message, an improvement, or just a change? To me it seems more confusing but maybe that's an appropriate message for users of this material.

[jdemeyer]

Replying to kcrisman:

             sage: Sym = SymmetricFunctions(FractionField(QQ['t'])).macdonald()
             Traceback (most recent call last):
             ...
-            ValueError: parameter q must be in the base ring
+            TypeError: unable to convert string

Is this a regression in terms of the error message, an improvement, or just a change?

Probably all three. In any case, the code is more clear, the exception type (TypeError instead of ValueError) is surely better. The error message is less clear, but that's something which can be fixed independent of this ticket.

(As a personal pet peeve, I really dislike excessive exception raising code. Moreover, I prefer a good traceback over a good error message.)

[jdemeyer]

@kcrisman: I can improve the "unable to convert string" error message on a new ticket, but only if somebody (you?) commits to reviewing it.

[kcrisman]

Until the end of the semester I can commit to nearly nothing, unfortunately, and in any case I don't know that code at all so I'd be reluctant to say what a "good" message would be :-(

[jdemeyer]

Replying to kcrisman:

Until the end of the semester I can commit to nearly nothing, unfortunately, and in any case I don't know that code at all so I'd be reluctant to say what a "good" message would be :-(

Would you be willing to just accept the change on this ticket then?

[kcrisman]

Moreover, I prefer a good traceback over a good error message.

Well, most people who don't know programming would probably prefer the opposite.

Actually, upon reading the code I think that this is definitely a regression. The only reason it complains about a string is because one doesn't do var('q'); surely there is a way to check whether the string 'q' will be a variable in the given ring. I agree that the way the original code was written is not optimal with such a default, but given that the documentation does directly refer to a parameter q (and t) this change is not good. Surely there is some way to at least use

self.q = Sym.base_ring()(q)

or something like it to try to convert the string and then give a sensible error if it's not possible, since this is clearly how it's designed. The new error message makes it sound like you aren't supposed to use a string, which is manifestly not the case - especially since the string is the default!

That said, I am happy with anything that raises an error that the user can actually understand, so I'm not suggesting one has to go back to the previous. Perhaps

try:
    self.q, self.t do their thing
except TypeError:
    do something with SR.var

and then all other errors are as they should be, instead of raising the message error as previously.

[jdemeyer]

Replying to kcrisman:

Moreover, I prefer a good traceback over a good error message.

Well, most people who don't know programming would probably prefer the opposite.

I get your point. In Python 2, this is difficult to get right. In Python 3, there is ​https://www.python.org/dev/peps/pep-3134/.

[git]

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

​abc5a70	Improve "unable to convert string" error message
​52c15f5	Merge branch 't/18110/improve__unable_to_convert_string__error_message' into t/18084/fix_bad_library_uses_if_var__

[jdemeyer]

The error message is now

TypeError: unable to evaluate 'q' in Fraction Field of Univariate Polynomial Ring in t over Rational Field

[kcrisman]

The change message here lgtm, thanks for updating that. I'll look briefly at #18110 as well.

[kcrisman]

I guess that was all I needed to review - if it still passes tests I have no further objections.

[jdemeyer]

OK, so positive_review modulo #18110.