Repository: sympy/sympy
Pull request: Improved simplification for expressions containing sign()
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #19484

#### Brief description of what is fixed or changed
Simplification for `sign()` now woks due to addition of `sign._eval_rewrite_as_Abs()`.
Working examples:
```
>>> x = Symbol('x')
>>> e = x + sign(x + x**3)

>>> simplify(Abs(x + x**3) * e)
x**3 + x*Abs(x**3 + x) + x

>>> e = x**2 + sign(x**3 + 1)

>>> simplify(Abs(x**3 + 1) * e)
x**3 + x**2*Abs(x**3 + 1) + 1

>>> simplify(sign(x) * Abs(x))
x
```

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* simplify
    * Improved simplification for sign()

* functions
    * Improved condition checking in `piecewise_simplify()`
<!-- END RELEASE NOTES -->

Problem statement:
sympy cannot simplify sign(x) * abs(x)
```
>>> from sympy import Symbol, sign
>>> x = Symbol('x', real=True)
>>> x_new = abs(x)*sign(x)
>>> x_new
Abs(x)*sign(x)
>>> x_new.simplify()
Abs(x)*sign(x)
```

I would expect the result to be just `x`. gh-19277 may be related.

Hints from issue discussion:
The rewrite suggested in #19277 would be able to make this work:
```julia
In [19]: e = sign(x)*Abs(x)                                                                                                                    

In [20]: e                                                                                                                                     
Out[20]: │x│⋅sign(x)

In [21]: simplify(e)                                                                                                                           
Out[21]: │x│⋅sign(x)

In [22]: e2 = e.subs(sign(x), Piecewise((0, Eq(x, 0)), (x/Abs(x), True)))                                                                      

In [23]: e2                                                                                                                                    
Out[23]: 
⎛⎧ 0   for x = 0⎞    
⎜⎪              ⎟    
⎜⎨ x            ⎟⋅│x│
⎜⎪───  otherwise⎟    
⎝⎩│x│           ⎠    

In [24]: simplify(e2)                                                                                                                          
Out[24]: x
```
Although the equivalent with the Piecewise conditions inverted does not simplify:
```julia
In [25]: e2 = e.subs(sign(x), Piecewise((x/Abs(x), Ne(x, 0)), (0, True)))                                                                      

In [26]: e2                                                                                                                                    
Out[26]: 
⎛⎧ x            ⎞    
⎜⎪───  for x ≠ 0⎟    
⎜⎨│x│           ⎟⋅│x│
⎜⎪              ⎟    
⎝⎩ 0   otherwise⎠    

In [27]: simplify(e2)                                                                                                                          
Out[27]: 
⎧x  for x ≠ 0
⎨            
⎩0  otherwise
```
What's the mechanism for making simplify do this automatically?
First we need `sign.eval_rewrite_as_Abs` so that you can do `sign(x).rewrite(Abs)` (#19277).

Then we need simplify to do something like `if expr.has(sign): expr.rewrite(Abs)` to see if it helps with simplifying.

Ideally I would prefer the second form of the Piecewise. That doesn't work because `piecewise_fold` is unable to detect this case so `piecewise_fold` or `Piecewise._eval_simplify` should be improved to handle that case. Specifically when we have:
```julia
In [1]: Piecewise((f(x), Ne(x, 0)), (f(0), True))                                                                                              
Out[1]: 
⎧f(x)  for x ≠ 0
⎨               
⎩f(0)  otherwise
```
It should be possible to spot that for cases after the `Ne` condition we can substitute the opposite of that condition in to the first expression and if that gives the same result as the next case we can combine the cases giving the expression from the first case with the `Or` of the conditions e.g.:
```julia
In [4]: p = Piecewise((f(x), Ne(x, 0)), (f(0), True))                                                                                          

In [5]: p                                                                                                                                      
Out[5]: 
⎧f(x)  for x ≠ 0
⎨               
⎩f(0)  otherwise

In [6]: p.args[0]                                                                                                                              
Out[6]: (f(x), x ≠ 0)

In [7]: p.args[0][0]                                                                                                                           
Out[7]: f(x)

In [8]: p.args[0][0].subs(p.args[0][1].lhs, p.args[0][1].rhs) == p.args[1][0]                                                                  
Out[8]: True

In [11]: p2 = Piecewise((p.args[0][0], Or(p.args[0][1], p.args[1][1])))                                                                        

In [12]: p2                                                                                                                                    
Out[12]: f(x)
```
After adding `sign._eval_rewrite_as_Abs` and `if expr.has(sign): expr.rewrite(Abs)` in
` simplify()` the outputs are ;
```
>>> simplify(sign(x) * Abs(x))
x

>>> e = x**2 + sign(x**3 + 1)
>>> simplify(Abs(x**3 + 1) * e)
/     2 | 3    |            3
|    x *|x  + 1|       for x  = -1
<
| 3    2 | 3    |
\x  + x *|x  + 1| + 1   otherwise

>>> f = Function('f')
>>> e = x + sign(x + f(x)**3)
>>> simplify(Abs(x + x**3) * e)
/                 | 3    |                          3
|               x*|x  + x|                 for x + f (x) = 0
|
|                            |  / 2    \|
</  |     3   |        3   \ |x*\x  + 1/|
|\x*|x + f (x)| + x + f (x)/*|----------|      otherwise
|                            |     3    |
|                            |x + f (x) |
\

```
@oscarbenjamin should I add this to the existing PR #19487 
I'm not sure if those are actually simpler or not. In some parts of simplify things like this are tried and the transformation is only kept if it seems to make the result simpler so maybe the rewrite should be tested there.
Those outputs are not fully simplified, but the first one is definitely better.

All of those long outputs are of the form
```
/ x      for y = 0
<
\ x + y  otherwise
```
This looks like a bug in simplifying `Piecewise`
I think the problem here is that expressions like `x**3 + 1` don't satisfy the `_blessed()` condition https://github.com/sympy/sympy/blob/25fbcce5b1a4c7e3956e6062930f4a44ce95a632/sympy/functions/elementary/piecewise.py#L1243
as value of `_diff_wrt` is False for such expressions. 
See #7027.
@oscarbenjamin @eric-wieser Removing the `if _blessed()` condition works fine in this case and does not break any tests. Would it be fine to make such a change?
It's hard to discuss these changes without seeing them. You can push them to the PR or to a new PR and then ask about them.

Commit message: Impoved simplification for sign()

Simplify() for sign(x)*Abs(x) now works due to
sign._eval_rewrite_as_Abs().

Patch to review:
diff --git a/sympy/functions/elementary/piecewise.py b/sympy/functions/elementary/piecewise.py
index b7ccb6943ace..c423e8bb5219 100644
--- a/sympy/functions/elementary/piecewise.py
+++ b/sympy/functions/elementary/piecewise.py
@@ -1238,11 +1238,8 @@ def piecewise_simplify(expr, **kwargs):
             if eqs and not other:
                 eqs = list(ordered(eqs))
                 for e in eqs:
-                    # these blessed lhs objects behave like Symbols
-                    # and the rhs are simple replacements for the "symbols"
-                    if _blessed(e):
-                        _prevexpr = _prevexpr.subs(*e.args)
-                        _expr = _expr.subs(*e.args)
+                    _prevexpr = _prevexpr.subs(*e.args)
+                    _expr = _expr.subs(*e.args)
             # Did it evaluate to the same?
             if _prevexpr == _expr:
                 # Set the expression for the Not equal section to the same
diff --git a/sympy/simplify/simplify.py b/sympy/simplify/simplify.py
index b79aba30f508..0819a332d7b0 100644
--- a/sympy/simplify/simplify.py
+++ b/sympy/simplify/simplify.py
@@ -615,6 +615,10 @@ def done(e):
     from sympy.simplify.hyperexpand import hyperexpand
     from sympy.functions.special.bessel import BesselBase
     from sympy import Sum, Product, Integral
+    from sympy.functions.elementary.complexes import sign, Abs
+
+    if expr.has(sign):
+            expr = expr.rewrite(Abs)
 
     # Deal with Piecewise separately to avoid recursive growth of expressions
     if expr.has(Piecewise):


Code review comments:
diff_hunk: @@ -615,6 +615,10 @@ def done(e):
     from sympy.simplify.hyperexpand import hyperexpand
     from sympy.functions.special.bessel import BesselBase
     from sympy import Sum, Product, Integral
+    from sympy.functions.elementary.complexes import sign, Abs
+
+    if expr.has(sign):
path: sympy/simplify/simplify.py
text: @user1:
```suggestion
    # must come before `Piecewise` since this introduces more `Piecewise` terms
    if expr.has(sign):
```

diff_hunk: @@ -615,6 +615,10 @@ def done(e):
     from sympy.simplify.hyperexpand import hyperexpand
     from sympy.functions.special.bessel import BesselBase
     from sympy import Sum, Product, Integral
+    from sympy.functions.elementary.complexes import sign, Abs
+
+    if expr.has(sign):
+            expr = expr.rewrite(Abs)
path: sympy/simplify/simplify.py
text: @user1:
```suggestion
        expr = expr.rewrite(Abs)
```

diff_hunk: @@ -1238,11 +1238,8 @@ def piecewise_simplify(expr, **kwargs):
             if eqs and not other:
                 eqs = list(ordered(eqs))
                 for e in eqs:
-                    # these blessed lhs objects behave like Symbols
-                    # and the rhs are simple replacements for the "symbols"
-                    if _blessed(e):
path: sympy/functions/elementary/piecewise.py
text: @user1:
There must have been a reason for adding this check. Some substitutions aren't valid. I thought maybe that substituting for an ExprCondPair in a Piecewise would fail. Has that been changed?

@author:
> I thought maybe that substituting for an ExprCondPair in a Piecewise would fail. Has that been changed?
```
eq = Piecewise((x, Eq(y, 0)), (y, True))
print(eq.subs([(x, 2), (y, 1)]))
```
This does work.
> There must have been a reason for adding this check.

I understand this but the `_blessed()` condition checks on the basis of `_diff_wrt` which is False by default for all expressions. 

@author:
Can you give some examples which should not be valid ?

@user1:
There are lots of possible invalid substitutions e.g.:
```julia
In [1]: Piecewise((x, y<1), (z, True))                                                                                                         
Out[1]: 
⎧x  for y < 1
⎨            
⎩z  otherwise

In [2]: Piecewise((x, y<1), (z, True)).subs(y < 1, exp(2))                                                                                     
---------------------------------------------------------------------------
TypeError
```
Here we are substituting an `Expr` in place of a `Boolean` and `Piecewise` rightly complains.

I would investigate this by using git blame to find the commit/PR where this piece of code was introduced and who the author was or who merged it. The discussion in the PR might explain the reason. Also you could CC the author giving them a chance to explain.

@author:
The example that you showed does raise a TypeError for me as well. Also I found the commit and I get https://github.com/sympy/sympy/commit/9d3fa8b4d4963c059c139f3860b8cc1d611adbae
It appears there is something wrong with the commit/blame history as mentioned in the PR https://github.com/sympy/sympy/pull/18488 and issue https://github.com/sympy/sympy/issues/19181.

@user1:
It looks like that commit has ruined the history in this file

@user1:
You can find the blame from before that commit here:
https://github.com/sympy/sympy/blame/d5df2bd6a274db8b8d3a5cf2622d16dfb3f3d8a4/sympy/functions/elementary/piecewise.py

@user2:
For sake of simplicity it is attempting only straightforward subs that are in the form (s1, s2) or (s, ns) or (s, r) where s is a symbol-like entity and ns and r are number symbol and rational, respectively. It was added in #17259

@author:
As this does not work on instances of `Expr`, would removing the condition be fine?

@author:
@user2 It seems that in the PR you mentioned `piecewise_simplify()` was introduced and did the same work as `_eval_simplify()`. I think the condition specifically was modified/added in https://github.com/sympy/sympy/commit/5e34c1b1370d5996c10cb2e72841cbc752cb419f

@user2:
substitution of non-symbol-like entities is not as simple as for the others. What test fails if you leave this optimization in place? Is there an impact on speed?

@author:
None of the tests fail. I used timeit for measuring the execution time and ran it multiple times. The results I got are: 
With `_blessed()`: 0.3315324999999998 , 0.3113937
without : 0.35980409999999985 , 0.32984559999999874
Although the condition helps with speed I am not sure if it is significant enough.

@author:
@user2 @user1 Is it merge able now? 

@user2:
I would like to see the use of `_blessed` retained. It is used above and it is used here, likely, for the same reason. So without good reason to do otherwise I would like to keep it.

@author:
The condition prevents from any non-symbol like objects (like the ones mentioned in the issue) to be simplified. If removing it isn't recommended, I think I could be modified to include `Expr` objects as well. Do you think such a change would be fine?

@user2:
> like the ones mentioned in the issue

One of these expressions should be added as a test because nothing currently fails if this code is reverted back to using `_blessed`.

@author:
On the local branch same as #19487 where `_eval_rewrite_as_Abs()` is implemented and `_blessed` is used, the following are the results:
`Abs(x) * sign(x)`  gives `|𝑥|sign(𝑥)`  and

`simplify(Piecewise((x*Abs(x + f(x)**3), Eq(x + f(x)**3, 0)),(x*Abs(x + f(x)**3) + x + f(x)**3, True)))` gives
`Piecewise((x*Abs(x + f(x)**3), Eq(x + f(x)**3, 0)), (x*Abs(x + f(x)**3) + x + f(x)**3, True))`

On this branch these cases are simplified.

@user2:
I added the test that would fail if the `_blessed` was not removed. I wonder if a compromise would be to only do this `if len(args) == 2 or _blessed(e):`

@author:
I don't think `len(args) ==2` would work for a general case. I tried 
```
_blessed = lambda e: (getattr(e.lhs, '_diff_wrt', False) or isinstance(e.lhs, Expr)) and (
        getattr(e.rhs, '_diff_wrt', None) or
        isinstance(e.rhs, (Rational, NumberSymbol, Expr)))
```
Although it works but I think it is very similar to removing the condition.

@user2:
I wasn't thinking to modify `_blessed`, just to make the change roosed below.

@user2:
I applied the suggestion...let's see if that passes.

@author:
I think it would pass but `len(args) == 2` would work only for Piecewise containing 2 arguments. What about a general case for any number of arguments?