Opened 4 years ago

Closed 4 years ago

#18084 closed enhancement (fixed)

Fix bad library uses of var()

Reported by: jdemeyer 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) Commit: 52c15f5b76067d7201659f4e7f1b3faa4a846b56
Dependencies: #18110 Stopgaps:

Description

Sage library code should not use var()!

Change History (20)

comment:1 Changed 4 years ago by 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.

comment:2 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/fix_bad_library_uses_if_var__

comment:3 Changed 4 years ago by git

  • Commit set to f680be3a9f7da9ca3957a9709997a30917bba687

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

f680be3Use SR.var()

comment:4 Changed 4 years ago by jdemeyer

  • Status changed from new to needs_review
  • Summary changed from Fix bad library uses if var() to Fix bad library uses of var()

comment:5 Changed 4 years ago by rws

  • Reviewers set to Ralf Stephan
  • Status changed from needs_review to positive_review

Looks good and passes tests in specific main categories.

comment:6 follow-up: Changed 4 years ago by kcrisman

  • Status changed from positive_review to needs_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 4 years ago by jdemeyer

  • Status changed from needs_info to needs_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 4 years ago by 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.

comment:9 follow-up: Changed 4 years ago by 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 :-(

comment:10 in reply to: ↑ 9 Changed 4 years ago by 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?

comment:11 follow-up: Changed 4 years ago by kcrisman

  • Reviewers changed from Ralf Stephan to Ralf Stephan, Karl-Dieter Crisman
  • Status changed from needs_review to needs_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 4 years ago by 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/.

comment:13 Changed 4 years ago by jdemeyer

  • Dependencies set to #18110

comment:14 Changed 4 years ago by git

  • Commit changed from f680be3a9f7da9ca3957a9709997a30917bba687 to 52c15f5b76067d7201659f4e7f1b3faa4a846b56

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 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:16 Changed 4 years ago by jdemeyer

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 4 years ago by kcrisman

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

comment:18 Changed 4 years ago by kcrisman

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

comment:19 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to positive_review

OK, so positive_review modulo #18110.

comment:20 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/fix_bad_library_uses_if_var__ to 52c15f5b76067d7201659f4e7f1b3faa4a846b56
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.