New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dummy(commutative=False).is_zero -> False #10728

Open
smichr opened this Issue Mar 4, 2016 · 13 comments

Comments

Projects
None yet
4 participants
@smichr
Copy link
Member

smichr commented Mar 4, 2016

>>> from sympy.matrices import MatrixSymbol
>>> MatrixSymbol('A', 5, 3).is_zero
False

Why does this think it is not zero? Wouldn't it be better to give None? I found the is_ZeroMatrix attribute, but changing it to None doesn't affect the above behavior.

smichr added a commit to smichr/sympy that referenced this issue Mar 4, 2016

10401: evaluate Eq/Ne when the lhs - rhs evaluates
The condition that the lhs and rhs be complex was added
without a test originally and it is not clear what it is
there for. The equality can still evaluate if one side
is real and the other complex. The condition that both
sides be Expr guards against objects thinking they are
not zero (like MatAdd -- see issue sympy#10728.

The test that was corrected wouldn't fail as it was, but
the inequality now evaluates so the simpler expression
was used.

smichr added a commit to smichr/sympy that referenced this issue Mar 4, 2016

10401: evaluate Eq/Ne when the lhs - rhs evaluates
The condition that the lhs and rhs be complex was added
without a test originally and it is not clear what it is
there for. The equality can still evaluate if one side
is real and the other complex. The condition that both
sides be Expr guards against objects thinking they are
not zero (like MatAdd -- see issue sympy#10728.

The test that was corrected wouldn't fail as it was, but
the inequality now evaluates so the simpler expression
was used.
@AnishShah

This comment has been minimized.

Copy link
Contributor

AnishShah commented Mar 5, 2016

@smichr MatrixSymbol has an explicit assumption: commutative=False, which deduces zero=False

In [8]: StdFactKB({'commutative': False})
Out[8]: 
{'algebraic': False,
 'commutative': False,
 'complex': False,
 'composite': False,
 'even': False,
 'imaginary': False,
 'integer': False,
 'irrational': False,
 'negative': False,
 'noninteger': False,
 'nonnegative': False,
 'nonpositive': False,
 'nonzero': False,
 'odd': False,
 'positive': False,
 'prime': False,
 'rational': False,
 'real': False,
 'transcendental': False,
 'zero': False}
@smichr

This comment has been minimized.

Copy link
Member

smichr commented Mar 5, 2016

Thanks. Consider this:

>>> A, B = symbols("A B", commutative=False)
>>> A.is_zero
False

I guess that conclusion is fine since if we know it's not commutative and zero is commutative then the value cannot be zero. Here is the problem, though:

>>> (A - B).is_zero
False

That should not be since if B == A then the result which involved non-commutative values is zero (which is commutative).

@smichr

This comment has been minimized.

Copy link
Member

smichr commented Mar 5, 2016

The culprit is

>>> (A - B).is_commutative
False  <-- should be None
@smichr

This comment has been minimized.

Copy link
Member

smichr commented Mar 5, 2016

    # for performance reason, we don't let is_commutative go to assumptions,
    # and keep it right here
    __slots__ = ['is_commutative']

So is_commutative will never be None if there are noncommutative symbols in an expression, hence is_zero will always be False. That should be changed so that if the expression is noncommutative then None is returned for is_zero.

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Mar 5, 2016

So is_commutative will never be None if there are noncommutative symbols in an expression,

Perhaps this should be changed by calling nc_part.is_zero if nc_part is not None.

@smichr

This comment has been minimized.

Copy link
Member

smichr commented Mar 5, 2016

That's a good idea.

Another issue, Dummy(commutative=False).is_zero is always False. But that's wrong (isn't it?) since one might have an expression in which which symbols are non-commutative, but reserve the right to make one of them 0. So the assumption that something is noncommutative should not imply non-zero though the reverse is true: if it's zero, it's commutative.

Part of the problem is that is_commutative has two meanings: 1) has noncommutative symbols 2) is actually noncommutative. There is no way to give None for the 2nd meaning with the current arrangement as a symbol must be one or the other. From symbol.py:

        # be strict about commutativity: cannot be None
        is_commutative = fuzzy_bool(assumptions.get('commutative', True))
        if is_commutative is None:
            whose = '%s ' % obj.__name__ if obj else ''
            raise ValueError(
                '%scommutativity must be True or False.' % whose)

@smichr smichr changed the title MatrixSymbol thinks it is not zero Dummy(commutative=False).is_zero -> False Mar 5, 2016

@smichr smichr added the assumptions label Mar 5, 2016

@AnishShah

This comment has been minimized.

Copy link
Contributor

AnishShah commented Mar 6, 2016

@smichr working on this...
Just want to make sure that I understood correctly, what you want is:

>>> Dummy(commutative=False).is_zero
 < None >

This might also return None for MatrixSymbol(commutative=False).is_zero, as assumptions do not consider matrices while deducing from commutative=False.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Mar 7, 2016

