#8553 closed defect (fixed)
Minor bugs in solve() if dictionary result requested
Reported by: | leif | Owned by: | leif |
---|---|---|---|
Priority: | trivial | Milestone: | sage-4.7.1 |
Component: | symbolics | Keywords: | solve, solution_dict, beginner |
Cc: | schilly, mhansen, burcin, jason, kcrisman | Merged in: | sage-4.7.1.alpha2 |
Authors: | Leif Leonhardy | Reviewers: | Harald Schilly, Mike Hansen, Burcin Erocal, Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
solve()
raises an IndexError
on empty solutions if a list of solution dictionaries is requested.
The description/examples may mislead w.r.t. the result type.
Apply trac_8553_solve_fix_IndexError.3.patch and trac_8553-review.patch.
Attachments (6)
Change History (41)
Changed 13 years ago by
Attachment: | trac_8553_solve_dictionary_result_fix.patch added |
---|
comment:2 Changed 13 years ago by
Status: | new → needs_review |
---|
comment:3 Changed 13 years ago by
Reviewers: | → schilly |
---|---|
Status: | needs_review → positive_review |
patch works for 4.3.4.rc0 and looks fine. i would just like to see a "TESTS::" header for the additional doctests and a line referring to this ticket, smth. like "this fixes the problem reported in ticket #8553".
comment:4 Changed 13 years ago by
Milestone: | → sage-4.3.4 |
---|
comment:5 Changed 13 years ago by
Style-wise, this
{} if 'solution_dict' in kwds and kwds['solution_dict']==True else []
should be
{} if kwds.get('solution_dict', False) is True else []
comment:6 follow-up: 7 Changed 13 years ago by
Or even
{} if kwds.get('solution_dict', False) else []
if you want to be a bit more lax with inputs.
comment:7 Changed 13 years ago by
Cc: | mhansen added |
---|
Replying to mhansen:
Or even
{} if kwds.get('solution_dict', False) else []if you want to be a bit more lax with inputs.
Please correct me, but I do not see any difference to your former suggestion. In what sense does this relax the inputs?
comment:8 follow-up: 9 Changed 13 years ago by
In the first cases, passing in solution_dict=1 would be the same as passing in solution_dict=False. In the second case, solution_dict=1 would be the same as solution_dict=True.
comment:9 Changed 13 years ago by
Replying to mhansen:
In the first cases, passing in solution_dict=1 would be the same as passing in solution_dict=False. In the second case, solution_dict=1 would be the same as solution_dict=True.
That's perhaps what one would expect.
But solution_dict=1
evaluates to True
in all versions, while solution_dict=42
surprisingly evaluates to False
, even in your latter version.
comment:10 Changed 13 years ago by
sage: kwds = {'solution_dict': 42} sage: {} if 'solution_dict' in kwds and kwds['solution_dict']==True else [] {} sage: {} if kwds.get('solution_dict', False) is True else [] [] sage: {} if kwds.get('solution_dict', False) else [] {} sage: kwds = {'solution_dict': 42r} sage: {} if 'solution_dict' in kwds and kwds['solution_dict']==True else [] [] sage: {} if kwds.get('solution_dict', False) is True else [] [] sage: {} if kwds.get('solution_dict', False) else [] {}
comment:11 Changed 13 years ago by
I have to corrct myself: 1
gives True
only in the first (because of ==
comparison) and the last implementation.
comment:12 Changed 13 years ago by
sage: kwds = {'solution_dict': 42} sage: {} if 'solution_dict' in kwds and kwds['solution_dict']==True else [] [] sage: {} if kwds.get('solution_dict', False) is True else [] [] sage: {} if kwds.get('solution_dict', False) else [] {} sage: sage: kwds = {'solution_dict': 42r} sage: {} if 'solution_dict' in kwds and kwds['solution_dict']==True else [] [] sage: {} if kwds.get('solution_dict', False) is True else [] [] sage: {} if kwds.get('solution_dict', False) else [] {}
4.3.4.alpha1
Notice the difference in the first case; 42 is not true for me ;-)
comment:13 follow-up: 14 Changed 13 years ago by
sage: kwds = {'solution_dict': 42} sage: print kwds {'solution_dict': 42} sage: sage: kwds = {'solution_dict': 42r} sage: print kwds {'solution_dict': 42}
Preparser issue?
comment:14 follow-up: 15 Changed 13 years ago by
Replying to leif:
Preparser issue?
no, that's a feature and notice there is an "r" behind the 42!
sage: preparse('42r + 42') '42 + Integer(42)'
comment:15 follow-up: 16 Changed 13 years ago by
Replying to schilly:
Ok, but I still don't understand the different behavior of Mike's and my machine:
sage: kwds = {'solution_dict': 42} sage: {} if 'solution_dict' in kwds and kwds['solution_dict']==True else []
While Mike gets the empty set, I get the empty list, and if the doctests passed on your 4.3.4.rc0, your version behaves like my 4.3.4.alpha1 (on Ubuntu 9.04 x86_64), because I added the solution_dict=42
case with an expected empty list.
comment:16 Changed 13 years ago by
Replying to leif:
I still get the empty list for the code above with 4.3.4.rc0 (just built on Ubuntu 9.04 x86_64), but I'll update the patch to the latter suggested version (with the 42-testcase changed, i.e. inverted!).
Changed 13 years ago by
Attachment: | trac_8553_solve_dictionary_result_fix-variant2.patch added |
---|
comment:18 follow-up: 19 Changed 13 years ago by
Reviewers: | schilly → Harald Schilly, Mike Hansen, Burcin Erocal |
---|---|
Status: | positive_review → needs_work |
AFAICT, solve
returns a list of possible solutions to the given equality. When the solution_dict
parameter is True
, this list is populated by dictionaries instead of symbolic relations.
With this reasoning the current behavior of returning an empty list if we can't find any solutions even if the solution_dict
parameter is True
seems correct to me.
IMHO, the only thing that needs to be fixed is the IndexError
when there is no solution. Comments?
comment:19 follow-up: 21 Changed 13 years ago by
Replying to burcin:
IMHO, the only thing that needs to be fixed is the
IndexError
when there is no solution. Comments?
Yes please, I don't get it why this is so complicated! All unrelated issues should be discussed off-ticket.
comment:20 Changed 13 years ago by
Replying to burcin:
AFAICT,
solve
returns a list of possible solutions to the given equality. When thesolution_dict
parameter isTrue
, this list is populated by dictionaries instead of symbolic relations.With this reasoning the current behavior of returning an empty list if we can't find any solutions even if the
solution_dict
parameter isTrue
seems correct to me.
That's really a matter of design; one could equally well specify the function to return a list containing an empty dictionary in that case; i.e., the current specification is not precise (and that's not the only one).
But you're right, unless we change the informal "prototype" (its signature), it should in any case return a list of something.
I'd change the patch to reflect whatever behavior you want... ;-)
-Leif
comment:21 follow-up: 22 Changed 13 years ago by
Replying to schilly:
Replying to burcin:
IMHO, the only thing that needs to be fixed is the
IndexError
when there is no solution. Comments?Yes please, I don't get it why this is so complicated! All unrelated issues should be discussed off-ticket.
So we'd change the ticket description and the patch to only address and fix the index error?
Ok for me.
comment:22 Changed 13 years ago by
Hi Leif,
Replying to leif:
So we'd change the ticket description and the patch to only address and fix the index error?
Ok for me.
Can you provide a patch that fixes the IndexError
problem? If we get this reviewed in the next few days it might still make it to the next release.
Changed 13 years ago by
Attachment: | trac_8553_solve_fix_IndexError.patch added |
---|
comment:23 Changed 13 years ago by
Cc: | burcin added |
---|---|
Description: | modified (diff) |
Status: | needs_work → needs_review |
Sorry, unable to delete the previous patches; apply only the last one.
IndexError
issue fixed.- Type of parameter
solution_dict
relaxed as suggested by Mike Hansen (now consistent; also inexpression.pyx
/Expression.solve()
). - (More) Doctests added.
- Description made more clear. Some typos fixed.
comment:24 Changed 13 years ago by
Cc: | jason added |
---|
Changed 13 years ago by
Attachment: | trac_8553_solve_fix_IndexError.2.patch added |
---|
Rebase on sage-4.4.alpha0.
comment:25 Changed 13 years ago by
Works perfectly after this rebase. Positive review, but someone has to review this rebase first.
Changed 13 years ago by
Attachment: | trac_8553_solve_fix_IndexError.3.patch added |
---|
revised version of Leif's patch
comment:26 follow-up: 27 Changed 13 years ago by
The doctests for using values that are taken to be True in Leif's patch added a few extra seconds to the time to test sage/symbolic/relation.py
. I uploaded a revised version of Leif's patch, attachment:trac_8553_solve_fix_IndexError.3.patch, which cuts down on these examples.
attachment:trac_8553-review.patch adds a new test condition to check if the first argument is a list, after the statement that handles a single symbolic expression.
Can someone review my patch?
comment:27 follow-up: 28 Changed 13 years ago by
Replying to burcin:
The doctests for using values that are taken to be True in Leif's patch added a few extra seconds to the time to test
sage/symbolic/relation.py
.
:-) Adding # optional
or # long
would have been possible, too.
(Now the tests of Python int
s vs. Sage's Integer
s are omitted.)
comment:28 follow-up: 30 Changed 13 years ago by
Authors: | → Leif Leonhardy |
---|
Replying to leif:
Replying to burcin:
The doctests for using values that are taken to be True in Leif's patch added a few extra seconds to the time to test
sage/symbolic/relation.py
.:-) Adding
# optional
or# long
would have been possible, too.(Now the tests of Python
int
s vs. Sage'sInteger
s are omitted.)
Since they weren't testing the functionality of solve()
, but a convenience in Python, I think it's OK if they are not so extensive. I run doctests on these files very often, so it's rather important that even the long tests take a reasonable time.
Can you review my changes BTW?
comment:29 Changed 13 years ago by
Keywords: | beginner added |
---|
comment:30 follow-up: 33 Changed 13 years ago by
Status: | needs_review → needs_work |
---|
Replying to burcin:
Can you review my changes BTW?
m.to_poly_solve(variables)
might be called twice.
Btw, I think the patch should be finally reviewed by someone else, not me, as I'm one of the authors.
comment:31 Changed 12 years ago by
Cc: | kcrisman added |
---|
comment:32 Changed 12 years ago by
Just for reference, see this sage-support thread for another report of this.
comment:33 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Milestone: | sage-4.7 → sage-4.7.1 |
Reviewers: | Harald Schilly, Mike Hansen, Burcin Erocal → Harald Schilly, Mike Hansen, Burcin Erocal, Karl-Dieter Crisman |
Status: | needs_work → positive_review |
Replying to leif:
Replying to burcin:
Can you review my changes BTW?
m.to_poly_solve(variables)
might be called twice.
It is, but only in the unusual situation that Maxima's solve gave an error and to_poly_solve
gave the empty list. More to the point, this was there before, and should be a different ticket.
Btw, I think the patch should be finally reviewed by someone else, not me, as I'm one of the authors.
Not at all - usually it is appropriate to review a reviewer patch. This reviewer patch really is pretty minimally invasive, and in fact provides a much more useful error message.
It is true that
sage: solve(x==1,x,solution_dict=int(3)) [{x: 1}] sage: solve([0==1],x,solution_dict=int(3)) [] sage: solve(0==1,x,solution_dict=int(3)) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) /Users/karl-dietercrisman/Downloads/sage-4.7.rc1/<ipython console> in <module>() /Users/karl-dietercrisman/Downloads/sage-4.7.rc1/local/lib/python2.6/site-packages/sage/symbolic/relation.pyc in solve(f, *args, **kwds) 652 653 if not isinstance(f, (list, tuple)): --> 654 raise TypeError("The first argument must be a symbolic expression or a list of symbolic expressions.") 655 656 if len(f)==1 and is_Expression(f[0]): TypeError: The first argument must be a symbolic expression or a list of symbolic expressions.
which is not ideal. However, this is still better behavior than the mysterious message about bools and len before, so that could be another ticket.
In any case, this (somewhat surprisingly) still applies fine to 4.7.rc2! And does what it says. And passes tests. Positive review.
comment:34 Changed 12 years ago by
Merged in: | → sage-4.7.1.alpha2 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:35 Changed 12 years ago by
For the sagebot:
Apply trac_8553_solve_fix_IndexError.3.patch trac_8553-review.patch