Opened 10 years ago

Closed 9 years ago

#13645 closed defect (fixed)

better input handling for solve()

Reported by: llpamies Owned by: burcin
Priority: trivial Milestone: sage-5.12
Component: symbolics Keywords:
Cc: kcrisman Merged in: sage-5.12.beta0
Authors: Burcin Erocal Reviewers: Punarbasu Purkayastha
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by ppurka)

Consider the following Sage code:

sage: a,b=var('a,b')                    
sage: solve([a+b+a*b == 1], a)          
[a == -(b - 1)/(b + 1)]

If I want to do something similar with elements of a PolynomialRing? the code crashes:

sage: poly.<a,b> = PolynomialRing(RR)
sage: solve([a+b+a*b == 1], a)

Apply: trac_13645-solve_input_handling-rebased.patch

Attachments (2)

trac_13645-solve_input_handling.patch (3.6 KB) - added by burcin 10 years ago.
trac_13645-solve_input_handling-rebased.patch (3.7 KB) - added by ppurka 9 years ago.
Rebased to 5.11-beta3

Download all attachments as: .zip

Change History (16)

comment:1 Changed 10 years ago by burcin

  • Milestone changed from sage-5.4 to sage-duplicate/invalid/wontfix
  • Reviewers set to Burcin Erocal
  • Status changed from new to needs_review

Here is what I get:

sage: poly.<a,b> = PolynomialRing(RR)
sage: solve([a+b+a*b == 1], a)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/burcin/sage/sage-5.2/<ipython console> in <module>()

/home/burcin/sage/sage-5.2/local/lib/python2.7/site-packages/sage/symbolic/relation.pyc in solve(f, *args, **kwds)
    676     for v in variables:
    677         if not is_SymbolicVariable(v):
--> 678             raise TypeError, "%s is not a valid variable."%v
    679 
    680     try:

TypeError: not all arguments converted during string formatting

Is this what you mean by a "crash"?

Note that when you work with variables in a polynomial ring, the comparison is evaluated immediately.

sage: poly.<a,b> = PolynomialRing(RR)
sage: a+b+a*b == 1             
False

In this case, the call to solve becomes solve([False], a). The comparison operator for polynomial rings is important (think monomial orders and Groebner bases), we cannot change it to keep the relations symbolic.

I suggest we close this as invalid.

comment:2 follow-up: Changed 10 years ago by llpamies

Tanks burcin, do not close the ticket, I think that something needs to be fixed.

If you say that,

The comparison operator for polynomial rings is important (think monomial orders and Groebner bases), we cannot change it to keep the relations symbolic.

Shouldn't Sage raise an exception when evaluating "a+b+a*b == 1", saying something like "Symbolic comparisons are not allowed for polynomial rings.", instead of returning False. And besides that, why is "solve([False], a)" raising such a incomprehensible "TypeError?" exception ?

Thanks,

Changed 10 years ago by burcin

comment:3 in reply to: ↑ 2 Changed 10 years ago by burcin

  • Authors set to Burcin Erocal
  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-5.4
  • Priority changed from major to trivial
  • Reviewers Burcin Erocal deleted

OK. I attached a patch to fix the error message. Now we have:

sage: poly.<a,b> = PolynomialRing(RR)
sage: solve([a+b+a*b == 1], a) 
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/burcin/sage/sage-5.2/<ipython console> in <module>()

/home/burcin/sage/sage-5.2/local/lib/python2.7/site-packages/sage/symbolic/relation.pyc in solve(f, *args, **kwds)
    681             return f[0].solve(*args,**kwds)
    682         # otherwise complain

--> 683         raise TypeError("The first argument to solve() should be a symbolic expression or a list of symbolic expressions, cannot handle %s"%repr(type(f[0])))
    684 
    685     # f is a list of such expressions or equations


TypeError: The first argument to solve() should be a symbolic expression or a list of symbolic expressions, cannot handle <type 'bool'>

Please review.

Replying to llpamies:

If you say that,

The comparison operator for polynomial rings is important (think monomial orders and Groebner bases), we cannot change it to keep the relations symbolic.

Shouldn't Sage raise an exception when evaluating "a+b+a*b == 1", saying something like "Symbolic comparisons are not allowed for polynomial rings.", instead of returning False.

No, that operations checks for equality of polynomials. That polynomial is not equal to 1. As I wrote before, we cannot modify the comparison of polynomials.

And besides that, why is "solve([False], a)" raising such a incomprehensible "TypeError?" exception ?

This was caused by a typo. Python was complaining about a different issue when it said TypeError: not all arguments converted during string formatting. I hope the new error message is more useful.