This is tricky. The issue comes from confusion of different kinds of zero, the real/complex zero (which is_zero refers to) and a non-commutative "matrix" zero, which is what MatrixSymbol('x', n, n) - MatrixSymbol('x', n, n) should refer to (if we use matrices it's easy to see the difference because here the zero is an n x n matrix of zeros).

If we confuse the two zeros in the assumptions system, as we already do with our arithmetic, then the only way to be consistent is to disallow the sum of commutative symbols to be commutative ((A + B).is_commutative=None), which reduces the usefulness of that assumption greatly.

I suppose there's a secondary issue here, which is that MatrixSymbol.is_commutative=False really is wrong, because the matrix could be a matrix that commutes with every other matrix, like the zero matrix or the identity (let's just assume square n x n matrices for now). The way the assumptions generally work, x.is_something = False means it is always false for any possible value of x that the other assumptions allow.

In all, I see this is a strong argument that commutative should not be part of the assumptions system. It should just be a property, which just hints Mul and other functions that the order in products matters.

@smichr

This comment has been minimized.

Copy link
Member

smichr commented Mar 8, 2016

commutative should not be part of the assumptions system

+1 Basically, it already isn't part of it since the property is not deduced, it is assigned in operations.py. But it creeps in because the zero property looks at the commutative property.

Note, too, that MatrixExpr at line 65 (tried copying the line but it didn't work for me) assigns `is_ZeroMatrix to False when I think it should be None for the reasons stated by @asmeurer: it is a symbol and it might represent the zero matrix symbolically; it's not a literal zero matrix but it could be.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Mar 9, 2016

I think it is deduced, from this fact (and the other facts that imply zero -> complex).

I think ZeroMatrix is a class, and is_ZeroMatrix is the same as isinstance(ZeroMatrix).

@AnishShah

This comment has been minimized.

Copy link
Contributor

AnishShah commented Mar 9, 2016

MatrixSymbol.is_commutative=False really is wrong, because the matrix could be a matrix that commutes with every other matrix, like the zero matrix or the identity.

+1. is_commutative should be None for MatrixSymbols.

Also, I think, commutative for symbols works correctly except for the Add class. I had a look at code and the logic does not consider commutative property. Like @smichr said, subtraction of two equal noncommutative symbols can be zero. But, commutative is correct for Mul & Pow as each symbol is noncommutative which means each symbol is not zero (because zero is commutative).

So, I think, we have two issues here:

  1. commutative in Add class
  2. commutative in Matrix

What do you think? @asmeurer

@smichr

This comment has been minimized.

Copy link
Member

smichr commented Mar 9, 2016

Leave a comment

I think it is deduced

For symbols, perhaps as ~comm -> ~complex and 0->complex hence ~complex->~0. For operations the line I cited just sets the attribute and skips assumptions.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Mar 10, 2016

Maybe I'm missing something. operations.py sets is_commutative=False, and from that the assumptions set is_zero=False.

Also, I think, commutative for symbols works correctly except for the Add class. I had a look at code and the logic does not consider commutative property. Like @smichr said, subtraction of two equal noncommutative symbols can be zero. But, commutative is correct for Mul & Pow as each symbol is noncommutative which means each symbol is not zero (because zero is commutative).

@AnishShah but as I noted, this will make commutative useless. Any A + B can't be certain to be commutative by this rule because B could be -A. Also, it looks like you can't create Symbol('A', commutative=None), which leads me to suspect that quite a few things won't work for an expression where is_commutative=None (maybe they will end up doing the right thing anyway, though, since commutative=None ought to be treated the same as commutative=False, and bool(None) is False).

I still feel that commutative just doesn't belong in the assumptions. Mathematically, it's a sketchy property, since commutativity is technically a property of two objects (and their multiplication operation). I suppose you could say a single object is "commutative" if it commutes with everything, and noncommutative otherwise. The only value of the assumptions is that it gives us a nice framework to make deductions like (A=noncommutative, B=noncommutative) -> (A*B=noncommutative). But there aren't really any useful deductions based on other assumptions from commutative, as far as I can tell (other than commutative=False -> every number assumption=False, which we are seeing issues with here). But again the point of commutative isn't to do some deductive computations, it's just to signal algorithms to keep things in order.

skirpichev added a commit to skirpichev/diofant that referenced this issue Oct 4, 2016

XXX Add._eval_is_commutative made more conservative
In general, (A + B) is not noncommutative, even if both A and
B are noncommutative symbols.  E.g. if B = -A.

sympy/sympy#10728

skirpichev added a commit to skirpichev/diofant that referenced this issue Oct 16, 2016

Make Add._eval_is_commutative more conservative
In general, (A + B) is not noncommutative, even if both A and B are
noncommutative symbols.  E.g. if B = -A.

The test_issue_6001() was reverted to the original version, before of
the commit ffdb509.

Fixes sympy/sympy#10728

skirpichev added a commit to skirpichev/diofant that referenced this issue Nov 3, 2016

Make Add._eval_is_commutative more conservative
In general, (A + B) is not noncommutative, even if both A and B are
noncommutative symbols.  E.g. if B = -A.

The test_issue_6001() was reverted to the original version, before of
the commit ffdb509.

Fixes sympy/sympy#10728
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment