Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support bool inputs for MatrixBandPart #2269

Merged
merged 9 commits into from
Nov 16, 2023
Merged

Support bool inputs for MatrixBandPart #2269

merged 9 commits into from
Nov 16, 2023

Conversation

Gerstenberger
Copy link
Contributor

Fixes #2268

Please let me know if this is something you want to support.
Should there also be a test case?

Signed-off-by: Alexander Gerstenberger <agerstenberger@apptek.com>
Signed-off-by: Alexander Gerstenberger <agerstenberger@apptek.com>
Signed-off-by: Alexander Gerstenberger <agerstenberger@apptek.com>
Signed-off-by: Alexander Gerstenberger <agerstenberger@apptek.com>
Signed-off-by: Alexander Gerstenberger <agerstenberger@apptek.com>
Signed-off-by: Alexander Gerstenberger <agerstenberger@apptek.com>
Signed-off-by: Alexander Gerstenberger <agerstenberger@apptek.com>
Signed-off-by: Alexander Gerstenberger <agerstenberger@apptek.com>
@Gerstenberger
Copy link
Contributor Author

Added a test case

@@ -3926,6 +3926,15 @@ def func(input_x, low, high):
return tf.identity(res, name=_TFOUTPUT)
self._run_test_case(func, [_OUTPUT], {_INPUT: input_val, _INPUT1: low_val, _INPUT2: high_val})

@check_opset_min_version(11, "CumSum")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to add a limitation for CumSum op for this test?

Copy link
Contributor Author

@Gerstenberger Gerstenberger Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i copied the function from above and then made edits (the other test don't need this too then? check opset 7 would be sufficient for MatrixBandPart op?). I removed the limitation for the test now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on if ONNX op CumSum is required during the conversion of a tf op. If yes, then we need to limit min opset version to 11 because ONNX supports CumSum op since 11. By adding this, the test case will be ignored if the test suite is running with an ONNX opset lower than 11.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MatrixBandPart is a tf op. It doesn't have a requirement for ONNX opset version. The ONNX opset version requirement depends on all ONNX ops we use during the conversion of a TF op.

Signed-off-by: Alexander Gerstenberger <agerstenberger@apptek.com>
Copy link
Collaborator

@fatcat-z fatcat-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contributions!

@fatcat-z fatcat-z merged commit 278bf8a into onnx:main Nov 16, 2023
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MatrixBandPart op in tf2onnx does not support bool inputs
2 participants