Implement triangular #64
Conversation
src/lib/validate-input.js
Outdated
| if (i !== j) { | ||
| throw new Error('The input should be a 2-D tensor of [N, N] shape.'); | ||
| } | ||
| if (!Number.isInteger(diagonal) || diagonal >= i || diagonal <= -i) { |
There was a problem hiding this comment.
Spec doesn't give the restrict for this diagonal . Please correct me, if I understand wrongly. Thanks.
There was a problem hiding this comment.
Spec doesn't give the restrict for this diagonal .
Correct - no restriction here. If diagonal is beyond the dimensions of the output tensor, then the output will simply be fully zero or fully copied.
There was a problem hiding this comment.
Updated by removing this restriction. Please take another look, thanks.
BruceDai
force-pushed
the
add_triangular
branch
from
455471d
to
faa746b
Compare
| it('triangular fully zero upper=false diagonal=-4', function() { | ||
| testTriangular( | ||
| { | ||
| shape: [3, 3], |
There was a problem hiding this comment.
All the shapes are square. We should include at least one asymmetric case (different height and width) to ensure we don't have any bugs where we accidentally swap rows for columns (e.g. shape: [4, 3].)
There was a problem hiding this comment.
Thanks @fdwr .
Last I referred to https://en.wikipedia.org/wiki/Triangular_matrix:
In mathematics, a triangular matrix is a special kind of [square matrix](https://en.wikipedia.org/wiki/Square_matrix).
``
OK, I will update it without restricting square matrix.
There was a problem hiding this comment.
I've read other definitions like "An m×n matrix A is called upper triangular if all entries lying below the diagonal entries are zero" which imply m and n can be distinct, but it's certainly true that that square matrices (a subset of rectangular matrices) satisfy that too. ONNX doesn't care. Even Wolfram Alpha accepts the following fine MatrixForm[UpperTriangularize[{{1, 2, 3, 4}, {4, 5, 6, 7}, {7, 8, 9, 10}}]].
OK, I will update it without restricting square matrix.
👍
| ], | ||
| }, | ||
| { | ||
| shape: [3, 3], |
There was a problem hiding this comment.
We should also include one with a batch count other than 1.
🤨 It looks like a spec typo that I missed where it says triangular takes exactly 2D tensors, but instead it should say the input is at least 2D. You can see other ML libraries just treat each 2D subsection as a separate batch:
https://numpy.org/doc/stable/reference/generated/numpy.triu.html
https://pytorch.org/docs/stable/generated/torch.tril.html
https://onnx.ai/onnx/operators/onnx__Trilu.html
So shape [4,3,5,7] would apply the triangular masking to the last two dimensions. Let's create a WebNN issue...
There was a problem hiding this comment.
Let's create a WebNN issue...
Done, please see webmachinelearning/webnn#494, thanks.
|
@fdwr I've updated commit to support at least 2D tensor, please take another look, thanks. |
@fdwr @huningxin PTAL, thanks.