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

DimShuffle's C code does not correctly handle ndarray views #699

Closed
brandonwillard opened this issue Dec 14, 2021 · 1 comment · Fixed by #701
Closed

DimShuffle's C code does not correctly handle ndarray views #699

brandonwillard opened this issue Dec 14, 2021 · 1 comment · Fixed by #701
Labels
bug Something isn't working C-backend important

Comments

@brandonwillard
Copy link
Member

brandonwillard commented Dec 14, 2021

Here's a simple example:

import aesara.tensor as at


at.broadcast_to(0, (2, 3))[None].eval()
# array([[[  0, 122, -76],
#         [-88,  49,  86]]], dtype=int8)

Here's the same result, just computed more directly via the generated C thunk:

import numpy as np
import aesara


x_at = at.vector()
x_fn = aesara.function([x_at], x_at[None])

x_fn.fn.fgraph.toposort()
# [InplaceDimShuffle{x,0}(<TensorType(float64, vector)>),
#  DeepCopyOp(InplaceDimShuffle{x,0}.0)]

dimshuffle_thunk = x_fn.fn.thunks[0]

dimshuffle_thunk
# <function aesara.graph.op.COp.make_c_thunk.<locals>.rval()>

dimshuffle_thunk.inputs[0][0] = np.broadcast_to(0, (6,))
dimshuffle_thunk()
dimshuffle_thunk.outputs[0][0]
# array([[              0,               0, 140009315547216,
#                      33,               0,               0]])

np.shares_memory(dimshuffle_thunk.inputs[0][0], dimshuffle_thunk.outputs[0][0])
# True
@brandonwillard brandonwillard added bug Something isn't working important C-backend labels Dec 14, 2021
@brandonwillard
Copy link
Member Author

brandonwillard commented Dec 14, 2021

It looks like the C implementation attempts to change the dimensions and strides itself, and it probably makes some strong assumptions about them in the process. Since broadcasting changes that information, those assumptions are probably bad.

Instead, we should be able to use PyArray_Transpose and PyArray_Newshape in the same way the Python implementation does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C-backend important
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant