Opened 6 years ago
Closed 6 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, GitHub, GitLab) | Commit: | 52c15f5b76067d7201659f4e7f1b3faa4a846b56 |
Dependencies: | #18110 | Stopgaps: |
Description
Sage library code should not use var()
!
Change History (20)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
- Branch set to u/jdemeyer/fix_bad_library_uses_if_var__
comment:3 Changed 6 years ago by
- Commit set to f680be3a9f7da9ca3957a9709997a30917bba687
Branch pushed to git repo; I updated commit sha1. New commits:
f680be3 | Use SR.var()
|
comment:4 Changed 6 years ago by
- 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 6 years ago by
- 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: ↓ 7 Changed 6 years ago by
- 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 6 years ago by
- 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 stringIs 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 6 years ago by
@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: ↓ 10 Changed 6 years ago by
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 6 years ago by
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: ↓ 12 Changed 6 years ago by
- 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 6 years ago by
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 6 years ago by
- Dependencies set to #18110
comment:14 Changed 6 years ago by
- Commit changed from f680be3a9f7da9ca3957a9709997a30917bba687 to 52c15f5b76067d7201659f4e7f1b3faa4a846b56
comment:15 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:16 Changed 6 years ago by
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 6 years ago by
The change message here lgtm, thanks for updating that. I'll look briefly at #18110 as well.
comment:18 Changed 6 years ago by
I guess that was all I needed to review - if it still passes tests I have no further objections.
comment:19 Changed 6 years ago by
- Status changed from needs_review to positive_review
OK, so positive_review modulo #18110.
comment:20 Changed 6 years ago by
- Branch changed from u/jdemeyer/fix_bad_library_uses_if_var__ to 52c15f5b76067d7201659f4e7f1b3faa4a846b56
- Resolution set to fixed
- Status changed from positive_review to closed
I assume you mean
sage.calculus.calculus.var.var
. Searching the source forcalculus.var
shows that the only files at risk for doing so are:unless someone has gone out of their way to obfuscate the incantation.