aesara icon indicating copy to clipboard operation
aesara copied to clipboard

Numpy-like helper functions fail with non-symbolic inputs

Open ricardoV94 opened this issue 2 years ago • 10 comments

Unlike most Ops, that convert inputs with as_tensor_variable, several of the helper functions in tensor.basic do not convert the inputs, leading to some failures when trying to access attributes that are only present in TensorVariables

import numpy as np
import aesara.tensor as at

at.transpose(np.eye(3))
AttributeError: 'numpy.ndarray' object has no attribute 'broadcastable'

ricardoV94 avatar Mar 29 '22 12:03 ricardoV94

Yeah, I think I noticed this deficiency for at.broadcast_to as well.

brandonwillard avatar Mar 30 '22 23:03 brandonwillard

Hello, can I work on this issue?

ltoniazzi avatar Jul 16 '22 21:07 ltoniazzi

Hello, can I work on this issue?

Yes

ricardoV94 avatar Jul 17 '22 07:07 ricardoV94

It seems that these issues (along with the same for at.flatten) are already fixed in the environment aesara-dev. Is there a duplicate issue? Should I just try spot other similar bugs?

ltoniazzi avatar Jul 20 '22 22:07 ltoniazzi

It seems that these issues (along with the same for at.flatten) are already fixed in the environment aesara-dev.

You mean they're fixed on upstream (i.e. aesara-devs) main, no?

Is there a duplicate issue? Should I just try spot other similar bugs?

Yeah, there are still a few helper functions in aesara.tensor.basic that aren't calling as_tensor_variable/as_tensor.

The basic idea is that any function that immediately uses a method/attribute from the TensorVariable interface on one of its arguments is implicitly assuming that its arguments are TensorVariables, but they might need to be converted first.

For example, aesara.tensor.basic.roll uses x.ndim, but, if x isn't a numpy.ndarray or TensorVariable, then that expression will fail. Converting x to a TensorVariable before that statement is evaluated will fix the problem. tile, inverse_permutation, and diag all appear to have the same problem.

Aside from those cases, it's possible that some Op.make_node methods don't perform TensorVariable conversions, and, since most of those helper functions call Op.make_node, if neither the helper function nor the Op performs the conversion, there will likely be an error somewhere down the road.

I can't think of any such helper function + Op combinations right now, but they could exist.

brandonwillard avatar Jul 21 '22 02:07 brandonwillard

Yes that's what I meant!, and thanks a lot for the hints!

ltoniazzi avatar Jul 21 '22 08:07 ltoniazzi

Two questions:

  1. Is it better to use _x = as_tensor_variable(x) or x = as_tensor_variable(x) to perform TensorVariable conversions?
  2. Can someone suggest a testing strategy or a suitable example to test this TensorVariable conversions? (Something simple but ugly could be to pass a list, for example at.roll([1,2,3], 2), as some functions like roll still run with a numpy array even though the TensorVariable conversion is not performed on the input x)

ltoniazzi avatar Aug 27 '22 22:08 ltoniazzi

  1. Is it better to use _x = as_tensor_variable(x) or x = as_tensor_variable(x) to perform TensorVariable conversions?

aesara.tensor.as_tensor_variable is the interface for TensorVariable creation from Python/NumPy values.

2. Can someone suggest a testing strategy or a suitable example to test this TensorVariable conversions? (Something simple but ugly could be to pass a list, for example at.roll([1,2,3], 2), as some functions like roll still run with a numpy array even though the TensorVariable conversion is not performed on the input x)

Most of the test for aesara.tensor.as_tensor_variable are here.

brandonwillard avatar Aug 27 '22 23:08 brandonwillard

Thanks, but for (1) I meant whether I should rename the input x to _x like here, or leave it as x like here.

ltoniazzi avatar Aug 27 '22 23:08 ltoniazzi

Thanks, but for (1) I meant whether I should rename the input x to _x like here, or leave it as x like here.

Oh, that's usually done for typing (i.e. Mypy) purposes, so, yeah, you can use that idiom.

brandonwillard avatar Aug 27 '22 23:08 brandonwillard