comment:4 Changed 10 years ago by burcin

  • Summary changed from PolynomialRing variables are not generic symbolic variables to better input handling for solve()

comment:5 Changed 10 years ago by ppurka

Should this be explicitly provided in the documentation of solve as a warning? It seems natural that someone could try to use variables from a polynomial ring and work with them. After all, they are "variables."

I have something like this in mind:

.. warning:

    Provide only symbolic variables to the ``solve`` function, that is,
    variables obtained by using the :func:`var`. Other variables, for instance
    variables obtained from polynomial rings, should not be provided to the
    ``solve`` function.

what other variables can be defined in Sage, by the way?

comment:6 Changed 9 years ago by chapoton

There is another problem here, not related to equality:

sage: a,b=var('a,b')                 
sage: solve([a+b+a*b- 1], a)
[a == -(b - 1)/(b + 1)]
sage: poly.<a,b> = PolynomialRing(RR)
sage: solve([a+b+a*b- 1], a)         

The last line does not work, but it could and should ! It can be done by converting polynomials to elements of the symbolic ring.

comment:7 Changed 9 years ago by ppurka

Here is an example code which can convert the polynomial to a symbolic expression.

R.<y,z> = RR[]
a = y^2 + 2 * z^2

is_Polynomial(a)  # test for polynomial a
False

is_MPolynomial(a) # test for multinomial a
True

va = a.variables()
vSR = map(SR, va)
aSR = a.subs({_x: _xs for _x,_xs in zip(va, vSR)})

type(aSR)
<type 'sage.symbolic.expression.Expression'>

solve(aSR, *vSR)
([y == -I*sqrt(2)*z, y == I*sqrt(2)*z], [1, 1])

This can be done internally in the solve function.

There is only one problem to this approach. Polynomial rings can be over other fields, for instance finite fields, too. The output of solve in this case doesn't really make much sense because it loses the information that the elements are in finite fields.

sage: F.<b> = GF(4)
sage: R.<y,z> = F[]
sage: a = y^2 + b* z^2
sage: type(a)
<type 'sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular'>
sage: is_MPolynomial(a)
True

sage: va = a.variables()
sage: vSR = map(SR, va)
sage: aSR = a.subs({_x: _xs for _x,_xs in zip(va, vSR)})
sage: type(aSR)
<type 'sage.symbolic.expression.Expression'>
sage: aSR
y^2 + b*z^2
sage: solve(aSR, *vSR)
([y == -sqrt(-b)*z, y == sqrt(-b)*z], [1, 1])

sage: aSR.coeffs(z)
[[y^2, 0], [b, 2]]
sage: type(aSR.coeffs(z)[1][0])
<type 'sage.symbolic.expression.Expression'>

So, we need some test to check whether the polynomial is defined only over reals or complexes.

comment:8 Changed 9 years ago by kcrisman

  • Cc kcrisman added

I somehow feel like maybe it makes more sense that if we have type bool in the list of things as in Burcin's error message

TypeError: The first argument to solve() should be a symbolic expression or a list of symbolic expressions, cannot handle <type 'bool'>

That perhaps we can suggest to the user that instead of a polynomial ring, they should use symbolic variables (rather than the message about bool, which will no doubt confuse people). Of course, there is the other question of whether we want

solve([True,x==1],x)

to return x==1 as a solution, which would muddle this whole approach up.

I don't mind (uncharacteristically) if people can't get an answer to this, not only for the base field issue, but also because we don't want people using polynomial rings any more unless they know what they are doing. The symbolic ring should take care of all "calculus mode" users, which was not the case in the (long ago) past.

comment:9 Changed 9 years ago by ppurka

Yikes. Indeed, I forgot that polynomial ring elements actually evaluate to a boolean when input with comparison operators. My example will working only when the relation is (implicitly) equality to zero.

Then Burcin's patch seems the right thing to do.

comment:10 follow-up: Changed 9 years ago by kcrisman

Then Burcin's patch seems the right thing to do.

So... do you want to review this, then?

comment:11 in reply to: ↑ 10 Changed 9 years ago by ppurka

  • Status changed from needs_review to positive_review

Replying to kcrisman:

Then Burcin's patch seems the right thing to do.

So... do you want to review this, then?

Yes. Positive review. I am uploading a rebased patch.

Changed 9 years ago by ppurka

Rebased to 5.11-beta3

comment:12 Changed 9 years ago by ppurka

  • Description modified (diff)
  • Reviewers set to Punarbasu Purkayastha

Patchbot apply trac_13645-solve_input_handling-rebased.patch

comment:13 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:14 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.12.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.