Skip to content
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

support construct submatrix from numpy array #22822

Open
wants to merge 2 commits into
base: 4.x
Choose a base branch
from

Conversation

huww98
Copy link

@huww98 huww98 commented Nov 17, 2022

Some APIs behave differently when operating on a small mat, or an ROI of a large mat. For example cv::filter2D. However, python cannot construct a Mat with ROI. This patch enables python to invoke these behaviors that rely on ROI.

cv2.filter2D(img[y1:y2, x1:x2], -1, kernel)

This may change the behavior of some old code. But I think the new behavior is more suitable and matches the expectation of C++ programmers.

I think this is a "Small feature", so the base branch is 4.x.

Not sure whether or where I should add some docs about this?

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

@asmorkalov asmorkalov added category: python bindings pr: needs test New functionality requires minimal tests set labels Nov 17, 2022
@asmorkalov
Copy link
Contributor

@huww98 Could you add simple test with the feature?

@asmorkalov asmorkalov removed the pr: needs test New functionality requires minimal tests set label Nov 23, 2022
@asmorkalov
Copy link
Contributor

I added simple test for ROI in Python. The feature works good to me with Python 3.6.9.

@huww98
Copy link
Author

huww98 commented Nov 23, 2022

I got some unexpected results while writing my test. I'm debugging this.

img = np.array([
    [1,1,1],
    [1,10,1],
    [1,1,1],
], dtype=np.uint8)
kernel = np.full((3,3), 1/9, dtype=np.float32)
submatrix = img[1:2, 1:2]
result_submatrix = cv.filter2D(submatrix, -1, kernel)
self.assertEqual(result_submatrix[0,0], 2)

got

AssertionError: 4 != 2

@huww98
Copy link
Author

huww98 commented Nov 23, 2022

Should we add a new parameter roi=False to cv2.Mat constructor, so this feature is opt-in and not breaking existing code?

@asmorkalov
Copy link
Contributor

It looks like the issue is related to filter2D, but not ROI. BORDER_DEFAULT is equivalent to BORDER_REFLECT_101 gfedcb|abcdefgh|gfedcba. With larger input filter2D works correctly:

import cv2 as cv
import numpy as np

kernel = np.full((3,3), 1/9, dtype=np.float32)

img = np.array([
    [1,1,1,1],
    [1,10,1,1],
    [1,1,1,1],
    [1,1,1,1],
], dtype=np.uint8)

submatrix = img[1:3, 1:3]
print(submatrix)
result_submatrix = cv.filter2D(submatrix, -1, kernel)
print(result_submatrix)

Output:

[[10  1]
 [ 1  1]]
[[2 2]
 [2 2]]

@asmorkalov
Copy link
Contributor

@alalek What do you think on roi=False?

@VadimLevin
Copy link
Contributor

As direct test of sub-matrix support, I suggesting you to add test_InputArray_submatrix test cases similar to existing test_InputArray checks.
Test Cases
dumpInputArray function declaration
dumpInputArray function definition

[15, 16, 17, 18, 19]], dtype=np.uint8)
roi_min, roi_max, _, _ = cv.minMaxLoc(m[1:3, 1:4])
self.assertEqual(roi_min, 6)
self.assertEqual(roi_max, 13)
Copy link
Member

@alalek alalek Nov 23, 2022

Choose a reason for hiding this comment

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

This test passes without the fix.

@asmorkalov
Copy link
Contributor

Made experiments with C++ code. The first case with original matrix 3.x produce correct result. It means that filter2d works well.
Code:

#include "opencv2/core.hpp"
#include "opencv2/highgui.hpp"
#include "opencv2/imgproc.hpp"
#include <iostream>
using namespace cv;

int main(int argc, char **argv)
{
    cv::Mat kernel(3, 3, CV_32FC1, Scalar(1.f/9.f));
    unsigned char input_data[] = {
        1,1,1,
        1,10,1,
        1,1,1
    };
    cv::Mat input(Size(3,3), CV_8UC1, input_data);
    cv::Mat roi = input(Rect(1,1,1,1));
    std::cout << "Roi:\n" << roi << std::endl;
    cv::Mat result;
    filter2D(roi, result, -1, kernel);
    std::cout << "Roi Result:\n" << result << std::endl;
    return 0;
}

Output:

Roi:
[ 10]
Roi Result:
[  2]

@huww98
Copy link
Author

huww98 commented Dec 19, 2022

got

AssertionError: 4 != 2

This is because we are not copying over the stride for dims with size 1. So the matrix in my test case is recognized as 9x1 rather than 3x3.

After further investigation, I find it challenging to deal with the automatic copy. It is hard to anticipate what will happen when a submatrix is passed in, but it needs copying.

  • Refusing to proceed would break many existing codes.
  • Copying the whole matrix then making an ROI would consume unbounded memory.
  • Only copy the submatrix would preserve the current behavior. But make it difficult to diagnose when one really wants an ROI.
  • What to do if the submatrix doesn't need copying, but the full matrix does?

My plan is to abandon this PR. And design a more explicit API for ROI. e.g.:

  1. Mimic C++ API design, and also cv2.UMat.
    roi = cv2.Mat(img, (1,1,1,1))
  2. roi_of: explicitly specify the full matrix.
    roi = cv2.Mat(img[1:2, 1:2], roi_of=img)

Then we can just refuse to proceed if copying is needed in this new API.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants