Conversation
The `reducing()` method didn't handle negative values for indices, and didn't reverse the value of the axis as we do elsewhere, so results were incorrect. Also, we now parse and save the value of `keep_dims`, though I can't find evidence that it does much of anything: test cases pass different values for it but none of them fail (even though we ignore it), and at least one reference implementation I see doesn't seem to do anything with it.
|
Gentle Review Ping |
| if (index < 0) { | ||
| index += in->rank(); | ||
| } | ||
| index = in->rank() - 1 - index; |
There was a problem hiding this comment.
This is pretty sketchy, having a mix of dimension indices using the Halide convention and TFlite convention is going to lead to bugs. In this case it's tricky because the dimension comes from a buffer. I wonder if we should just not allow reduction indices to be runtime variables, and make this a vector of indices at parse time?
There was a problem hiding this comment.
True, but unfortunately it exists elsewhere already too -- e.g., ReshapeOp allows for the new shape to be an arbitrary Tensor, and (AFAICT) there's no constraint that requires that the new-shape-tensor be constant at parse time (it could be dynamic in theory). Same story here. (I haven't actually found such a case in practice, so maybe it's de facto not legal.)
(But yeah, having the conventions be different is a big pain and the source of various bugs over time. I don't like mixing them either.)
2 participants
The
reducing()method didn't handle negative values for indices, and didn't reverse the value of the axis as we do elsewhere, so results were incorrect.Also, we now parse and save the value ofkeep_dims, though I can't find evidence that it does much of anything: test cases pass different values for it but none of them fail (even though we ignore it), and at least one reference implementation I see doesn't seem to do anything with it.