Skip to content

Promote to ONNX commit that has StringNormalizer#499

Merged
yuslepukhin merged 9 commits intomasterfrom
yuslepukhin/promote_strnormalizer
Mar 9, 2019
Merged

Promote to ONNX commit that has StringNormalizer#499
yuslepukhin merged 9 commits intomasterfrom
yuslepukhin/promote_strnormalizer

Conversation

@yuslepukhin
Copy link
Member

Adjust implementation to match ONNX spec.

  Adjust implementation to match ONNX spec.
@yuslepukhin yuslepukhin requested review from snnn and wschin February 21, 2019 00:44
@yuslepukhin yuslepukhin requested a review from a team as a code owner February 21, 2019 00:44
@snnn
Copy link
Contributor

snnn commented Feb 21, 2019

@snnn
Copy link
Contributor

snnn commented Feb 21, 2019

Please feel free to revise the doc and make it better. :-)

kernel_registry.Register(BuildKernelCreateInfo<ONNX_OPERATOR_TYPED_KERNEL_CLASS_NAME(kCpuExecutionProvider, kOnnxDomain, 9, int64_t, NonZero)>());
kernel_registry.Register(BuildKernelCreateInfo<ONNX_OPERATOR_TYPED_KERNEL_CLASS_NAME(kCpuExecutionProvider, kOnnxDomain, 9, string, Where)>());
kernel_registry.Register(BuildKernelCreateInfo<ONNX_OPERATOR_TYPED_KERNEL_CLASS_NAME(kCpuExecutionProvider, kOnnxDomain, 9, float, Where)>());

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this empty line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I need to add a comment // Opset 10

wschin
wschin previously approved these changes Feb 21, 2019
Copy link
Contributor

@wschin wschin left a comment

Choose a reason for hiding this comment

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

Looks great. Just one tiny comment.

@snnn
Copy link
Contributor

snnn commented Feb 21, 2019

The failed Linux test was due to disk space full. I'll solve it tomorrow.

Copy link
Contributor

@snnn snnn left a comment

Choose a reason for hiding this comment

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

import onnxruntime

x = ['monday' 'tuesday']
y = ['monday' 'tuesday']
np.testing.assert_allclose(x,y)

When I run this code, I got:

Traceback (most recent call last):
  File "1.py", line 6, in <module>
    np.testing.assert_allclose(x,y)
  File "/usr/local/lib64/python3.6/site-packages/numpy/testing/_private/utils.py", line 1493, in assert_allclose
    verbose=verbose, header=header, equal_nan=equal_nan)
  File "/usr/local/lib64/python3.6/site-packages/numpy/testing/_private/utils.py", line 781, in assert_array_compare
    val = comparison(x, y)
  File "/usr/local/lib64/python3.6/site-packages/numpy/testing/_private/utils.py", line 1488, in compare
    equal_nan=equal_nan)
  File "/usr/local/lib64/python3.6/site-packages/numpy/core/numeric.py", line 2521, in isclose
    xfin = isfinite(x)
TypeError: ufunc 'isfinite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

Exactly the same error.

So, please fix your test code in the ONNX repo.

onnx/onnx#1831

@skottmckay
Copy link
Contributor

skottmckay commented Feb 27, 2019

onnx/onnx#1831

Can we update to onnx/onnx@873ddbb instead?

That has some changes to BackendTest, which enables overriding the comparison function. We can also use that to fix the GRU test.

e.g. code from my branch (need ONNX update to check it in) to do the override to handle the GRU test.

bb372a9

It would however be better to fix the ONNX level comparison code, but if that is going to take longer this is possibly a short term workaround.

snnn
snnn previously approved these changes Mar 8, 2019
@yuslepukhin
Copy link
Member Author

Created onnx/onnx#1851

@yuslepukhin yuslepukhin merged commit 6136efc into master Mar 9, 2019
@yuslepukhin yuslepukhin deleted the yuslepukhin/promote_strnormalizer branch March 9, 2019 01:15
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.

5 participants