Add C++ StackNode #214
Add C++ StackNode #214
Conversation
| if isinstance(arrays, ArraySymbol): | ||
| return arrays |
There was a problem hiding this comment.
What if axis is not 0? I know that numpy handles the case that arrays is itself an array, but it seems like maybe we should disallow it (as we are doing currently). Or perhaps there's a way to have it work with StackNode?
There was a problem hiding this comment.
If the axis is not 0 then I guess that's a ValueError?
There was a problem hiding this comment.
Shouldn't support for non-0 axis be as simple as changing which index the 1 gets inserted to in the shape?
There was a problem hiding this comment.
I don't believe so, as the ordering of the elements in the buffer needs to change as well, e.g.
>>> np.stack(np.arange(10).reshape(2, 5), axis=1).flatten()
array([0, 5, 1, 6, 2, 7, 3, 8, 4, 9])There was a problem hiding this comment.
The allocation is handled by the strides change. So the same shape/strides trick we used from concatenate should work here I believe.
There was a problem hiding this comment.
#153 (comment) is what I mean
There was a problem hiding this comment.
Oh sorry, I thought you meant it was a special case of reshape. You're right. The same trick works, but it does seem somewhat difficult to write code that handles both cases of the source arrays being separate operands or different slices of the same (single) operand. Especially since the latter will require examining the diff() of the single operand, but then determining which slice each update belongs to.
Certainly doable but it seems like you'll need to write a separate case in at least propagate() to handle it. Unless I'm missing something and you have another genius indexing trick in mind 😆
There was a problem hiding this comment.
Hmm not sure I follow. What functionality are you suggesting we implement for input combination (ArraySymbol, non-zero axis)?
| // Since stack adds a new dimension of size equal to the number | ||
| // of inputs, we need to do this for the strided iterator to work | ||
| std::vector<ssize_t> shape(this->shape().begin(), this->shape().end()); | ||
| shape[this->axis_] = 1; |
There was a problem hiding this comment.
Smart! Perhaps worth saving this special shape on the class so you don't have to recreate it here and in initialize_state (with a comment explaining what it is). But current way is definitely fine as well.
tests/test_symbols.py
Outdated
| with self.subTest("input arrays must have same shape and ndim"): | ||
| model = Model() | ||
| A = model.constant(np.arange(4).reshape((2,2))) | ||
| B = model.constant(np.arange(4).reshape((2,1,2))) |
There was a problem hiding this comment.
Should have spaces between elements in a tuple, I would do a quick check for that in the Python code and in Python examples in the docstrings
There was a problem hiding this comment.
Ah thanks, I went over that test too quickly and forgot about the formatting
There was a problem hiding this comment.
Actually I see that the formatting is a bit off in several places. I'll do some work setting up a git hook so I don't forget the formatting all the time 😅
|
So I feel bad about this, but on reflection I think we should not add the C++ stack node. And do our stacking via Python with reshape and concatenate. We should add a Python |
|
Made an issue for the Python implementations: #240 |
Removed python implementation of stack which was using a combination of Reshape + Concatenate.
Python side now relies on C++ StackNode implementation.
Few updated tests, and new tests added for C++ and Python