[ops ] guard shape for decimal and negative values #7014
Conversation
| @@ -50,6 +51,8 @@ function broadcastTo_<R extends Rank>( | |||
| throw new Error(`broadcastTo(): Invalid broadcast shape [${shape}].`); | |||
| } | |||
|
|
|||
| assertNonNegativeIntegerDimensions(shape); | |||
There was a problem hiding this comment.
We can avoid having to check this for values we get from convertToTensor if we add this check to every convertToTensor call. We already check dtype, so checking shape as well seems fine to me.
There was a problem hiding this comment.
Unfortunately, I don't think there's a similar fix for ops that make new tensors unless we make changes to the op function, and even then, it's probably not really possible since you don't know the order of the args.
There was a problem hiding this comment.
Reviewable status:
complete! 2 of 1 approvals obtained (waiting on @mattsoulanille)
tfjs-core/src/ops/broadcast_to.ts line 54 at r1 (raw file):
Previously, mattsoulanille (Matthew Soulanille) wrote…
Unfortunately, I don't think there's a similar fix for ops that make new tensors unless we make changes to the
opfunction, and even then, it's probably not really possible since you don't know the order of the args.
are you suggesting to add shape check to convertToTensor method? That method does not have shape value issue, since it is either from a existing tensor or from an array of array, the shape is derived.
There was a problem hiding this comment.
Reviewable status:
tfjs-core/src/ops/broadcast_to.ts line 54 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
are you suggesting to add shape check to
convertToTensormethod? That method does not have shape value issue, since it is either from a existing tensor or from an array of array, the shape is derived.
but looks like the previous line is doing the shape check for negative and integer. I will consolidate.
There was a problem hiding this comment.
Reviewable status:
tfjs-core/src/ops/broadcast_to.ts line 54 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
but looks like the previous line is doing the shape check for negative and integer. I will consolidate.
Oh. oops. That was the wrong line.
Here's where we check for dtype in convertToTensor when the input is a TensorLike and not a Tensor. I think we can add a shape check there as well
https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/tensor_util_env.ts#L108
There was a problem hiding this comment.
Reviewable status:
tfjs-core/src/ops/broadcast_to.ts line 54 at r1 (raw file):
Previously, mattsoulanille (Matthew Soulanille) wrote…
Oh. oops. That was the wrong line.
Here's where we check for dtype in convertToTensor when the input is a TensorLike and not a Tensor. I think we can add a shape check there as well
https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/tensor_util_env.ts#L108
Wait, actually, I misread the type of convertToTensor and thought it was using a TensorInfo. TensorLike should not need its shape checked for decimals or negatives.
fixed #7008 and #6987
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is