Opened 9 years ago

Last modified 6 years ago

#10750 needs_work defect

Fix solve so that additional args are properly handled

Reported by: kcrisman Owned by: burcin
Priority: major Milestone:
Component: symbolics Keywords: solve, maxima, Maxima, sd35.5
Cc: jhpalmieri Merged in:
Authors: John Palmieri Reviewers: Paul Zimmermann
Report Upstream: N/A Work issues: error in Maxima
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

In this ask.sagemath question, there is some inconsistency in our use of Maxima's solve, or possibly its solving.

s=list(var('s_%d' % i) for i in range(3));
var('x y z');
print solve([],s_0,s_1,s_2)
print solve([z==z],s_0,s_1,s_2)
print solve([z==z,y==y],s_0,s_1,s_2)
print solve([z==z,y==y,x==x],s_0,s_1,s_2)
print solve([s_0==0],s_0,s_1,s_2);
print solve([s_0==0,s_1==s_1],s_0,s_1,s_2)
[[s_0 == c1, s_1 == c2, s_2 == c3]]
({s_0: r1}, [])
[[s_0 == r6, s_1 == r5, s_2 == r4]]
[[s_0 == r9, s_1 == r8, s_2 == r7]]
([s_0 == 0], [1])
[[s_0 == 0, s_1 == r11, s_2 == r10]]

Most of these do make sense, but you'll notice the change from complex to real variables, one place that seems to have a multiplicity, and the one with only a dictionary as output!

See also this sage-devel thread for another example of where this cropped up. We need to change the behavior of solve and the documentation to fix this.

Attachments (1)

trac_10750-solve.patch (4.3 KB) - added by jhpalmieri 8 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 follow-up: Changed 9 years ago by burcin

AFAIK, the issue with only a dictionary output was fixed in #8553. Multiplicity seems to be (one of) the problem(s) in #5679. Perhaps we should make this ticket only about the real & complex variables.

There are so many tickets about solve(). It would be good to make a trac wiki page to list all of them. This should help avoid duplicates.

comment:2 in reply to: ↑ 1 Changed 8 years ago by kcrisman

There are so many tickets about solve(). It would be good to make a trac wiki page to list all of them. This should help avoid duplicates.

This is here.

comment:3 Changed 8 years ago by kcrisman

  • Cc jhpalmieri added
  • Description modified (diff)

Okay, it turns out that there are a few problems here.

  • When one solves a single Expression, all args are passed to Expression.solve() which means s_1 goes to multiplicities, and so we are getting multiplicities with the trailing [] and [1].
  • If one does this with three variables, then solution_dict is also True and we get the dict.
  • Then there is the complex/real issue, which I haven't delved into.

We should fix this all so that the solutions make sense and are consistent.

comment:4 Changed 8 years ago by kcrisman

  • Description modified (diff)

comment:5 Changed 8 years ago by kcrisman

  • Description modified (diff)

comment:6 Changed 8 years ago by kcrisman

  • Summary changed from Unify certain solve outputs to Fix solve so that additional args are properly handled

comment:7 Changed 8 years ago by jhpalmieri

The documentation for "solve" in symbolic/relation.py says that the arguments are

    -  ``f`` - equation or system of equations (given by a
       list or tuple)
    
    -  ``*args`` - variables to solve for.
    
    -  ``solution_dict``

