Skip to content

Make default axis of softmax in onnx "-1" without opset option#24613

Merged
asmorkalov merged 12 commits intoopencv:4.xfrom
WanliZhong:softmax_default_axis
Dec 15, 2023
Merged

Make default axis of softmax in onnx "-1" without opset option#24613
asmorkalov merged 12 commits intoopencv:4.xfrom
WanliZhong:softmax_default_axis

Conversation

@WanliZhong
Copy link
Copy Markdown
Member

@WanliZhong WanliZhong commented Nov 29, 2023 •
edited
Loading

Try to solve problem: #24476 (comment)

ONNX
opset <= 11 use 1
else use -1

TensorFlow
TF version = 2.x use -1
else use 1

Darknet, Caffe, Torch
use 1 by definition

@WanliZhong WanliZhong added the category: dnn (onnx) ONNX suport issues in DNN module label Nov 29, 2023
@WanliZhong WanliZhong added this to the 4.9.0 milestone Nov 29, 2023
fengyuentau
fengyuentau reviewed Nov 29, 2023
View reviewed changes
@asmorkalov asmorkalov requested a review from dkurt November 29, 2023 06:38
@WanliZhong
Copy link
Copy Markdown
Member Author

WanliZhong commented Nov 30, 2023

Not sure why always abi test failed.

@opencv-alalek opencv-alalek added the pr: needs test New functionality requires minimal tests set label Nov 30, 2023
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 1, 2023

@WanliZhong Could you push to the branch to trigger build once more. CI was broken the time you created the patch. My fault.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 1, 2023

@WanliZhong Could you add some test for the case or unlock some "conformance" test that covers it?

@WanliZhong
Copy link
Copy Markdown
Member Author

WanliZhong commented Dec 1, 2023

@asmorkalov Thanks for comments. I will try it.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 6, 2023 

[  FAILED  ] 15 tests, listed below:
[  FAILED  ] Test_Caffe_nets.Colorization/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Caffe_nets.FasterRCNN_vgg16/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Caffe_nets.FasterRCNN_zf/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Caffe_nets.RFCN/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Darknet_layers.avgpool_softmax/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Int8_layers.Softmax_slim_v2_TF/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Int8_nets.RFCN/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Caffe_layers.Softmax/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Model.DetectionOutput/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_ONNX_nets.Squeezenet/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_TensorFlow_layers.softmax_keras/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_TensorFlow_layers.softmax_slim/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_TensorFlow_layers.softmax_slim_v2/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Torch_layers.net_softmax/0, where GetParam() = OCV/CPU
[  FAILED  ] Test_Torch_layers.net_logsoftmax/0, where GetParam() = OCV/CPU

@WanliZhong
Copy link
Copy Markdown
Member Author

WanliZhong commented Dec 6, 2023

yes, the failed CI have shown we can't modify the default axis to -1 in softmax layer. I propose just make this happen on onnx model.

@WanliZhong WanliZhong mentioned this pull request Dec 14, 2023
Merged
13 tasks
@opencv-alalek opencv-alalek added test and removed pr: needs test New functionality requires minimal tests set labels Dec 15, 2023
opencv-alalek
opencv-alalek reviewed Dec 15, 2023
View reviewed changes
modules/dnn/src/onnx/onnx_importer.cpp
{
const std::string& layer_type = node_proto.op_type();
int axis;
if (layerParams.has("opset") && layerParams.get<int>("opset") > 11) {
Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek Dec 15, 2023

Choose a reason for hiding this comment

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

Should we update parseQSoftmax too? (may be in a separate PR?)

Copy link
Copy Markdown
Member Author

@WanliZhong WanliZhong Dec 15, 2023

Choose a reason for hiding this comment

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

Let me try to change it in QSoftmax

Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Dec 15, 2023

Choose a reason for hiding this comment

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

Please create dedicated PR for parseQSoftmax.

fengyuentau
fengyuentau reviewed Dec 15, 2023
View reviewed changes
fengyuentau
fengyuentau approved these changes Dec 15, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@fengyuentau fengyuentau left a comment

Choose a reason for hiding this comment

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

👍

@asmorkalov asmorkalov merged commit 6ae1709 into opencv:4.x Dec 15, 2023
@asmorkalov asmorkalov mentioned this pull request Jan 19, 2024
Merged
@CNOCycle CNOCycle mentioned this pull request Mar 29, 2024
Open
thewoz pushed a commit to thewoz/opencv that referenced this pull request May 29, 2024
@WanliZhong @thewoz
Merge pull request opencv#24613 from WanliZhong:softmax_default_axis
284e6be 
Make default axis of softmax in onnx "-1" without opset option opencv#24613

Try to solve problem: opencv#24476 (comment)

**ONNX**
`opset <= 11` use 1
`else` use -1

**TensorFlow**
`TF version = 2.x` use -1
`else` use 1

**Darknet, Caffe, Torch**
use 1 by definition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov left review comments

@opencv-alalek opencv-alalek opencv-alalek left review comments

@fengyuentau fengyuentau fengyuentau approved these changes

@dkurt dkurt dkurt approved these changes

Assignees

@dkurt dkurt

Labels

category: dnn (onnx) ONNX suport issues in DNN module test

Projects

None yet

Milestone

4.9.0

Development

Successfully merging this pull request may close these issues.

5 participants

@WanliZhong @asmorkalov @fengyuentau @dkurt @opencv-alalek