Improve QOL when reshaping array symbols #218
Conversation
| x_ptr->grow(state); | ||
| graph.propagate(state, graph.descendants(state, {x_ptr})); | ||
|
|
||
| THEN("c has the same shape as y and the same values") { |
There was a problem hiding this comment.
Is the shape check captured implicitly by the state check? Here and in commit/revert below
There was a problem hiding this comment.
copy and paste error for the message itself. The size is checked implicitly, and the shape does not vary.
| // Check if there are any -1s, and if not fallback to other input checking. | ||
| auto it = std::ranges::find(shape, -1); | ||
| if (it == shape.end()) return shape; | ||
|
|
There was a problem hiding this comment.
| // Ensure there is at most one -1 | |
| if (++it != shape.end()) { | |
| // Same error message as NumPy | |
| throw std::invalid_argument("can only specify one unknown dimension"); | |
| } |
There was a problem hiding this comment.
This actually gets checked by the ArrayOutputMixin because that doesn't allow any -1. So I am deferring the error to there. The error message is slightly less explicit, but IMO it's OK.
There was a problem hiding this comment.
I think it checks that it's not dynamic, i.e. the first index is not negative, but that won't catch the case of multiple -1s...
There was a problem hiding this comment.
Actually, I don't think it gets checked at all. Just returns DYNAMIC_SIZE if the first index is negative. So then the this->dynamic() check below would return true and throw an error if there are an even number of -1s in the shape, but not if there are an odd number
There was a problem hiding this comment.
Let me add a test case
There was a problem hiding this comment.
4442ba1 adds a test case, it does correctly throw.
There was a problem hiding this comment.
It's because it results in a negative or otherwise non-matching shape. So it fails on the check for consistency between Reshape's shape and its predecessor's shape.
There was a problem hiding this comment.
Ok, I found an edge case, ReshapeNode(&A, {12, -1, -1}. Will fix.
Adds flattening array, reshaping non-contiguous arrays, the
Copysymbol (see #16), and allows reshaped arrays to be used as indexers.