Fix 9363: Bug in cancel MatrixElement expressions

[truongduy134]

Refer to issue: #9363

Problem: When applying cancel function with expression involving MatrixElement (e.g. M[0,0] + 10), the result is not correct (MatrixElement disappears). See the following program:

 >>> from sympy import *
 >>> X = Symbol('X', commutative=False)
 >>> M = MatrixSymbol('M', 3, 3)
 >>> cancel(X + 1)
X + 1
>>> cancel(M[0,0] + 1)
1

Fix Attempt: In the condition at line https://github.com/sympy/sympy/blob/master/sympy/polys/polytools.py#L6291-L6292, use

bool(x.is_commutative) 

instead of

x.is_commutative

So if x.is_commutative is None, we treat x as non-commutative. Note that in Python, strangely (None and True) is None, similar to (None and False)) (which is the cause of the bug)

[debugger22]

What was it returning instead of a boolean?

[truongduy134]

If one of the operands is None, the expression returns None (instead of a Boolean). Quite strange indeed.

>>> a = None and True
>>> type(a)
<type 'NoneType'>

For the case of M[0,0], M[0,0].is_commutative is None. Hence, M[0,0] is included sifted[None]. However, https://github.com/truongduy134/sympy/blob/master/sympy/polys/polytools.py#L6292 considers sifted[True] and sifted[False] only. That results in M[0,0] is lost in the final result

[debugger22]

Cool. How about this one? Perhaps, it's more readable.

sifted = sift(f.args, lambda x: x.is_commutative is True and not x.has(Piecewise))

[truongduy134]

+1 . I will make this small change

[debugger22]

I'm +1 irrespective of this https://github.com/sympy/sympy/pull/9364/files#r29551392

[truongduy134]

@debugger22 : I changed the style to increase readability as you suggest :)

[cbm755]

I'm not sure about this fix: there may be a deeper issue. I knowSymbols cannot have commutative =None. Does it make sense that MatrixElement does not know if it is commutative? If that is so, then probably this patch is ok.

[jcrist]

What could the matrix be composed of that wouldn't commute? I suppose SymPy's Matrix type can hold arbitrary expressions (some of which may not commute), so having MatrixSymbol be composed of MatrixElement's with no assumptions on them may make sense. On the other hand, I can't think of a use for non-commutative elements in a matrix.

[truongduy134]

@cbm755: Although I realized this issue with MatrixElement examples, we can apply "cancel" to any kind of expressions expr in general. And may there be some cases that expr.args (which can be sub-expressions) that have is_commutative equals to None ?

[cbm755]

It looks like that sift line needs to avoid return None otherwise we can have a bug (have I got that right?). so your patch is safer in that sense. So we could merge it for that reason...

Then file an issue for commutative properies of MatrixElement.

@jcrist we forbid unknown commutativity for Symbol. Maybe should be same for contents of a MatrixSymbol??

[truongduy134]

@cbm755 : Thanks. If sift returns None, terms in None bucket will be dropped in the result

BTW, regarding MatrixElement, can we have an enhancement that we can specify assumptions for MatrixElement (just like assumptions for Symbol)?. There are a lot of use cases for such feature, like in this issue, we may need to specify commutativity for MatrixElement. Or in another issue of mine (#9305), I need to declare a MatrixSymbol of real-valued symbols.

[cbm755]

If sift returns None, terms in None bucket will be dropped in the result

I'm now +1 because of this.

And I agree element-wise assumptions on MatrixSymbols would be nice to have!

[debugger22]

BTW, regarding MatrixElement, can we have an enhancement that we can specify assumptions for MatrixElement (just like assumptions for Symbol)?

Thanks for the suggestion! I will be working on assumptions this summer. I've added it in my list.

[truongduy134]

@debugger22 : 👍 That's great

[debugger22]

I think we are on a consensus here to merge this. @cbm755

[cbm755]

+1.

Is there an explicit issue # for the MatrixElement commutativity (specially being None?)

[debugger22]

I couldn't find any.

[cbm755]

I filed #9391 for deeper issue of commutativity of MatrixElement.

@truongduy134 Thanks again for fix!

@debugger22 are you going to update AUTHORS/Aboutus or shall I?

[debugger22]

I'm sorry! I forgot to do that. Doing it now.

[debugger22]

@truongduy134 Does this look good to you? ae5742a

[truongduy134]

@debugger22 : Look great. Thanks a lot :)