Fix shape inference for Loop@v19 and v21 #164
Conversation
|
|
||
| for name, typ in zip(carried_names, carried_types): | ||
| output_types[name] = typ | ||
|
|
There was a problem hiding this comment.
Did you not want to check the output type of the body is compatible with the initial value as per #163 (comment)?
There was a problem hiding this comment.
Right now this PR just continues with the existing behavior of earlier opsets, which is arguably not good enough. @MatejUrbanQC How did you end up solving your issue? Would we break your solution if we were to impose stricter checks here?
There was a problem hiding this comment.
I'm not using loops in the end, so that's fine.
But I think this only make sense if you can explicitly mark dimensions that you want to change in the loop as None.
Let's say I want to use a function lambda x: concat([x, [0]], axis=0) in the loop.
If my input has shape (None,), then the shape inference passes, because the output is also (None,).
However, if my input has known shape, it will fail.
So it's not just very limiting, it's also inconsistent.
These issues should go away if you can say "This dimensions will be changed, treat it as None". If the number of dimensions will also change, you set the whole shape to None.
|
I'm marking this as "draft" since the current implementation is too limiting / inconsistent. |
|
Will be superseded by #198. |
Fixes #163
Checklist
CHANGELOG.rstentry