better input handling for solve()

[sagetrac-llpamies]

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: attachment: trac_13645-solve_input_handling-rebased.patch

CC: @kcrisman

Component: symbolics

Author: Burcin Erocal

Reviewer: Punarbasu Purkayastha

Merged: sage-5.12.beta0

Issue created by migration from https://trac.sagemath.org/ticket/13645

[burcin]

Reviewer: Burcin Erocal

[burcin]

comment:1

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.

[sagetrac-llpamies]

comment:2

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,

[burcin]

comment:3

Attachment: trac_13645-solve_input_handling.patch.gz

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 @sagetrac-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.

[burcin]

Changed reviewer from Burcin Erocal to none

[burcin]

Author: Burcin Erocal

[ppurka]

comment:5

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?

[fchapoton]

comment:6

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.

[ppurka]

comment:7

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.

[kcrisman]

comment:8

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.

[ppurka]

comment:9

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.

[kcrisman]

comment:10

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

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

[ppurka]

comment:11

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.

[ppurka]

Rebased to 5.11-beta3

[ppurka]

Reviewer: Punarbasu Purkayastha

[ppurka]

comment:12

Attachment: trac_13645-solve_input_handling-rebased.patch.gz

Patchbot apply trac_13645-solve_input_handling-rebased.patch

[jdemeyer]

Merged: sage-5.12.beta0