-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[OV CPU] Convert Node converting integer to unsigned one, it did clamp #32389
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?
[OV CPU] Convert Node converting integer to unsigned one, it did clamp #32389
Conversation
|
build_jenkins |
|
build_jenkins |
1 similar comment
|
build_jenkins |
|
build_jenkins |
a003adf to
fff566e
Compare
a16e4bf to
5323327
Compare
|
@nshchego, could you please review this PR? |
| const auto& funcInputs = function->inputs(); | ||
| const auto& funcInput = funcInputs[0]; | ||
| ov::Tensor tensor(funcInput.get_element_type(), targetInputStaticShapes[0]); | ||
| std::fill_n(tensor.data<int32_t>(), tensor.get_size(), -1); |
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.
Why the only one negative value is validated here?
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.
Thanks, changed to a random value.
|
|
||
| TEST_P(ConvertI32ToU8CPULayerTest, CompareWithRefs) { | ||
| run(); | ||
| CheckPluginRelatedResults(compiledModel, std::set<std::string>{"Convert", "Subgraph"}); |
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.
Do we really need to check any plugin specific parameters in this test? Looks like we need to compare output blobs only. If so, reusing of the shared tests is sufficient here.
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.
Understand your opilion, if we want to use shared test class, we also need to create a new class to re-write generate_input function with negative value inputs.
root cause is the default test case input value is random generated but with limited range, use int32->uint8 as example, defined in
- ranges.hpp, it outputs ranges [0, 1000]
- ranges.cpp, it outputs ranges [0, 255] (cause model's output port is uint8, its range is [0,255])
then, final range will be [0,255], and results the test input data will be limited in range [0,255], no negative data is tested.
so, if we want to use the shared test, we also need to create a new class to re-write the generate_input func to confirm the negative value (out of range) is tested.
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.
As suggested, moved test from cpu plugin into shared place
40615e7 to
3c14015
Compare
As Convert Op's Specification said.
Reproduce:
Test when int32 value -1 convert to uint8
Tickets: