Broken on Sympy 1.6.2

[asadalam]

Hi,

Only a small change will make your script compatible with python 3. Is there any particular reason to stick to python 2?

[andravin]

Patches welcome! Of course backwards compatibility is desirable, if possible.

[andravin]

.. but it works for me with Python 3.6.9.

[andravin]

Python3 support was added by pull request #20 last year. I will leave this issue open for now in case somebody can reproduce a specific issue with python3 support.

[asadalam]

I tried on python 3.8 and sympy 1.6.2 and it didn't work. The scaling of the matrices, for e.g., G = (A(a,alpha,r).T/f).T did not work. It had to be replaced with G = (A(a,alpha,r).T*(f**-1)).T

[asadalam]

I will have to check with other versions of Python.

[andravin]

Interesting. I have not used Python 3.8 yet. I am busy with other projects this week, but will be able to test a patch this weekend. Thanks for reporting!

[andravin]

I can reproduce the error with Python 3.6.9 by upgrading to Sympy 1.6.2.

[andravin]

This is fun ..

>>> wincnn.showCookToomFilter((0,1,-1), 2, 3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/andrew/develop/wincnn/wincnn.py", line 105, in showCookToomFilter
    AT,G,BT,f = cookToomFilter(a,n,r,fractionsIn)
  File "/home/andrew/develop/wincnn/wincnn.py", line 55, in cookToomFilter
    G = (A(a,alpha,r).T/f).T
  File "/home/andrew/.local/lib/python3.6/site-packages/sympy/core/decorators.py", line 127, in binary_op_wrapper
    return func(self, other)
  File "/home/andrew/.local/lib/python3.6/site-packages/sympy/matrices/common.py", line 2712, in __truediv__
    return self.__div__(other)
  File "/home/andrew/.local/lib/python3.6/site-packages/sympy/core/decorators.py", line 127, in binary_op_wrapper
    return func(self, other)
  File "/home/andrew/.local/lib/python3.6/site-packages/sympy/matrices/common.py", line 2452, in __div__
    return self * (self.one / other)
TypeError: unsupported operand type(s) for /: 'One' and 'MutableDenseMatrix'

That must be the same error you got. Let's apply your fix internally to sympy/matrices/common.py at line 2452:

    @call_highest_priority('__rdiv__')
    def __div__(self, other):
        return self * other**(-1)
        # return self * (self.one / other)

.. and try again ..

>>> wincnn.showCookToomFilter((0,1,-1), 2, 3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'wincnn' is not defined
>>> import wincnn
wincnn.showCookToomFilter((0,1,-1), 2, 3)
AT = 
⎡1  1  1   0⎤
⎢           ⎥
⎣0  1  -1  1⎦

G = 
⎡ 1    0     0 ⎤
⎢              ⎥
⎢1/2  1/2   1/2⎥
⎢              ⎥
⎢1/2  -1/2  1/2⎥
⎢              ⎥
⎣ 0    0     1 ⎦

BT = 
⎡1  0   -1  0⎤
⎢            ⎥
⎢0  1   1   0⎥
⎢            ⎥
⎢0  -1  1   0⎥
⎢            ⎥
⎣0  -1  0   1⎦

FIR filter: AT*((G*g)(BT*d)) =
⎡d[0]⋅g[0] + d[1]⋅g[1] + d[2]⋅g[2]⎤
⎢                                 ⎥
⎣d[1]⋅g[0] + d[2]⋅g[1] + d[3]⋅g[2]⎦

Then I noticed that all of the matrix division code was replaced in the Sympy master branch in the last few weeks. I suspect you found a bug in Sympy 1.6.2.

Still we should apply your fix since it is correct and works around the issue in Sympy.

[andravin]

@asadalam please let me know if the pull request works for you.

[asadalam]

I made a pull request but there is no change. The matrices are still being divided.

[andravin]

Hm... did you look at #26 ? It incorporate your suggested fix.

[asadalam]

Yeah, I saw that. Apologies, but I was not very well versed with pull requests, but now I was able to pull the changes of #26 and it works. Thanks.

[andravin]

Thanks for reporting this and coming up with the fix @asadalam

[asadalam]

You are welcome.