【Hackathon 5th No.95】add paddle unique op #20077
Conversation
|
|
||
| if (axis.size() != 0) { | ||
| auto axis_node = std::make_shared<default_opset::Constant>(element::i32, Shape{}, axis); | ||
| outputs = std::make_shared<ov::opset10::Unique>(x, axis_node, true, dtype, dtype)->outputs(); |
There was a problem hiding this comment.
Can we use default_opset other than opset10 ?
There was a problem hiding this comment.
To the best of my knowledge, the unique operator is defined in opset10
But the default_opset is 9
There was a problem hiding this comment.
It is okay here. I will upgrate the default_opset later.
| reture_inverse=False, | ||
| reture_counts=False, | ||
| dtype="int64", | ||
| axes=None, |
There was a problem hiding this comment.
Can we leave these parameter's name as origin one, which can be loaded to paddle.unique directly
|
@yuxu42 Could you help to have a review on this PR, thanks |
|
@ceciliapeng2011 could you please help reivew? Thanks! |
yuxu42
added
ExternalPR
|
This PR will be closed in a week because of 2 weeks of no activity. |
| for out in unique_outs: | ||
| if out is not None: | ||
| if out.dtype == paddle.int64 or out.dtype == paddle.int32: | ||
| out = paddle.cast(out, "float32") |
There was a problem hiding this comment.
I can't figure out why cast operation is needed. In my understanding, we should keep the original unique output for verification.
There was a problem hiding this comment.
AFAIK, the openvino test compare_results only support fp32 & int32, so for the int64 index we should convert to float32/int32
There was a problem hiding this comment.
@AndPuQing Thanks for your very detailed analysis. Your finding makes sense.
Maybe the test case framework is not perfect enough to call. Don't worry at all. If you think it is not reasonable, just pull PR to fix or optimize it, which is much appreciated. We should NOT skip it using a work-around.
If you have no time for this case, let me know.
There was a problem hiding this comment.
Sorry, it seems I may not have the time to rectify the test case framework.
|
|
||
| named_outputs["Index"] = {outputs[2]}; | ||
| named_outputs["Indices"] = {outputs[1]}; | ||
| named_outputs["Counts"] = {outputs[3]}; |
There was a problem hiding this comment.
- In OpenVINO unique spec,
outputs[3]type is controled bycount_element_type. https://docs.openvino.ai/2023.1/openvino_docs_ops_movement_Unique_10.html. - In PaddlePaddle spec, I don't find the
return_countsdtype. I am worried because I run into the similar issue before. Could you confirm it here?
There was a problem hiding this comment.
In paddle, the count_element_type and index_element_type both be controlled by attr_dtype
| auto x = node.get_input("X"); | ||
|
|
||
| std::vector<Output<Node>> outputs; | ||
| NamedOutputs named_outputs; |
There was a problem hiding this comment.
As far as I know, the NamedOutputs is a std::map.
In this case, you'd better use constructor with initailization list insteading of inserting k-v one by one, I think.
|
@AndPuQing could you please fix the issues and rebase the code? Thanks! |
AndPuQing
changed the title
Done |
|
Could you change the PR's title as 【Paddle Hackathon 5th No.95】? |
|
@ceciliapeng2011 can we merge the PR? Thanks! |
Details:
Tickets:
reference
Note
dtype float32, int32, int64 would be supported, float64 ov TestConvert not be supported