add pnp domain with one op: LinalgSVD #5821
Conversation
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [0.618057, -0.412802, -0.841065, -2.316042] | ||
| ] | ||
| ]) | ||
| U, S, Vh = np.linalg.svd(A, full_matrices=True) |
Check warning
Code scanning / lintrunner
EDITORCONFIG-CHECKER/editorconfig Warning
| [0.618057, -0.412802, -0.841065, -2.316042] | ||
| ] | ||
| ]) | ||
| U, S, Vh = np.linalg.svd(A, full_matrices=True) |
Check warning
Code scanning / lintrunner
RUFF/W291 Warning
| @@ -0,0 +1,37 @@ | |||
| # Copyright (c) ONNX Project Contributors | |||
Check warning
Code scanning / lintrunner
RUFF/format Warning
| @@ -0,0 +1,37 @@ | |||
| # Copyright (c) ONNX Project Contributors | |||
Check warning
Code scanning / lintrunner
BLACK-ISORT/format Warning
| @@ -0,0 +1,75 @@ | |||
| /* | |||
Check warning
Code scanning / lintrunner
CLANGFORMAT/format Warning
| @@ -0,0 +1,75 @@ | |||
| /* | |||
Check warning
Code scanning / lintrunner
EDITORCONFIG-CHECKER/editorconfig Warning
| @@ -74,6 +74,7 @@ | |||
| ("1.14.0", 9, 19, 3, 1), | |||
| ("1.14.1", 9, 19, 3, 1), | |||
| ("1.15.0", 9, 20, 4, 1), | |||
| ("1.16.0", 9, 20, 4, 1, 1), | |||
Check failure
Code scanning / lintrunner
MYPY/list-item Error
There was a problem hiding this comment.
Need to update VersionRowType in line 47
|
|
||
| ONNX_PNP_OPERATOR_SET_SCHEMA(LinalgSVD, 1, | ||
| OpSchema() | ||
| .SetDoc(R"DOC(For internal use.)DOC") |
There was a problem hiding this comment.
That would be great to have a longer description. onnx runtimes do not need to declare an operator in onnx to implement a kernel. Is it needed for onnx?
There was a problem hiding this comment.
+1. I don't understand what "internal use" means here.
| @@ -25,6 +26,7 @@ | |||
| ONNX_DOMAIN = "" | |||
| ONNX_ML_DOMAIN = "ai.onnx.ml" | |||
| AI_ONNX_PREVIEW_TRAINING_DOMAIN = "ai.onnx.preview.training" | |||
| AI_ONNX_PNP_DOMAIN = "ai.onnx.pnp" | |||
There was a problem hiding this comment.
Just thinking aloud: I wonder if "ai.onnx.linalg" would be better?
There was a problem hiding this comment.
I would think ai.onnx.linalg is a clearer domain name
|
|
||
| namespace ONNX_NAMESPACE { | ||
|
|
||
| ONNX_PNP_OPERATOR_SET_SCHEMA(LinalgSVD, 1, |
There was a problem hiding this comment.
We could just call it "SVD". Especially if we change the domain to "ai.onnx.linalg" as discussed elsewhere.
| ONNX_NAMESPACE::propagateElemTypeFromInputToOutput(ctx, 0, 2); | ||
| int64_t full_matrices = ctx.getAttribute("full_matrices")->i(); | ||
|
|
||
| const TensorShapeProto& A_shape = ctx.getInputType(0)->tensor_type().shape(); |
There was a problem hiding this comment.
These lines should be protected by the usual hasNInputShape or hasInputShape.
| auto* u_shape = ctx.getOutputType(0)->mutable_tensor_type()->mutable_shape(); | ||
| auto* s_shape = ctx.getOutputType(1)->mutable_tensor_type()->mutable_shape(); | ||
| auto* v_shape = ctx.getOutputType(2)->mutable_tensor_type()->mutable_shape(); | ||
| if (A_shape.dim_size() == 3) { |
There was a problem hiding this comment.
These shape aspects should be explained in the operator documentation: I guess the input is required to be either 2-dim or 3-dim?
| "LinalgSVD", | ||
| inputs=["A"], | ||
| outputs=["U", "S", "Vh"], | ||
| threshold=threshold, |
|
What about the reference implementation? I guess since numpy has one, it should be easy to add? |
|
Please also take a look at this PR #4416 for various aspects mentioned there, in case it is useful |
|
Hi can anyone please check in this branch to the Onnx main branch so it can be used with the main build |
Description
It is needed to add data pre/post-processing (pnp) ops to ONNX. These ops mostly come from pre-processing SIG. They usually do not exist in a neural network but are still required in order for a runtime to execute in an end-to-end scenario in production.
This PR is to add infrastructure support for the pnp domain. Followings are still needed to be solved:
Motivation and Context