-
Notifications
You must be signed in to change notification settings - Fork 955
Add support for tensors with 0 in shape #1196
Conversation
Let me know if you have ideas for other ops to test (in addition to matmul, add and concat). Asking since as you can see, the fix sometimes is op-specific, like in the concat case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, you might want to check logic op for example two zero size tensors x > y, expandDim seems to be special for zero size tensors.
Reviewed 11 of 11 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, @tafsiri, and @pyu10055)
src/util.ts, line 265 at r1 (raw file):
} implicitIdx = i; } else if (shape[i] <= 0) {
shape[i] < 0 ?
src/ops/broadcast_util.ts, line 87 at r1 (raw file):
b = 1; } if (a === 1) {
does this method support broadcast [0,2], [2,2] => [0,2] ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 11 of 11 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @tafsiri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Added unit tests for expandDim, and tf.greater to verify it works with 0-sized tensors.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @dsmilkov and @tafsiri)
src/util.ts, line 265 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
shape[i] < 0 ?
Ah nice find, done. The first if fires for shape >= 0, so this if would never fire for 0, but good to be precise.
src/ops/broadcast_util.ts, line 87 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
does this method support broadcast [0,2], [2,2] => [0,2] ?
Yes, see the added unit tests in broadcast_util_test.ts
.
Fixes tensorflow/tfjs#567 (see the issue for details)
FEATURE
This change is