Opened 8 years ago

Closed 8 years ago

#18084 closed enhancement (fixed)

Fix bad library uses of var()

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-6.6
Component: symbolics Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Ralf Stephan, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: 52c15f5 (Commits, GitHub, GitLab) Commit: 52c15f5b76067d7201659f4e7f1b3faa4a846b56
Dependencies: #18110 Stopgaps:

Status badges

Description

Sage library code should not use var()!

Change History (20)

comment:1 Changed 8 years ago by Nils Bruin

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.

comment:2 Changed 8 years ago by Jeroen Demeyer

Branch: u/jdemeyer/fix_bad_library_uses_if_var__

comment:3 Changed 8 years ago by git

Commit: f680be3a9f7da9ca3957a9709997a30917bba687

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

f680be3Use SR.var()

comment:4 Changed 8 years ago by Jeroen Demeyer

Status: newneeds_review
Summary: Fix bad library uses if var()Fix bad library uses of var()

comment:5 Changed 8 years ago by Ralf Stephan

Reviewers: Ralf Stephan
Status: needs_reviewpositive_review

Looks good and passes tests in specific main categories.

comment:6 Changed 8 years ago by Karl-Dieter Crisman

Status: positive_reviewneeds_info
             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.

comment:7 in reply to:  6 Changed 8 years ago by Jeroen Demeyer

Status: needs_infoneeds_review

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

comment:8 Changed 8 years ago by Jeroen Demeyer

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

comment:9 Changed 8 years ago by Karl-Dieter Crisman

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

comment:10 in reply to:  9 Changed 8 years ago by Jeroen Demeyer

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?

comment:11 Changed 8 years ago by Karl-Dieter Crisman

Reviewers: Ralf StephanRalf Stephan, Karl-Dieter Crisman
Status: needs_reviewneeds_work

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.

comment:12 in reply to:  11 Changed 8 years ago by Jeroen Demeyer

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

comment:13 Changed 8 years ago by Jeroen Demeyer

Dependencies: #18110

comment:14 Changed 8 years ago by git

Commit: f680be3a9f7da9ca3957a9709997a30917bba68752c15f5b76067d7201659f4e7f1b3faa4a846b56

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

abc5a70Improve "unable to convert string" error message
52c15f5Merge branch 't/18110/improve__unable_to_convert_string__error_message' into t/18084/fix_bad_library_uses_if_var__

comment:15 Changed 8 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:16 Changed 8 years ago by Jeroen Demeyer

The error message is now

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

comment:17 Changed 8 years ago by Karl-Dieter Crisman

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

comment:18 Changed 8 years ago by Karl-Dieter Crisman

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

comment:19 Changed 8 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

OK, so positive_review modulo #18110.

comment:20 Changed 8 years ago by Volker Braun

Branch: u/jdemeyer/fix_bad_library_uses_if_var__52c15f5b76067d7201659f4e7f1b3faa4a846b56
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.