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.

Sign up for GitHub

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

Jump to bottom

Fix bug in TensorVariable.__rmatmul__ #465

Merged
merged 2 commits into from
Oct 4, 2023
Merged

Fix bug in TensorVariable.__rmatmul__ #465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2858e69
Fix TensorVariable __rmatmul__
ricardoV94 Oct 3, 2023
686725c
Add explicit check for failing ndarray.dot(TensorVariable)
ricardoV94 Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pytensor/tensor/variable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ def __matmul__(left, right):
return at.math.matmul(left, right)

def __rmatmul__(right, left):
return at.math.matmul(right, left)
return at.math.matmul(left, right)

def sum(self, axis=None, dtype=None, keepdims=False, acc_dtype=None):
"""See :func:`pytensor.tensor.math.sum`."""
Expand Down
17 changes: 12 additions & 5 deletions tests/tensor/test_variable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from pytensor.compile.mode import get_default_mode
from pytensor.graph.basic import Constant, equal_computations
from pytensor.tensor import get_vector_length
from pytensor.tensor.basic import as_tensor, constant
from pytensor.tensor.basic import constant
from pytensor.tensor.elemwise import DimShuffle
from pytensor.tensor.math import dot, eq, matmul
from pytensor.tensor.shape import Shape
Expand Down Expand Up @@ -75,18 +75,20 @@ def test_numpy_method(fct, value):
utt.assert_allclose(np.nan_to_num(f(value)), np.nan_to_num(fct(value)))


def test_infix_dot_method():
def test_dot_method():
X = dmatrix("X")
y = dvector("y")

res = X.dot(y)
exp_res = dot(X, y)
assert equal_computations([res], [exp_res])

# This doesn't work. Numpy calls TensorVariable.__rmul__ at some point and everything is messed up
X_val = np.arange(2 * 3).reshape((2, 3))
res = as_tensor(X_val).dot(y)
res = X_val.dot(y)
exp_res = dot(X_val, y)
assert equal_computations([res], [exp_res])
with pytest.raises(AssertionError):
assert equal_computations([res], [exp_res])


def test_infix_matmul_method():
Expand All @@ -98,10 +100,15 @@ def test_infix_matmul_method():
assert equal_computations([res], [exp_res])

X_val = np.arange(2 * 3).reshape((2, 3))
res = as_tensor(X_val) @ y
res = X_val @ y
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the test relevant to the reported bug

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was running test_dot_method, and it failed with an error that __dot__ was not implemented

Copy link
Member Author

@ricardoV94 ricardoV94 Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but if that's the only test that used it, it's not a reason to have the mysterious dunder method. I couldn't see it being used anywhere else nor could I see how to trigger it naturally

exp_res = matmul(X_val, y)
assert equal_computations([res], [exp_res])

y_val = np.arange(3)
res = X @ y_val
exp_res = matmul(X, y_val)
assert equal_computations([res], [exp_res])


def test_empty_list_indexing():
ynp = np.zeros((2, 2))[:, []]
Expand Down
Loading