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

Create IsNaN-20 and IsInf-20 #5583

Merged
merged 19 commits into from
Sep 21, 2023
Merged

Conversation

justinchuby
Copy link
Contributor

@justinchuby justinchuby commented Sep 12, 2023

Description

Create IsNaN-20 and IsInf-20 to include all floating point types. Note that even though float8 types like e4m3fnuz do not have representation for infinity, we still define the operator on these types so that the operators can be conveniently applied on all floating point types.

Checklist

  • Update the opset in defs.cc
  • Move the old definition of old.cc
  • Add the operator to onnx/defs/operator_sets.h
  • Create node tests
  • Disable the tests in test_backend_onnxruntime.py
  • Create reference implementation
  • Update version converter
  • Create shape inference and checker tests
  • Build docs

Motivation and Context

Fixes #5260

@justinchuby justinchuby requested a review from a team as a code owner September 12, 2023 00:22
@justinchuby justinchuby added operator Issues related to ONNX operators auto update doc Generate md/proto files automatically using the CI pipeline labels Sep 12, 2023
@justinchuby justinchuby added this to the 1.15 milestone Sep 12, 2023
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@xadupre
Copy link
Contributor

xadupre commented Sep 12, 2023

About isinf, e4m3fnuz has no infinite value. Should we exclude it or return false ? Same goes with e5m2fnuz.

@justinchuby
Copy link
Contributor Author

About isinf, e4m3fnuz has no infinite value. Should we exclude it or return false ? Same goes with e5m2fnuz.

I think we should return False. Do we know how other frameworks handle them?

@xadupre
Copy link
Contributor

xadupre commented Sep 12, 2023

Only e5m2 has infinite values and CUDA uses this type to compute the gradient. The only operator available is Gemm so there is no division involved and the default behaviour is to saturate the value to the maximum. I assume nan cannot appear with this only operation.

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby justinchuby requested a review from a team as a code owner September 12, 2023 16:52
justinchuby and others added 4 commits September 12, 2023 17:04
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@justinchuby justinchuby reopened this Sep 12, 2023
@gramalingam
Copy link
Contributor

I agree that for the float-8 variants that do not have an infinity value, IsInf should always return false. (While this is a redundant op for this type, it may still be helpful in defining functions that work generically for all floating-types.)

@justinchuby justinchuby added the review needed: operators approvers Require reviews from members of operators-approvers label Sep 12, 2023
@justinchuby justinchuby removed the review needed: operators approvers Require reviews from members of operators-approvers label Sep 19, 2023
@justinchuby justinchuby added this pull request to the merge queue Sep 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2023
@justinchuby
Copy link
Contributor Author

justinchuby commented Sep 19, 2023

I may need to exclude test_isinf_float16_cpu (but it was added already?)

@liqunfu
Copy link
Contributor

liqunfu commented Sep 19, 2023

I may need to exclude test_isinf_float16_cpu

Why other tests for new and updated have been passing? And why it was passing before?

@justinchuby
Copy link
Contributor Author

justinchuby commented Sep 19, 2023

Why other tests for new and updated have been passing? And why it was passing before?

It should have been skipped (and should pass) already. I am not very sure.

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@justinchuby
Copy link
Contributor Author

Everything now fails though @liqunfu @jcwchen Should opset20 tests be auto skipped or do I need to manually add skips?

@jcwchen
Copy link
Member

jcwchen commented Sep 20, 2023

Everything now fails though @liqunfu @jcwchen Should opset20 tests be auto skipped or do I need to manually add skips?

Which test failure are you referring to? The latest commit seems to complain the node backend data is inconsistent. I would suggest you recreate those node backend data from scratch (manually running command) again.

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@justinchuby
Copy link
Contributor Author

@jcwchen Looks like the new test is not skipped automatically? https://dev.azure.com/onnx-pipelines/onnx/_build/results?buildId=52121&view=logs&j=825fcbdb-febe-56c2-0b31-e8b200b321eb&t=46007089-1c35-5679-e243-e8ae35eaeec6&l=656

Copy link
Member

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

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

@jcwchen Looks like the new test is not skipped automatically? https://dev.azure.com/onnx-pipelines/onnx/_build/results?buildId=52121&view=logs&j=825fcbdb-febe-56c2-0b31-e8b200b321eb&t=46007089-1c35-5679-e243-e8ae35eaeec6&l=656

I thought https://github.com/onnx/onnx/blob/main/onnx/test/test_backend_onnxruntime.py should be covered by ORT_MAX_ONNX_OPSET_SUPPORTED_VERSION, but it seems it doesn't and we need to manually filter new versions as https://github.com/onnx/onnx/blob/main/onnx/test/test_backend_onnxruntime.py#L234-L254. It might make sense that we enable ORT_MAX_ONNX_OPSET_SUPPORTED_VERSION for test_backend_onnxruntime.py as well to save some effort. cc @xadupre for inputs. Thanks!

Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby justinchuby added this pull request to the merge queue Sep 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 21, 2023
@justinchuby justinchuby added this pull request to the merge queue Sep 21, 2023
Merged via the queue into onnx:main with commit 81ec6a5 Sep 21, 2023
35 checks passed
@justinchuby justinchuby deleted the justinchu/inf-nan-20 branch September 21, 2023 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto update doc Generate md/proto files automatically using the CI pipeline operator Issues related to ONNX operators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Support all floating dtypes for IsInf and IsNaN
5 participants