The function is defined as def solve(f, *args, **kwds):. So I think we can assume that args is the list of variables, whereas any keywords are in kwds. So

  • we should document some more possible keywords, like multiplicities, or at least give pointers to documentation to any other solve routines which get called by this one, and
  • if args has length greater than one, then we obviously shouldn't just call Expression.solve(), but deal with it some other way. Can we just change the beginning?
    • sage/symbolic/relation.py

      diff --git a/sage/symbolic/relation.py b/sage/symbolic/relation.py
      a b def solve(f, *args, **kwds): 
      646646        []
      647647    """
      648648    from sage.symbolic.expression import is_Expression
      649     if is_Expression(f): # f is a single expression
       649    # f is a single expression and there is a single variable
       650    if is_Expression(f) and len(args) == 1:
      650651        ans = f.solve(*args,**kwds)
      651652        return ans
    (This is completely untested. Maybe we also need to check is_SymbolicVariable(args[0])...)

comment:8 Changed 8 years ago by kcrisman

Some of the documentation things you mention are dealt with (to some extent) at #10444, which bitrotted a bit.

Your potential solution seems reasonable - really (as Jason implies in his post about the Python keywords issue) the args shouldn't go on without keywords in this case. I wouldn't worry as much about the is_Symbolic... though I guess it couldn't hurt.

comment:9 follow-up: Changed 8 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Status changed from new to needs_review

Oh, I didn't see your comment, so I wrote a new patch which adds the same documentation as in #10444, although not quite in the same way. The attached patch passes doctests for me, at least for the patched file. It just adds one new doctest, to illustrate that you can solve for a single equation with respect to more than one variable.

I'm marking as this as "needs review", but it may just be a first draft. I certainly haven't tested it very thoroughly.

Changed 8 years ago by jhpalmieri

comment:10 in reply to: ↑ 9 Changed 8 years ago by kcrisman

I'm marking as this as "needs review", but it may just be a first draft. I certainly haven't tested it very thoroughly.

I think you changed the previous test, which is fine. But we should catch the solution_dict issue as well (that is to say we should test that that error no longer occurs).

To test this, one should especially try to make things which previously would have gone into Expression.solve break. For instance, we would want to make sure multiplicities still could work as well as possible. I think your code will keep that, but it's what would want to be checked. I do not think I will have time to do that right now, sadly.

Someone would also want to just check whether this is enough to close #10444 as a dup, which jhpalmieri is suggesting and I suspect is true.

comment:11 Changed 8 years ago by zimmerma

  • Keywords sd35.5 added
  • Reviewers set to Paul Zimmermann
  • Status changed from needs_review to needs_work
  • Work issues set to error in Maxima

after importing the patch on top of sage-4.8.apha6, I get:

Loading Sage library. Current Mercurial branch is: 10750
sage: s=list(var('s_%d' % i) for i in range(3));
sage: var('x y z');
sage: print solve([],s_0,s_1,s_2)
[
[s_0 == c1, s_1 == c2, s_2 == c3]
]
sage: print solve([z==z],s_0,s_1,s_2)
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)

/localdisk/tmp/sage-4.8.alpha6/<ipython console> in <module>()

/localdisk/tmp/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/symbolic/relation.pyc in solve(f, *args, **kwds)
    736                 raise
    737 
--> 738     if len(s)==0: # if Maxima's solve gave no solutions, try its to_poly_solve
    739         try:
    740             s = m.to_poly_solve(variables)

/localdisk/tmp/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/interfaces/maxima_abstract.pyc in __len__(self)
   1618         """
   1619         P = self._check_valid()
-> 1620         return int(P.eval('length(%s)'%self.name()))
   1621 
   1622     def dot(self, other):

/localdisk/tmp/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/interfaces/maxima_lib.pyc in _eval_line(self, line, locals, reformat, **kwds)
    418                     statement = line[:ind_semi]
    419                     line = line[ind_semi+1:]
--> 420                 if statement: result = ((result + '\n') if result else '') + max_to_string(maxima_eval("#$%s$"%statement))
    421             else:
    422                 statement = line[:ind_dollar]

/localdisk/tmp/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/libs/ecl.so in sage.libs.ecl.EclObject.__call__ (sage/libs/ecl.c:4732)()

/localdisk/tmp/sage-4.8.alpha6/local/lib/python2.6/site-packages/sage/libs/ecl.so in sage.libs.ecl.ecl_safe_apply (sage/libs/ecl.c:2838)()

RuntimeError: ECL says: Error executing code in Maxima: length: argument cannot be a symbol; found all

Paul

comment:12 Changed 8 years ago by kcrisman

Paul's problem is just that Maxima recognized that everything is a solution and returned all.

(%i1) solve(q=q,(w,y,z));
(%o1)                                 all
(%i2) solve(w=w,(w,y,z));
(%o2)                                 all
(%i3) solve(w=w,y);
(%o3)                                 all
(%i4) solve(w=w,w);
(%o4)                                 all

We would need to special-case this somehow. Good eye!


Here's another place where someone might have run into this problem. We should really fix this soon.

comment:13 Changed 7 years ago by kcrisman

See also #13286, which is yet another inconsistency.

comment:14 Changed 6 years ago by kcrisman

I think http://ask.sagemath.org/question/3526/ is probably related to this as well.

Note: See TracTickets for help on using tickets.