Skip to content
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

times: elementwise multiplication broken on SymPy 1.9 #1109

Closed
cbm755 opened this issue Apr 9, 2022 · 7 comments · Fixed by #1146
Closed

times: elementwise multiplication broken on SymPy 1.9 #1109

cbm755 opened this issue Apr 9, 2022 · 7 comments · Fixed by #1146

Comments

@cbm755
Copy link
Collaborator

cbm755 commented Apr 9, 2022

Consider

 syms x
 A = [1 x];
 B = [2 3];
 A .* B

On SymPy 1.9, I get:

Python exception: AssertionError
    occurred at line 6 of the Python code block:
    return x.multiply_elementwise(y)

Looks like an upstream issue but its fixed in the later SymPy 1.10.x. Should bisect and ensure upstream has a unit test for this...

If so (rather than our fault), maybe we want to implement a blocklist or at leat a warnlist on "known bad" SymPy versions. E.g., in sympref diagnose.

@cbm755 cbm755 added the upstream label Apr 9, 2022
cbm755 added a commit that referenced this issue Apr 9, 2022
See Issue #1109.  This was covered by a doctest but its so basic we
should have it in the main test suite.
@chrisjohgorman
Copy link
Contributor

A git bisect leaves [67b59a8f591180fb1e5ecd02e793b929541327e1] multiply_elementwise error fixed as the commit which fixes the issue. Looking at the commit, I don't see a unit test added. Of course it's possible to have added the unit test in a different commit. Where would I go to add the unit test?

diff --git a/sympy/matrices/repmatrix.py b/sympy/matrices/repmatrix.py
index 2f0cdcd6ca..84cde91b1e 100644
--- a/sympy/matrices/repmatrix.py
+++ b/sympy/matrices/repmatrix.py
@@ -239,8 +239,9 @@ def _eval_matrix_mul(self, other):
         return classof(self, other)._fromrep(self._rep * other._rep)
 
     def _eval_matrix_mul_elementwise(self, other):
-        rep = self._rep.mul_elementwise(other._rep)
-        return classof(self, other)._fromrep(rep)
+        selfrep, otherrep = self._rep.unify(other._rep)
+        newrep = selfrep.mul_elementwise(otherrep)
+        return classof(self, other)._fromrep(newrep)
 
     def _eval_scalar_mul(self, other):
         rep, other = self._unify_element_sympy(self._rep, other)

@chrisjohgorman
Copy link
Contributor

I looked around at sympy and found that pull request from the author also included a commit to the sympy/matrices/tests/test_commonmatrix.py. It mentions the issue #22353. I decided to run this test case, by hand in sympy-1.9 where it fails. It now runs properly in 1.10.

@cbm755
Copy link
Collaborator Author

cbm755 commented Jun 12, 2022

Anything we should do here for 3.0.0? We could have a known_bad list of SymPy versions, maintained in sympref and checked similar to how we check the minimum sympy version... Or just ignore it?

@cbm755 cbm755 added this to the 3.0.0 milestone Jun 12, 2022
@alexvong243f
Copy link
Collaborator

Actually, for this particular issue, we can workaround it by using hadamard_product(x, y) instead of x.multiply_elementwise(y) for sympy 1.9 because the bug for hadamard_product sympy/sympy#8557 has been fixed since Mar 2020 (sympy 1.9 was released on Oct 2021).

@cbm755
Copy link
Collaborator Author

cbm755 commented Jun 18, 2022

So could we keep using hadamard_product when SymPy >= 1.9? That sounds like a nice fix to me!

@alexvong243f
Copy link
Collaborator

Yes, hadamard_product works fine and passes all tests in sympy 1.9. Should we use hadamard_product instead of multiply_elementwise in sympy >1.9 as well?

@cbm755
Copy link
Collaborator Author

cbm755 commented Jun 19, 2022

Sure, try it? If tests pass, it should be good. Semantically correct is good!

alexvong243f pushed a commit to alexvong243f/octsympy that referenced this issue Jun 22, 2022
Closes gnu-octave#1109.

* inst/@sym/rdivide.m: Fix it.
* inst/@sym/times.m: Fix it.
alexvong243f pushed a commit to alexvong243f/octsympy that referenced this issue Jun 22, 2022
Otherwise, we use "hadamard_product".

Closes gnu-octave#1109.

* inst/@sym/rdivide.m: Fix it.
* inst/@sym/times.m: Fix it.
cbm755 added a commit that referenced this issue Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants