Fix for pretty printing of MatAdd symbols and numbers

[matthew-brett]

Apply suggestion from
#23552 (comment) and
add tests.

• printing
  • Fixed various bugs in pretty printing of bare symbols and numbers in MatAdd expressions.

[sympy-bot]

✅

Hi, I am the SymPy bot (v167). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• printing
  • Fixed various bugs in pretty printing of bare symbols and numbers in MatAdd expressions. (#23553 by @matthew-brett)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.11.

Click here to see the pull request description that was parsed.

Apply suggestion from
https://github.com/sympy/sympy/issues/23552#issuecomment-1140248756 and
add tests.  

<!-- BEGIN RELEASE NOTES -->
* printing
  * Fixed various bugs in pretty printing of bare symbols and numbers in `MatAdd` expressions.
<!-- END RELEASE NOTES -->

[ThePauliPrinciple]

Maybe something like this can work:

    def _print_MatAdd(self, expr):
        s = None
        for item in expr.args:
            pform = self._print(item)
            if s is None:
                s = pform     # First element
            else:
                #In case we have a MatAdd of a non-matrix and a matrix
                if hasattr(item, 'as_coeff_mmul'):
                    coeff = item.as_coeff_mmul()[0]
                else:
                    coeff = item.as_coeff_mul()[0]
                if S(coeff).could_extract_minus_sign():
                    s = prettyForm(*stringPict.next(s, ' '))
                    pform = self._print(item)
                else:
                    s = prettyForm(*stringPict.next(s, ' + '))
                s = prettyForm(*stringPict.next(s, pform))

        return s

[github-actions]

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [77f1d79c]       [b1cf21e1]
     <sympy-1.10.1^0>                 
+      97.2±0.3ms        178±0.5ms     1.84  sum.TimeSum.time_doit

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

[matthew-brett]

@ThePauliPrinciple - thanks. I've refactored the function and extended the tests quite a bit.

I do not understand why the code has to use the coefficients for as_coeff_mul etc, to determine if there is a minus sign attached the argument, but I'll take that on faith!

I have revised the release notes by editing the intial PR comment - so I think this one is good to go.

[ThePauliPrinciple]

why is this done?

[matthew-brett]

Only because I saw it lower down, and I didn't know for sure that it would be an indexable object. If it will, then can be omitted, of course.

[oscargus]

expr.args will always be an iterable, yes.

[matthew-brett]

Will it always be indexable? So I can do expr.args[0] and expr.args[1:]?

[oscargus]

args is always a list (or possibly a tuple, I do not recall).

It may be of length 1 though, e.g. MatAdd(A) will not evaluate to A (which is probably a deficiency, but then there is still the possibility that someone does MatAdd(A, evaluate=False)). Seems like MatAdd(evaluate=False) does evaluate to GenericZeroMatrix, so one can be sure that it is at least of length 1.

[matthew-brett]

Ah - length 1 is OK:

[ins] In [1]: a = [1]

[ins] In [2]: a[1:]
Out[2]: []

[oscargus]

It would be useful to see how these expressions print when evaluate is not False. We do not want to break the nice printing in those cases. So please add tests for that as well.

[matthew-brett]

@oscargus - thanks - I added tests for evaluated and not-evaluated - they give the same output.

Not to do with this PR directly, because this behavior has not changed, but is the following really the pprint output we want? I would read that as "A multiplied by -B".

In [4]: pprint(MatAdd(A, -B))
A -B

[matthew-brett]

I think the optional dependencies failure is not related. I think I have fixed the release notes now. Is there anything else I should do?

[oscargus]

I am a bit doubtful about this whole thing. It is not possible to add a MatrixSymbol with a Symbol (or a number) in SymPy, so why should it be support for printing an object that is directly created in an incorrect way? (The correct thing would be to check in MatAdd that the arguments are matrices or MatrixSymbol and bail out otherwise.)

This is also the reason why it prints badly. Doing the "correct" thing and subtract A and B will print in a slightly better way.

[matthew-brett]

Right - yes - I get the doubtful aspect. But the question is what to do about it. This fix seems to me better than what was there before. Before, and after, we have no problem forming the expression, but before, when you pprint - or when you call init_printing with any arguments at all, then the display causes this error. That seems like a bad situation. All this PR does is fix the pprint error. If anyone tries to evaluate this expression, they will get the expected error, which, at that point, will be easier to understand.

[oscargus]

Does pretty lead to the same error? In that case, I'd prefer to simply use that instead of fetching the print output.

[matthew-brett]

Yes, same error. Will push tests change to using pretty.

[oscargus]

If coeff is not already sympified that is a bug elsewhere. So S is not (should not be) required. Probably better to not have it as it will hide bugs.

[matthew-brett]

Unfortunately that does not work because:

[nav] In [10]: A = sympy.MatrixSymbol('A', 1, 1)

[ins] In [11]: B = sympy.MatrixSymbol('B', 1, 1)

[ins] In [12]: (A + B).args[1]
Out[12]: B

[ins] In [13]: (A + B).args[1].as_coeff_mmul()
Out[13]: (1, B)

Therefore:

[ins] In [15]: coeff = (A + B).args[1].as_coeff_mmul()[0]

[ins] In [16]: coeff
Out[16]: 1

[ins] In [17]: coeff.could_extract_minus_sign()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Input In [17], in <cell line: 1>()
----> 1 coeff.could_extract_minus_sign()

AttributeError: 'int' object has no attribute 'could_extract_minus_sign'

[oscargus]

Ahh, that is clearly a bug here:

sympy/sympy/matrices/expressions/matexpr.py

Lines 400 to 401 in 24afa46

	def as_coeff_mmul(self):
	return 1, MatMul(self)

as it should not return 1, but S.One

I'll open a PR for that and changing check default to True.

We'll see what remains of the original issue then, but even though this may not be merged(?), it was clearly worthwhile to identify two issues in the code!

[oscarbenjamin]

These things should not be in the test suite. This is an invalid use of MatAdd and it should be disallowed. This test deliberately asserts that MatAdd can be used in ways that break its internal invariants. This kind of thing just makes it hard in future to enforce or depend on those invariants in the codebase (at which point someone like me will come along and strip out all these tests as being invalid).

Whether or not this should raise an exception or emit a warning is a question that can reasonably be asked. We shouldn't add regression tests for invalid usage though.

Rather than trying to make this work can we please focus on how to better serve the usecase in a way that can reasonably be supported:

1. Maybe there could be a PrintAdd class whose sole purpose is for printing
2. Maybe we should make it easier somehow to use MatrixSymbol.
3. Something else?

I don't see why so much effort is being expended here on making it possible to do something in a bad way when there is already a perfectly good way: use MatrixSymbol.

[matthew-brett]

If I might say - I think the effort being expended is just because it's still not clear what the right thing to do is.

I see that these should in some sense not be allowed, but it is in fact possible to create these objects, and so you get a very obscure error from pprint.

I really have no skin in this game - I can solve my problem another way. It just seemed to me that the current error in pprint or pretty was probably not the best possible outcome - and that the problem needed to be fixed elsewhere - possibly to prevent these objects being created. But seemed out of scope.

I just put the tests in because I wanted to test I'd fixed the undesirable behavior. Why not just leave them in, then remove the ability to create these objects, and then remove the tests. But - as I say - totally up to y'all.

[oscarbenjamin]

I think the proper fix is for MatAdd to error when given non matrices as arguments. It used to. I don't know why that was changed. I'm not sure (given that at least one person is apparently using this) whether there should be a deprecation for this even though it was never something that was "supported".

[matthew-brett]

That's totally fine by me - as I say. This current fix is only for the case where you haven't done that yet...

[matthew-brett]

Just as a recap - the fix here is just to avoid the very confusing current situation, of an uninformative error from certain attempts to print these expressions, which are impossible to evaluate, but easy to generate. The potential for confusion may be best illustrated with:

[ins] In [1]: import sympy
[ins] In [2]: A = sympy.MatrixSymbol('A', 1, 1)
[ins] In [3]: b = sympy.Symbol('b')
[ins] In [4]: expr = sympy.MatAdd(A, b)
[ins] In [5]: display(expr)
b + A

[ins] In [6]: sympy.init_printing()
[ins] In [7]: display(expr)
---------------------------------------------------------------------------
... [snip] ...
AttributeError                            Traceback (most recent call last)
File ~/Library/Python/3.8/lib/python/site-packages/sympy/printing/pretty/pretty.py:876, in PrettyPrinter._print_MatAdd(self, expr)
    874     s = pform     # First element
    875 else:
--> 876     coeff = item.as_coeff_mmul()[0]
    877     if S(coeff).could_extract_minus_sign():
    878         s = prettyForm(*stringPict.next(s, ' '))

AttributeError: 'Symbol' object has no attribute 'as_coeff_mmul'

[oscarbenjamin]

My vote is for this:

diff --git a/sympy/matrices/expressions/matadd.py b/sympy/matrices/expressions/matadd.py
index 4f8c2b6..fd17ab9 100644
--- a/sympy/matrices/expressions/matadd.py
+++ b/sympy/matrices/expressions/matadd.py
@@ -34,7 +34,7 @@ class MatAdd(MatrixExpr, Add):
 
     identity = GenericZeroMatrix()
 
-    def __new__(cls, *args, evaluate=False, check=False, _sympify=True):
+    def __new__(cls, *args, evaluate=False, check=True, _sympify=True):
         if not args:
             return cls.identity

Then you get:

In [2]: import sympy
   ...: A = sympy.MatrixSymbol('A', 1, 1)
   ...: b = sympy.Symbol('b')
   ...: expr = sympy.MatAdd(A, b)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-57ac487b2ab3> in <module>
      2 A = sympy.MatrixSymbol('A', 1, 1)
      3 b = sympy.Symbol('b')
----> 4 expr = sympy.MatAdd(A, b)

~/current/sympy/sympy.git/sympy/matrices/expressions/matadd.py in __new__(cls, evaluate, check, _sympify, *args)
     50             if not any(isinstance(i, MatrixExpr) for i in args):
     51                 return Add.fromiter(args)
---> 52             validate(*args)
     53 
     54         if evaluate:

~/current/sympy/sympy.git/sympy/matrices/expressions/matadd.py in validate(*args)
    104 def validate(*args):
    105     if not all(arg.is_Matrix for arg in args):
--> 106         raise TypeError("Mix of Matrix and Scalar symbols")
    107 
    108     A = args[0]

TypeError: Mix of Matrix and Scalar symbols

We can of course debate the potential need for a warning to be emitted for a few releases rather than an error.

[matthew-brett]

We discussed this a bit above. What do you think should happen then for:

expr = sympy.MatAdd(A, b, check=False)
display(expr)

[oscarbenjamin]

What do you think should happen then for:

Garbage in, garbage out. The purpose of check=False is not to allow creating invalid expressions.

[matthew-brett]

Well - but I think that leaves us in the same place as we were at the beginning - is this really the best way to indicate that you have a garbage expression?

[ins] In [1]: import sympy
[ins] In [2]: A = sympy.MatrixSymbol('A', 1, 1)
[ins] In [3]: b = sympy.Symbol('b')
[ins] In [4]: expr = sympy.MatAdd(A, b, check=False)
[ins] In [5]: display(expr)
b + A

[ins] In [6]: sympy.init_printing()
[ins] In [7]: display(expr)
---------------------------------------------------------------------------
... [snip] ...
AttributeError                            Traceback (most recent call last)
File ~/Library/Python/3.8/lib/python/site-packages/sympy/printing/pretty/pretty.py:876, in PrettyPrinter._print_MatAdd(self, expr)
    874     s = pform     # First element
    875 else:
--> 876     coeff = item.as_coeff_mmul()[0]
    877     if S(coeff).could_extract_minus_sign():
    878         s = prettyForm(*stringPict.next(s, ' '))

AttributeError: 'Symbol' object has no attribute 'as_coeff_mmul'

I would argue no - that there is no benefit to this error specifically on printing - that is just makes the source of the error needlessly confusing.

Put another way - yes, by all means raise an error, but not here - where it appears sometimes, and sometimes not.

[oscarbenjamin]

We should make it clear that check=False is not designed to allow invalid input. Most likely the performance benefit of check=False doesn't really exist to maybe it doesn't really need to be supported at all. The problem right now is that check defaults to True so that invalid input is allowed implicitly.

[matthew-brett]

I feel I should give up at this point. Feel free to close the PR. I continue to think that this printing error is the wrong thing to do, and confusing, but if it seems like the right thing to do - that's fine - your call.

[oscarbenjamin]

I agree that the printing error is not ideal. It arises because of the earlier problem: creating an invalid expression. Ideally the error would be raised at the point of error when the expression is created. That's better for users because they see the error in the right place. It's also better for the library because we only need to have one place that ensures the validity of expressions (and can then rightfully presume that that validity holds elsewhere).

[oscargus]

With the changes in #23671 MatAdd(A, 1) will complain on creation (changing the default check to True) and MatAdd(A, 1, check=False) will give

AttributeError: 'One' object has no attribute 'as_coeff_mmul'

Actually, there is no use of check=False in the code, so one may be tempted to remove it... (Or, at least rename it to _check and use that internally, we can always support previous code by either allow passing it but not use it or just have a **kwargs argument at the end.)

[matthew-brett]

Well - I guess I'm just going to say it one more time - but is it the right thing to do, to have this error only come up for certain display options? Is this what y'all really want?

[ins] In [1]: import sympy
[ins] In [2]: A = sympy.MatrixSymbol('A', 1, 1)
[ins] In [3]: b = sympy.Symbol('b')
[ins] In [4]: expr = sympy.MatAdd(A, b, check=False)
[ins] In [5]: display(expr)
b + A

[ins] In [6]: sympy.init_printing()
[ins] In [7]: display(expr)
---------------------------------------------------------------------------
... [snip] ...
AttributeError                            Traceback (most recent call last)
File ~/Library/Python/3.8/lib/python/site-packages/sympy/printing/pretty/pretty.py:876, in PrettyPrinter._print_MatAdd(self, expr)
    874     s = pform     # First element
    875 else:
--> 876     coeff = item.as_coeff_mmul()[0]
    877     if S(coeff).could_extract_minus_sign():
    878         s = prettyForm(*stringPict.next(s, ' '))

AttributeError: 'One' object has no attribute 'as_coeff_mmul'

Again - I'd say no - that's very confusing. But your call.

[oscargus]

I agree that it is confusing. For reference, the LaTeX printer does not run into this problem, so I wonder what is different there? (I have not checked it carefully, but as far as I can tell it does not have a dedicated MatAdd printer.)

I'm inclined to say that your modified code (without the S in S(coeff) after #23553 is merged) is my proposed way to go, at least until check is possibly removed. And that the test is a simple smoke test, just doing pretty for one incorrect case with check=False and no checking of the output, with a comment that this is just to make sure that an exception is not raised.