Implement vectorized adstock transformations

[lucianopaz]

Closes #22

• Convert the old adstock transformations to use the vectorized convolution
• Write tests for the vectorized convolution with multiple dimensions and broadcasting patterns
• Maybe add an assert to prevent segfaults due to passing a incorrect axis?

[lucianopaz]

@ricardoV94, is there another way to implement this shape broadcasting?

• If I’m not explicit about it, broadcasting of symbolic x and w fails and takes the shape of x as the correct one (maybe symbolic isn’t the correct term, I mean tensors that have None in their shape).
• When I am explicit but the arrays fail to broadcast at run time, I get segfaults (probably due to the C backend trying to read or write to memory locations that aren’t part of the array)

[lucianopaz]

From a closer look at the source I think that there should be a couple of assert Ops there.

[drbenvincent]

Could I recommend:
a) placing an assert at the start of the function to ensure sizes of the data and parameters are aligned EDIT: I see that that's already on the todo list.
b) is it worth doing some timing tests, even if informal. I'm finding that inference is slowing down significantly when going from [4, T] sized inputs (ie 4 time series) to [4, 150, T] (ie 4 time series each of which has 150 geographical regions.

[drbenvincent]

Feature request: At the moment there's the assumption that if our data is shape [4, 150, T], then we have parameters of shape [4, 150]. This makes sense in the case that we want independent parameters for each predictor*region combination, whether we place a hierarchy on that or not. But, this is likely to significantly slow down inference.

Would it be possible to have the option (or an alternative function) which allows you to just have different priors over predictors which is applied over all regions. So in this example, that would be data of size [4, 150, T], but parameters of size [4]

[lucianopaz]

Feature request: At the moment there's the assumption that if our data is shape [4, 150, T], then we have parameters of shape [4, 150]. This makes sense in the case that we want independent parameters for each predictor*region combination, whether we place a hierarchy on that or not. But, this is likely to significantly slow down inference.

Would it be possible to have the option (or an alternative function) which allows you to just have different priors over predictors which is applied over all regions. So in this example, that would be data of size [4, 150, T], but parameters of size [4]

This is already supported, just pass the parameters with shape (4, 1) and the same parameter value will be used for every entry in the second axis of x. The logic behind this is that the T dimension of x ids ignored, and the rest of the axis are broadcasted following the same rules that numpy does. That’s why you got a failure to broadcast when the parameter had shape (4,), because 4 and 150 don’t broadcast together

[lucianopaz]

@ricardoV94, I'd like to ask for your input on the params_broadcast_shapes function that I added here. As I mentioned in some older comments, the pytensor version is not robust to broadcasting failures (shapes that cannot broadcast together are taken to broadcast just fine and can cause segfaults down the line). This version might be suitable for pytensor itself, if you think that it's OK. My main question is whether the approach that I followed for getting "concrete shapes" (shapes that are not TensorVariables themselves) is fine, or if there is something that I might have missed.

[lucianopaz]

I opened pymc-devs/pytensor#152, related to the problem I found with params_broadcast_shapes. If the fix from this PR looks ok, I can also apply it over on pytensor

[lucianopaz]

Pinging @ricardoV94 to have a look. I updated the params_shape_broadcast to rely on the tensors broadcastable attribute. If this isn't desirable, I can undo the last commit.

[ricardoV94]

@lucianopaz sorry for the delay. I think pymc-devs/pytensor#175 is a better solution

[lucianopaz]

@lucianopaz sorry for the delay. I think pymc-devs/pytensor#175 is a better solution

I don’t agree. I think that PR addresses a separate and valid issue: broadcast_to should check that the tensor can in fact broadcast to the requested shape. This PR actually enforces a similar check when computing the output shape of each tensor in params_shape_broadcast. It also enforces te broadcastable flag, which is ignored in the pytensor version.

[ricardoV94]

This seems like an odd place to reimplement a utility like this. Right now the broadcastable flag is ignored by design all over Pytensor. Wouldn't that be the place to start fixing things?

[juanitorduz]

Hey! Any blockers for this one? Anything I could do to support ?

[ricardoV94]

Would be good to see if we can remove the custom params_broadcast_shapes and use the one in PyTensor, now that we fixed BroadcastTo

[ricardoV94]

Closed in favor of #221