-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[cpu][gpu][ref]: OneHot op: Added support for handling negative indices in onnx.OneHot-11 fashion. #31539
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?
[cpu][gpu][ref]: OneHot op: Added support for handling negative indices in onnx.OneHot-11 fashion. #31539
Conversation
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.
LGTM for GPU part
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 changes in OneHot operation are backward compatible, and maintain original behavior by default. It should be acceptable to make it within the same op version with proper note in the spec, as it has been proposed. Such approach requires less changes in the code base, in fact mitigate regressions risk and reduce multi version op maintenance.
*Added suggestion for better tests coverage of the changes.
@@ -16,7 +17,9 @@ void one_hot(const INPUT_TYPE* indices, | |||
const size_t depth, | |||
const int64_t one_hot_axis, | |||
const char* on_value, | |||
const char* off_value) { | |||
const char* off_value, | |||
const op::v1::OneHot::NegativeIndicesMode mode) { |
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.
Optional: The reference could have a default (backward compatible) mode as well.
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.
Can this model be something not depends on OP version? if there will be new version OneHot the v1 mode maybe not used and usually references can be shared or expanded for new versions
/// \brief Sets the negative indices mode. | ||
void set_negative_indices_mode(NegativeIndicesMode mode) { | ||
m_negative_indices_mode = mode; | ||
} | ||
/// \return The negative indices mode. | ||
NegativeIndicesMode get_negative_indices_mode() const { | ||
return m_negative_indices_mode; | ||
} | ||
|
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.
Usually it is recommended by @praasz to have set/get body in .cpp
size_t output_offset = out_elem_size * (outer_i * depth + inner_i + oh_index * inner_block); | ||
const int64_t input_val = static_cast<int64_t>(indices[outer_i + inner_i]); | ||
const int64_t depth_i64 = static_cast<int64_t>(depth); | ||
const int64_t actual_index = (input_val < 0 && is_mode_normalize) ? depth_i64 + input_val : input_val; |
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 can see there is onnx test enabled, but any changes in the reference implementation should be covered by the corresponding "Template plugin" tests and expected output:
class ReferenceOneHotTest : public testing::TestWithParam<OneHotParams>, public CommonReferenceTest { |
@@ -85,13 +89,19 @@ void OneHot::validate_and_infer_types() { | |||
bool OneHot::visit_attributes(AttributeVisitor& visitor) { | |||
OV_OP_SCOPE(v1_OneHot_visit_attributes); | |||
visitor.on_attribute("axis", m_axis); | |||
visitor.on_attribute("negative_indices_mode", m_negative_indices_mode); |
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 new attribute should be covered by the corresponding "visitor" tests:
EXPECT_EQ(g_one_hot->get_axis(), one_hot->get_axis()); |
@@ -72,5 +72,5 @@ graph { | |||
} | |||
} | |||
opset_import { | |||
version: 9 | |||
version: 11 |
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.
In fact, there should be a new test model file added with version 11, and the old one should be kept for the tests of the older version.
OPENVINO_TEST(${BACKEND_NAME}, onnx_model_one_hot_without_axis_negative_indicies) { | ||
auto model = convert_model("one_hot_no_axis.onnx"); | ||
|
||
std::vector<std::vector<std::int64_t>> inputs{{0, -5, -4}, {2, 5}}; |
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.
Could you please add a test where on/off_value is a Constant? It's current limitation of CPU OneHot implementation:
openvino/src/plugins/intel_cpu/src/nodes/one_hot.cpp
Lines 40 to 48 in adcb532
if (ov::as_type_ptr<const ov::op::v0::Constant>(oneHot->get_input_node_shared_ptr(ON_VALUE_ID)) == nullptr) { | |
errorMessage = "Only const 'on_value' input is supported"; | |
return false; | |
} | |
if (ov::as_type_ptr<const ov::op::v0::Constant>(oneHot->get_input_node_shared_ptr(OFF_VALUEAXES_ID)) == | |
nullptr) { | |
errorMessage = "Only const 'off_value' input is supported"; | |
return false; | |
} |
@@ -174,6 +189,7 @@ const std::vector<ov::Shape> staticInputShapes0D = { | |||
const auto testCase_1d = ::testing::Combine( | |||
::testing::ValuesIn(static_shapes_to_test_representation(staticInputShapes0D)), | |||
::testing::Values(-1, 0), | |||
::testing::Values(OHMode::IGNORE_NEGATIVE, OHMode::NORMALIZE), |
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.
Great that the CPU tests has been extended, as those single_layer_tests use reference implementation for comparison, the missing part is to ensure correctness of the output values is to update the reference implementation tests.
@@ -90,6 +90,7 @@ onnx_model_conv_with_dynamic_batch | |||
# OneHot operation has a form that is not supported | |||
onnx_model_one_hot_without_axis | |||
onnx_model_one_hot_with_axis | |||
onnx_model_one_hot_without_axis_negative_indicies |
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 tests listed here are skipped for CPU/GPU plugin validation. The Template/interpreter plugin (reference) has a separate manifest file here https://github.com/openvinotoolkit/openvino/blob/master/src/frontends/onnx/tests/runtime/interpreter/unit_test.manifest
what means it is still tested with reference.
My suggestion is to create additional case to fit the plugin expectations, likely with on/off_value as a Constant input.
std::ostream& operator<<(std::ostream& s, const op::v1::OneHot::NegativeIndicesMode& mode); | ||
|
||
template <> | ||
class OPENVINO_API AttributeAdapter<op::v1::OneHot::NegativeIndicesMode> |
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 try to add appropriate extern template to openvino/src/core/include/openvino/core/attribute_adapter.hpp:265
It helps to speed up build
OneHot specification is extended with new attribute: negative_indices_mode, with two modes:
Added support of this new extension to reference, CPU and GPU implementations. Added new tests.
Tickets: