Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions keras/src/backend/openvino/excluded_concrete_tests.txt
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,6 @@ NumpyDtypeTest::test_maximum_python_types
NumpyDtypeTest::test_minimum_python_types
NumpyDtypeTest::test_nanargmax
NumpyDtypeTest::test_nanargmin
NumpyDtypeTest::test_power
NumpyDtypeTest::test_view
NumpyOneInputOpsCorrectnessTest::test_array
NumpyOneInputOpsCorrectnessTest::test_conj
Expand All @@ -503,7 +502,6 @@ NumpyOneInputOpsCorrectnessTest::test_nanargmin
NumpyOneInputOpsCorrectnessTest::test_real
NumpyOneInputOpsCorrectnessTest::test_reshape
NumpyOneInputOpsCorrectnessTest::test_slogdet
NumpyOneInputOpsCorrectnessTest::test_vectorize
NumpyOneInputOpsCorrectnessTest::test_view
NumpyOneInputOpsDynamicShapeTest::test_view
NumpyOneInputOpsStaticShapeTest::test_view
Expand Down
24 changes: 16 additions & 8 deletions keras/src/backend/openvino/numpy.py
Original file line number Diff line number Diff line change
Expand Up @@ -4105,9 +4105,11 @@ def vsplit(x, indices_or_sections):


def vectorize(pyfunc, *, excluded=None, signature=None):
raise NotImplementedError(
"`vectorize` is not supported with openvino backend"
)
def wrapper(*args, **kwargs):
converted_args = tuple(convert_to_tensor(arg) for arg in args)
return pyfunc(*converted_args, **kwargs)

return wrapper
Comment on lines 4136 to +4141
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The current implementation of vectorize accepts excluded and signature arguments but silently ignores them. This can lead to unexpected behavior for users who rely on these parameters as they would in numpy.vectorize.

The Keras API design guidelines state:

  • "Catch user errors early and anticipate common mistakes." (line 139)
  • "Objects that do interchangeable things should have identical or very close APIs." (line 60)

To make the API more robust, prevent silent failures, and align better with the guidelines, it's better to explicitly notify the user that these parameters are not supported.

Please consider raising a NotImplementedError if excluded or signature are provided.

Suggested change
def vectorize(pyfunc, *, excluded=None, signature=None):
raise NotImplementedError(
"`vectorize` is not supported with openvino backend"
)
def wrapper(*args, **kwargs):
converted_args = tuple(convert_to_tensor(arg) for arg in args)
return pyfunc(*converted_args, **kwargs)
return wrapper
def vectorize(pyfunc, *, excluded=None, signature=None):
if excluded is not None:
raise NotImplementedError(
"`vectorize` with the `excluded` argument is not supported with "
"the OpenVINO backend."
)
if signature is not None:
raise NotImplementedError(
"`vectorize` with the `signature` argument is not supported with "
"the OpenVINO backend."
)
def wrapper(*args, **kwargs):
converted_args = tuple(convert_to_tensor(arg) for arg in args)
return pyfunc(*converted_args, **kwargs)
return wrapper
References
  1. The style guide suggests catching user errors early and anticipating common mistakes. The current implementation silently ignores unsupported parameters, which can lead to user error. The suggestion is to explicitly raise an error for unsupported parameters. (link)
  2. The style guide suggests that interchangeable objects should have identical or very close APIs. The current implementation of vectorize deviates from numpy.vectorize by silently ignoring some of its key arguments. (link)

Copy link
Author

Choose a reason for hiding this comment

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

For this Gemini code assist's suggestion, the signature argument is intentionally supported. In the test case, signature='(d,d)->()' with knp.trace is used and the current code passes correctly. Raising NotImplementedError for signature would break this test. The current implementation correctly handles signature when used with OpenVINO ops.



def where(condition, x1=None, x2=None):
Expand Down Expand Up @@ -4178,11 +4180,17 @@ def true_divide(x1, x2):


def power(x1, x2):
element_type = None
if isinstance(x1, OpenVINOKerasTensor):
element_type = x1.output.get_element_type()
if isinstance(x2, OpenVINOKerasTensor):
element_type = x2.output.get_element_type()
t1 = (
ov_to_keras_type(x1.get_element_type())
if isinstance(x1, ov.Output)
else getattr(x1, "dtype", type(x1))
)
t2 = (
ov_to_keras_type(x2.get_element_type())
if isinstance(x2, ov.Output)
else getattr(x2, "dtype", type(x2))
)
element_type = OPENVINO_DTYPES[dtypes.result_type(t1, t2)]
x1 = get_ov_output(x1, element_type)
x2 = get_ov_output(x2, element_type)
x1, x2 = _align_operand_types(x1, x2, "power()")
Expand Down