dnn: support ONNX Slice with negative steps by adding and using cv::flipND by fengyuentau · Pull Request #22898 · opencv/opencv

fengyuentau · 2022-12-01T09:48:35Z

Current workaround for negative steps is computing the forward indexing range for slice and flip at axis with negative step:

x of shape [5, 10], x[5:0:-1, 10:1:-3] <=> np.flip(x[1:5:1, 2:10:3], aixs=(0, 1))

We can also extend Range to support backward indexing and steps like Python list, which needs more effort to do so I think.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

fengyuentau · 2022-12-05T02:25:29Z

@alalek I have added tests for both flipND and Slice with negative steps. Any test is missing here?

alalek · 2022-12-05T09:16:25Z

@fengyuentau Labels test and pr: needs test are different.

fengyuentau · 2022-12-05T10:03:29Z

Could you explain what the differences are? I thought the label “test” stands for missing tests, but it looks like I was wrong.

…

________________________________ 发件人: Alexander Alekhin ***@***.***> 发送时间: Monday, December 5, 2022 5:16:37 PM 收件人: opencv/opencv ***@***.***> 抄送: Yuantao Feng ***@***.***>; Mention ***@***.***> 主题: Re: [opencv/opencv] dnn: support ONNX Slice with negative steps by adding and using cv::flipND (PR #22898) @fengyuentau<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ffengyuentau&data=05%7C01%7C%7Cc69237f61b54458473e608dad6a16a20%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638058285998615153%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=U3pijJfJRqmN8q4hQ5nd3NLFwzODdfVAtqEC6GDveY4%3D&reserved=0> Labels test and pr: needs test are different. ― Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopencv%2Fopencv%2Fpull%2F22898%23issuecomment-1337006734&data=05%7C01%7C%7Cc69237f61b54458473e608dad6a16a20%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638058285998615153%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=eFqb4ueTPOgKp2t9tgkO0dsUjPfSFCKMNfFp%2F00qm2k%3D&reserved=0>, or unsubscribe<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAEDL63SWQDODXEJHPGAYFU3WLWXHLANCNFSM6AAAAAASQSTALY&data=05%7C01%7C%7Cc69237f61b54458473e608dad6a16a20%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638058285998615153%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3oGLYmXkP9GArpz%2B4xasw9Ju1wdbwiN6nu5ZNWtZOL4%3D&reserved=0>. You are receiving this because you were mentioned.Message ID: ***@***.***>

rogday

Thank you for your contribution! I think in the future we should split this kind of PRs in two. One in core only, and another one to dnn.

modules/core/src/matrix_transform.cpp

modules/dnn/src/layers/slice_layer.cpp

modules/core/src/matrix_transform.cpp

rogday

LGTM! 👍

asmorkalov · 2022-12-12T12:48:07Z

@alalek May we merge this patch?

modules/core/src/matrix_transform.cpp

alalek · 2022-12-12T12:52:26Z

modules/core/src/matrix_transform.cpp

+    }
+    else
+    {
+        flipNDImpl(src.ptr(), shape, step, dst.ptr(), axis);


dst step is not handled and not checked.

Do you mean I should check whether dst steps are the same with src steps?

Yes, btw in case of ROI we could just overlap not from the beggining. @alalek do we have a method that checks if mats are non-overlapping?

Perhaps there is no such method.
We could do simple check overlap between .data / .dataend here (it is not fully accurate, e.g. for 1d cases)

I think we should just check if .datas are equal (if it's the start of the allocation, might be .datastart instead). It's not ideal for roi-to-roi in the same matrix without overlaps, but I'm not sure there is a platform-independent solution to check if memory regions overlap(without UB).

Ideally speaking, we should check ROI overlaps but who would do that intentionally? Such operations do not make sense. Or if we are talking about unintentionally mis-operation, it is what it is as long as it does not trigger crashing.

Generally I agree, but in this case it doesn't cost us anything to change ptr() to datastart.

I re-implemented with dst in-place flip and also ensuring both src and dst are continuous. In this case, I suppose we can skip checking whether steps are equal, can't we?

alalek · 2022-12-12T12:53:31Z

modules/core/test/test_arithm.cpp

+INSTANTIATE_TEST_CASE_P(Arithm, FlipND, testing::Combine(
+    testing::Values(std::vector<int>{5, 10}, std::vector<int>{2, 3, 4}),
+    testing::Values(perf::MatType(CV_8UC1), CV_32FC1)
+));


ROI handing of src/dst/src+dst should be added.

I don't quite understand what ROI handling is here. Examples?

Take a look on OCL_TEST_P(Transpose, Mat) test design (input data generation).

So if I understand correctly, case of non continuous input mat should be tested. Correct me if I am wrong.

Right. ROI means region of interest - it is a sub-matrix from larger matrix.

However, for ND case this may be not really useful. In 2D case we have flip().
Anyway, we still need to provide checks for that (contiguous data assumption) in the source code of implementation.

Anyway, we still need to provide checks for that (contiguous data assumption) in the source code of implementation.

I don't think the implementation assumes that data is contiguous.

Do we still need checks for ROI handling since in the latest impl src and dst are assumed to be continuous?

modules/core/src/matrix_transform.cpp

rogday

Nothing major, looks good.

rogday · 2022-12-20T22:28:44Z

modules/core/src/matrix_transform.cpp

+    size_t offset = 0;
+    size_t offset_increment = axis == 0 ? 0 : step[axis - 1];
+    for (int i = 0; i < total; ++i, offset += offset_increment)
+        for (int j = 0, k = shape_at_axis - 1; j < (shape_at_axis + 1) / 2; ++j, --k)


Isn't +1 redundant? In case of shape [A, 3, B]: (3+1)/2=2, but we need only 1 swap.

You are right. +1 is removed in the latest commit.

rogday · 2022-12-20T22:28:54Z

modules/core/src/matrix_transform.cpp

+    int axis = (_axis + ndim) % ndim;
+
+    // return the src if it has only one element on the flip axis
+    if (shape[axis] == 1)


We don't need this special case, I think.

It serves like an early stop to improve efficiency, doesn't it?

To be honest, I don't know what we are winning here - one function frame that will do exactly this? I think copying is much slower than calling a function. @alalek can you elaborate?

I think copying is much slower than calling a function

function itself is heavier than simple copying.

There is no problem with that part. Refer to convertTo() implementation.

rogday · 2022-12-20T22:30:08Z

modules/core/src/matrix_transform.cpp

+    CV_INSTRUMENT_REGION();
+
+    Mat src = _src.getMat();
+    CV_Assert(src.isContinuous());


I believe we can lift that restriction.

I think so. I also lifted the restriction for dst since it will be covered by calling src.copyTo(dst) and be continuous in the end.

modules/core/src/matrix_transform.cpp

alalek

core part looks good to merge 👍

asmorkalov · 2022-12-23T08:37:35Z

@fengyuentau please squash commits before merge.

fengyuentau · 2022-12-23T08:49:10Z

@asmorkalov Commits are squashed. Ready to be merged.

fengyuentau added feature category: core category: dnn category: dnn (onnx) ONNX suport issues in DNN module labels Dec 1, 2022

fengyuentau force-pushed the slice_neg_steps branch from 6fa9abd to 0a12d7d Compare December 1, 2022 09:52

fengyuentau mentioned this pull request Dec 1, 2022

model & data for slice with negative steps opencv/opencv_extra#1023

Merged

asmorkalov requested a review from rogday December 1, 2022 10:48

asmorkalov added this to the 4.7.0 milestone Dec 1, 2022

rogday suggested changes Dec 7, 2022

View reviewed changes

rogday reviewed Dec 8, 2022

View reviewed changes

modules/dnn/src/layers/slice_layer.cpp Outdated Show resolved Hide resolved

rogday reviewed Dec 8, 2022

View reviewed changes

modules/dnn/src/layers/slice_layer.cpp Outdated Show resolved Hide resolved

rogday reviewed Dec 8, 2022

View reviewed changes

modules/core/src/matrix_transform.cpp Outdated Show resolved Hide resolved

rogday approved these changes Dec 9, 2022

View reviewed changes

fengyuentau force-pushed the slice_neg_steps branch from 789a0b2 to a9ea876 Compare December 9, 2022 08:48

alalek reviewed Dec 12, 2022

View reviewed changes

fengyuentau force-pushed the slice_neg_steps branch from fb493b5 to 3526072 Compare December 18, 2022 13:12

asmorkalov requested a review from alalek December 20, 2022 12:08

rogday reviewed Dec 20, 2022

View reviewed changes

fengyuentau force-pushed the slice_neg_steps branch from 02268cd to d7d0ddd Compare December 21, 2022 07:34

alalek reviewed Dec 22, 2022

View reviewed changes

modules/core/src/matrix_transform.cpp Show resolved Hide resolved

modules/core/src/matrix_transform.cpp Outdated Show resolved Hide resolved

alalek approved these changes Dec 23, 2022

View reviewed changes

add cv::flipND; support onnx slice with negative steps via cv::flipND

34a0897

fengyuentau force-pushed the slice_neg_steps branch from d1a7318 to 34a0897 Compare December 23, 2022 08:40

asmorkalov merged commit 4930516 into opencv:4.x Dec 23, 2022

alalek mentioned this pull request Jan 8, 2023

(5.x) Merge 4.x #23113

Merged

fengyuentau mentioned this pull request Mar 14, 2023

ONNX conformance test results #21078

Open

48 tasks

fengyuentau deleted the slice_neg_steps branch June 15, 2023 02:37

fengyuentau mentioned this pull request Jun 15, 2023

Consider half pixel mode in ONNX resize #23795

Merged

6 tasks

Uh oh!

Conversation

fengyuentau commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

fengyuentau commented Dec 5, 2022

Uh oh!

alalek commented Dec 5, 2022

Uh oh!

fengyuentau commented Dec 5, 2022 via email

Uh oh!

rogday left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rogday left a comment

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Dec 12, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogday Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fengyuentau Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rogday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fengyuentau commented Dec 1, 2022 •

edited

Loading

rogday Dec 15, 2022 •

edited

Loading

fengyuentau Dec 15, 2022 •

edited

Loading