-
Notifications
You must be signed in to change notification settings - Fork 19.6k
[OpenVINO Backend] : add support for numpy.nan_to_num #21186
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: 11happy <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #21186 +/- ##
==========================================
+ Coverage 82.59% 82.61% +0.02%
==========================================
Files 564 564
Lines 54407 54474 +67
Branches 8449 8469 +20
==========================================
+ Hits 44936 45003 +67
+ Misses 7396 7395 -1
- Partials 2075 2076 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: 11happy <[email protected]>
@@ -96,7 +96,6 @@ NumpyOneInputOpsCorrectnessTest::test_median | |||
NumpyOneInputOpsCorrectnessTest::test_meshgrid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also remove NumpyDtypeTest::test_nan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
keras/src/backend/openvino/numpy.py
Outdated
nan_vector = ov_opset.broadcast( | ||
ov_opset.constant(nan, dtype), shape_x | ||
).output(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be just a scalar. select operation will broadcast it automatically. No need broadcast by yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same comment for pos_inf
and neg_inf
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: 11happy <[email protected]>
Sure I will create a new issue there , also I have correctec the code based on new tests & previous reviews. |
Signed-off-by: 11happy <[email protected]>
Signed-off-by: 11happy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fchollet, looks good to me. Recommend to merge.
keras/src/backend/openvino/numpy.py
Outdated
neginf if neginf is not None else DTYPES_MIN[dtype], dtype | ||
).output(0) | ||
posinf_mask = ov_opset.is_inf( | ||
ov_opset.convert(x, Type.f32), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably it is a bug on OpenVINO side. Please create issue for non-f32 types in OpenVINO repo. For now, this WA absolutely works for us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done , I have created the issue.
Thank you
x = get_ov_output(x) | ||
dtype = x.get_element_type() | ||
nan_val = ov_opset.constant(nan, dtype).output(0) | ||
posinf_val = ov_opset.constant( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@11happy, let us do not call is_inf
and is_nan
for integer types. You can return input as is as I understand.
For floating point types you need to call but convert to fp32 is not needed.
Can you fix it please in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a check for int type & early retunr & removed the dtype conversion to float ,
if dtype.is_integral():
return OpenVINOKerasTensor(x)
removed this ov_opset.convert(x, Type.f32))
conversion from is_inf, with these changes test is failing , it is not able to detect the infs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@11happy, please clarify what cases exactly are failing. For int types, it should pass because there is no inf and nan values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the test case that is failing is previous one only
> self.assertAllClose(
knp.nan_to_num(x, nan=2, posinf=3, neginf=4), [1.0, 2.0, 3.0, 4.0]
)
keras/src/ops/numpy_test.py:4806:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
keras/src/testing/test_case.py:48: in assertAllClose
np.testing.assert_allclose(x1, x2, atol=atol, rtol=rtol, err_msg=msg)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
args = (<function assert_allclose.<locals>.compare at 0x722684c8ed40>, array([ 1.00000000e+00, 2.00000000e+00, 3.40282347e+38, -3.40282347e+38]), array([1., 2., 3., 4.]))
kwds = {'equal_nan': True, 'err_msg': 'None', 'header': 'Not equal to tolerance rtol=1e-06, atol=1e-06', 'verbose': True}
@wraps(func)
def inner(*args, **kwds):
with self._recreate_cm():
> return func(*args, **kwds)
E AssertionError:
E Not equal to tolerance rtol=1e-06, atol=1e-06
E None
E Mismatched elements: 2 / 4 (50%)
E Max absolute difference: 3.40282347e+38
E Max relative difference: 1.13427449e+38
E x: array([ 1.000000e+00, 2.000000e+00, 3.402823e+38, -3.402823e+38])
E y: array([1., 2., 3., 4.])
/usr/lib/python3.12/contextlib.py:81: AssertionError
I tried to debug it , my observations:
this is failing for f64 dtype, here the test case with dtype f64 is failing , also in conversion logic if I intentionally convert it to f64 type instead of f32 the above test fails. here specifically it passes with f16,f32, but fails with f64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now a reproducible code on f64 dtype for the same error
I have posted that on the previous ticket openvinotoolkit/openvino#30264
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, can you please do the implementation as follows:
- return input as is for integer types
- use
is_inf
andis_nan
mask for floating point types. In addition, use WA with convert for fp64, for other types it is not needed.
Best regards,
Roman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have also added a comment referencing the issue
Signed-off-by: 11happy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fchollet, looks good to me. Recommend to merge.
Overview:
Testing:
CC: