Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

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

GitHub link to the corresponding issue

Description (last modified by kcrisman)

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)

trac_8553_solve_dictionary_result_fix.patch (1.8 KB) - added by leif 13 years ago.
trac_8553_solve_dictionary_result_fix-variant2.patch (2.8 KB) - added by leif 13 years ago.
trac_8553_solve_fix_IndexError.patch (7.2 KB) - added by leif 13 years ago.
trac_8553_solve_fix_IndexError.2.patch (7.7 KB) - added by timdumol 13 years ago.
Rebase on sage-4.4.alpha0.
trac_8553_solve_fix_IndexError.3.patch (6.8 KB) - added by burcin 13 years ago.
revised version of Leif's patch
trac_8553-review.patch (6.3 KB) - added by burcin 13 years ago.
reviewer's patch

Download all attachments as: .zip

Change History (41)

comment:1 Changed 13 years ago by leif

sage: x=var('x')
sage: x
x
sage: solve([0==1], x, solution_dict = True)
[]
sage: solve([x==0, 0==1], x, solution_dict = True)
[]
sage: solve([(x == 0), (x == 1)], x, solution_dict = True)
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
...
IndexError: list index out of range

comment:2 Changed 13 years ago by leif

Status: newneeds_review

comment:3 Changed 13 years ago by schilly

Reviewers: schilly
Status: needs_reviewpositive_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 schilly

Milestone: sage-4.3.4

comment:5 Changed 13 years ago by mhansen

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 Changed 13 years ago by mhansen

Or even

{} if kwds.get('solution_dict', False) else []

if you want to be a bit more lax with inputs.

comment:7 in reply to:  6 Changed 13 years ago by leif

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 Changed 13 years ago by 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.

comment:9 in reply to:  8 Changed 13 years ago by leif

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 mhansen

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 leif

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 leif

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 Changed 13 years ago by leif

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 in reply to:  13 ; Changed 13 years ago by schilly

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 in reply to:  14 ; Changed 13 years ago by leif

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 in reply to:  15 Changed 13 years ago by leif

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

comment:17 Changed 13 years ago by leif

Apply either the first or the second patch, not both.

comment:18 Changed 13 years ago by burcin

Reviewers: schillyHarald Schilly, Mike Hansen, Burcin Erocal
Status: positive_reviewneeds_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 in reply to:  18 ; Changed 13 years ago by 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.

comment:20 Changed 13 years ago by leif

Replying to burcin:

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.

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 in reply to:  19 ; Changed 13 years ago by leif

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 in reply to:  21 Changed 13 years ago by burcin

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 leif

comment:23 Changed 13 years ago by leif

Cc: burcin added
Description: modified (diff)
Status: needs_workneeds_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 in expression.pyx/Expression.solve()).
  • (More) Doctests added.
  • Description made more clear. Some typos fixed.

comment:24 Changed 13 years ago by jason

Cc: jason added

Changed 13 years ago by timdumol

Rebase on sage-4.4.alpha0.

comment:25 Changed 13 years ago by timdumol

Works perfectly after this rebase. Positive review, but someone has to review this rebase first.

Changed 13 years ago by burcin

revised version of Leif's patch

Changed 13 years ago by burcin

Attachment: trac_8553-review.patch added

reviewer's patch

comment:26 Changed 13 years ago by 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. 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 in reply to:  26 ; Changed 13 years ago by 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 ints vs. Sage's Integers are omitted.)

comment:28 in reply to:  27 ; Changed 13 years ago by burcin

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 ints vs. Sage's Integers 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 leif

Keywords: beginner added

comment:30 in reply to:  28 ; Changed 13 years ago by leif

Status: needs_reviewneeds_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 kcrisman

Cc: kcrisman added

comment:32 Changed 12 years ago by kcrisman

Just for reference, see this sage-support thread for another report of this.

comment:33 in reply to:  30 Changed 12 years ago by kcrisman

Description: modified (diff)
Milestone: sage-4.7sage-4.7.1
Reviewers: Harald Schilly, Mike Hansen, Burcin ErocalHarald Schilly, Mike Hansen, Burcin Erocal, Karl-Dieter Crisman
Status: needs_workpositive_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 jdemeyer

Merged in: sage-4.7.1.alpha2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.