Fix bad library uses of var() #18084

jdemeyer · 2015-03-29T19:49:03Z

Sage library code should not use var()!

Depends on #18110

Component: symbolics

Author: Jeroen Demeyer

Branch/Commit: 52c15f5

Reviewer: Ralf Stephan, Karl-Dieter Crisman

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

The text was updated successfully, but these errors were encountered:

nbruin · 2015-03-29T20:51:13Z

comment:1

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.

jdemeyer · 2015-03-29T21:45:40Z

Branch: u/jdemeyer/fix_bad_library_uses_if_var__

sagetrac-git · 2015-03-30T06:17:46Z

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

`f680be3`	`Use SR.var()`

sagetrac-git · 2015-03-30T06:17:46Z

Commit: f680be3

rwst · 2015-03-30T06:40:23Z

Reviewer: Ralf Stephan

rwst · 2015-03-30T06:40:23Z

comment:5

Looks good and passes tests in specific main categories.

kcrisman · 2015-03-30T17:35:46Z

comment:6

             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 · 2015-03-30T20:03:16Z

comment:7

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 · 2015-03-30T20:06:19Z

comment:8

@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 · 2015-03-31T01:19:21Z

comment:9

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 · 2015-03-31T03:16:31Z

comment:10

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 · 2015-03-31T13:08:41Z

comment:11

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.

kcrisman · 2015-03-31T13:08:41Z

Changed reviewer from Ralf Stephan to Ralf Stephan, Karl-Dieter Crisman

jdemeyer · 2015-03-31T13:31:25Z

comment:12

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

jdemeyer · 2015-04-02T15:14:54Z

Dependencies: #18110

sagetrac-git · 2015-04-02T16:03:08Z

Changed commit from f680be3 to 52c15f5

sagetrac-git · 2015-04-02T16:03:08Z

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 · 2015-04-02T16:05:34Z

comment:16

The error message is now

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

kcrisman · 2015-04-02T16:08:19Z

comment:17

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

kcrisman · 2015-04-02T16:28:55Z

comment:18

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

jdemeyer · 2015-04-02T21:26:19Z

comment:19

OK, so positive_review modulo #18110.

vbraun · 2015-04-14T19:43:08Z

Changed branch from u/jdemeyer/fix_bad_library_uses_if_var__ to 52c15f5

jdemeyer added this to the sage-6.6 milestone Mar 29, 2015

jdemeyer added c: symbolics labels Mar 29, 2015

jdemeyer changed the title ~~Fix bad library uses if var()~~ Fix bad library uses of var() Mar 30, 2015

jdemeyer added the s: needs review label Mar 30, 2015

rwst added s: positive review and removed s: needs review labels Mar 30, 2015

kcrisman added s: needs info and removed s: positive review labels Mar 30, 2015

jdemeyer added s: needs review and removed s: needs info labels Mar 30, 2015

kcrisman added s: needs work and removed s: needs review labels Mar 31, 2015

jdemeyer added s: needs review and removed s: needs work labels Apr 2, 2015

jdemeyer added s: positive review and removed s: needs review labels Apr 2, 2015

vbraun removed the s: positive review label Apr 14, 2015

vbraun closed this as completed in 08b92f3 Apr 14, 2015

jdemeyer mentioned this issue Dec 29, 2022

Stop using old_style_globals #18083

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bad library uses of var() #18084

Fix bad library uses of var() #18084

jdemeyer commented Mar 29, 2015

nbruin commented Mar 29, 2015

jdemeyer commented Mar 29, 2015

sagetrac-git mannequin commented Mar 30, 2015

sagetrac-git mannequin commented Mar 30, 2015

rwst commented Mar 30, 2015

rwst commented Mar 30, 2015

kcrisman commented Mar 30, 2015

jdemeyer commented Mar 30, 2015

jdemeyer commented Mar 30, 2015

kcrisman commented Mar 31, 2015

jdemeyer commented Mar 31, 2015

kcrisman commented Mar 31, 2015

kcrisman commented Mar 31, 2015

jdemeyer commented Mar 31, 2015

jdemeyer commented Apr 2, 2015

sagetrac-git mannequin commented Apr 2, 2015

sagetrac-git mannequin commented Apr 2, 2015

jdemeyer commented Apr 2, 2015

kcrisman commented Apr 2, 2015

kcrisman commented Apr 2, 2015

jdemeyer commented Apr 2, 2015

vbraun commented Apr 14, 2015

Fix bad library uses of var() #18084

Fix bad library uses of var() #18084

Comments

jdemeyer commented Mar 29, 2015

nbruin commented Mar 29, 2015

jdemeyer commented Mar 29, 2015

sagetrac-git mannequin commented Mar 30, 2015

sagetrac-git mannequin commented Mar 30, 2015

rwst commented Mar 30, 2015

rwst commented Mar 30, 2015

kcrisman commented Mar 30, 2015

jdemeyer commented Mar 30, 2015

jdemeyer commented Mar 30, 2015

kcrisman commented Mar 31, 2015

jdemeyer commented Mar 31, 2015

kcrisman commented Mar 31, 2015

kcrisman commented Mar 31, 2015

jdemeyer commented Mar 31, 2015

jdemeyer commented Apr 2, 2015

sagetrac-git mannequin commented Apr 2, 2015

sagetrac-git mannequin commented Apr 2, 2015

jdemeyer commented Apr 2, 2015

kcrisman commented Apr 2, 2015

kcrisman commented Apr 2, 2015

jdemeyer commented Apr 2, 2015

vbraun commented Apr 14, 2015