Repository: sympy/sympy
Pull request: Fix bug in cancel function
<!-- 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://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #17843

#### Brief description of what is fixed or changed
Fix bug in cancel function

#### 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
   *  Fixing bug in cancel function
<!-- END RELEASE NOTES -->


Problem statement:
Noninteger coefficients in cancel function
Documentation says that in output of 'cancel' function "leading coefficients of numerator and denominator are integers" which isn't always true. For example
`>>> cancel('(x/2+1)/y')`
returns
`(x + 2)/(2*y)` 
which is fine, but
`>>> cancel('(x/2+1)')`
returns
`x/2 + 1`
and I would expect 
`(x+2)/2`

Hints from issue discussion:
@oscarbenjamin Can I take up this issue? And can you please give some pointers on how I should proceed to do that? Because I haven't contributed to sympy before. 
I'm not sure what `cancel` is supposed to do in this situation. The docstring says
```
Help on function cancel in module sympy.polys.polytools:

cancel(f, *gens, **args)
    Cancel common factors in a rational function ``f``.
    
    Examples
    ========
    
    >>> from sympy import cancel, sqrt, Symbol
    >>> from sympy.abc import x
    >>> A = Symbol('A', commutative=False)
    
    >>> cancel((2*x**2 - 2)/(x**2 - 2*x + 1))
    (2*x + 2)/(x - 1)
    >>> cancel((sqrt(3) + sqrt(15)*A)/(sqrt(2) + sqrt(10)*A))
    sqrt(6)/2
```
That suggests that `cancel` is intended to be used with a "rational function" as input but the example here `x/2 + 1` is not a rational function.

The tutorial though says something different:
https://docs.sympy.org/latest/tutorial/simplification.html#cancel
```
cancel() will take any rational function and put it into the standard canonical form, 𝑝/𝑞,
where 𝑝 and 𝑞 are expanded polynomials with no common factors, and the leading
coefficients of 𝑝 and 𝑞 do not have denominators (i.e., are integers).
```

The tutorial suggests factor can also be used for this
```
In [15]: f = x/2 + 1                                                                                                                                          

In [16]: f                                                                                                                                                    
Out[16]: 
x    
─ + 1
2    

In [17]: factor(f)                                                                                                                                            
Out[17]: 
x + 2
─────
  2 
```

That is a rational function and giving that to cancel gives:
```
In [18]: cancel(_)                                                                                                                                            
Out[18]: 
x    
─ + 1
2
```

I suspect that where this comes from is
```
In [20]: (x+2)*S.Half                                                                                                                                         
Out[20]: 
x    
─ + 1
2    

In [21]: (x+2)/2                                                                                                                                              
Out[21]: 
x    
─ + 1
2 
```
So the expression that you want here auto-simplifies. I guess factor must be creating this as
```
In [24]: Mul((x+2), S.Half, evaluate=False)                                                                                                                   
Out[24]: 
x + 2
─────
  2 
```

The auto-simplification won't happen for a non-rational:
```
In [27]: cancel(x/y + 1)                                                                                                                                      
Out[27]: 
x + y
─────
  y 
```

A momentary fix would be using `evaluate=False` in cancel but it won't last when any other operation occurs:
```
In [29]: Mul((x+2), S.Half, evaluate=False)                                                                                                                   
Out[29]: 
x + 2
─────
  2  

In [30]: _*3                                                                                                                                                  
Out[30]: 
3⋅x    
─── + 3
 2 
```

Essentially the problem here is that a 2-arg Mul distributes Rationals (this is a long-standing issue that is hard to fix).

You could try using `evaluate=False` at the appropriate place at the end of `cancel`.
Possibly it is best to have the denominator first in the Mul:
```
In [54]: Mul(S.Half, (x+2), evaluate=False) * 3                                                                                                               
Out[54]: 
3⋅(x + 2)
─────────
    2

In [55]: Mul((x+2), S.Half, evaluate=False) * 3                                                                                                               
Out[55]: 
3⋅x    
─── + 3
 2  
```
I'm not sure if that should be considered a bug as well...
Thank you so much for explaining it to me, @oscarbenjamin 

> "A momentary fix would be using evaluate=False "
    
 Okay, I'll try that

The docstring says that  `cancel(f, *gens, **args)'   --> Cancel common factors in a rational function ``f``.
And if we don't pass a rational function as an argument it should throw an error or something, no??

Actually the function is a rational function. It isn't in the form of a ratio but it can be put into that form.
Alright! 
I guess it should be:
```
return Mul(c, 1/Q.as_expr(), P.as_expr(), evaluate=False)
```
Or perhaps
```
return Mul(c, P.as_expr()/Q.as_expr(), evaluate=False)
```
Why don't you give it a try and run the tests to see whether any tests fail?
I tried changing ` c*(P.as_expr()/Q.as_expr())`  to `Mul(c, P.as_expr()/Q.as_expr(), evaluate=False)`
and only one test failed out of 145. Unfortunately, I couldn't figure out how to fix (AssertionError) that test

https://github.com/sympy/sympy/blob/master/sympy/polys/tests/test_polytools.py#L2866
If you run that code yourself what output do you get on master and what output do you get with this PR?

Also does it fix the issue?

Commit message: Update polytools.py

Patch to review:
diff --git a/sympy/polys/polytools.py b/sympy/polys/polytools.py
index ec7e536e9bdf..97759d70d951 100644
--- a/sympy/polys/polytools.py
+++ b/sympy/polys/polytools.py
@@ -6651,7 +6651,7 @@ def cancel(f, *gens, **args):
     c, P, Q = F.cancel(G)
 
     if not isinstance(f, (tuple, Tuple)):
-        return c*(P.as_expr()/Q.as_expr())
+        return c * Mul(1/Q.as_expr(), P.as_expr(), evaluate=False)
     else:
         if not opt.polys:
             return c, P.as_expr(), Q.as_expr()


Code review comments:
diff_hunk: @@ -6651,7 +6651,7 @@ def cancel(f, *gens, **args):
     c, P, Q = F.cancel(G)
 
     if not isinstance(f, (tuple, Tuple)):
-        return c*(P.as_expr()/Q.as_expr())
+        return c * Mul(1/Q.as_expr(), P.as_expr(), evaluate=False)
path: sympy/polys/polytools.py
text: @user1:
```suggestion
        return Mul(c, P.as_expr()/Q.as_expr(), evaluate=False)
```

@author:
I tried, didn't work out

@author:
https://github.com/sympy/sympy/issues/17843#issuecomment-568475591

@user1:
The one test that failed will just require modifying the expected result to be `Mul((-S.Half, x + sqrt(2)*x - 2), evaluate=False)`