python: support tuple src for cv::add()/subtract()/... #24074
Conversation
Relace ArgInfo(any,false) -> ArgInfo(any,0)
There was a problem hiding this comment.
Please add tests for NumPy array cases mentioned in the issue:
>>> cv2.subtract(mat, np.uint8([10, 10, 10]))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
cv2.error: OpenCV(4.7.0) D:\a\opencv-python\opencv-python\opencv\modules\core\src\arithm.cpp:652: error: (-215:Assertion failed) type2 == CV_64F && (sz2.height == 1 || sz2.height == 4) in function 'cv::arithm_op'
>>> cv2.subtract(mat, np.array([10, 10, 10]))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
cv2.error: OpenCV(4.7.0) D:\a\opencv-python\opencv-python\opencv\modules\core\src\arithm.cpp:652: error: (-215:Assertion failed) type2 == CV_64F && (sz2.height == 1 || sz2.height == 4) in function 'cv::arithm_op'
>>> cv2.subtract(mat, np.float64([10, 10, 10]))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
cv2.error: OpenCV(4.7.0) D:\a\opencv-python\opencv-python\opencv\modules\core\src\arithm.cpp:652: error: (-215:Assertion failed) type2 == CV_64F && (sz2.height == 1 || sz2.height == 4) in function 'cv::arithm_op'
|
Thank you very much for your review.
I'm sorry but I was only thinking about tuple support, didn't care about array support. |
|
Currently I think new patch handles array for cv::add()/cv::subtract()/... well. However, there is an additional problem. I'll continue to investigate more(But it seems to be hard). I'm sorry. memos
I tired with C++. #include <iostream>
#include <opencv2/core.hpp>
#include <opencv2/imgcodecs.hpp>
int main()
{
cv::Mat src = (cv::Mat_<uchar>(3, 9) <<
128, 192, 64, 128, 192, 64, 128, 192, 64,
128, 192, 64, 128, 192, 64, 128, 192, 64,
128, 192, 64, 128, 192, 64, 128, 192, 64 );
src = src.reshape(3);
std::cout << "<< src >>"
<< " cols=" << src.cols
<< " rows=" << src.rows << std::endl;
std::cout << src << std::endl;
cv::Mat dst;
cv::subtract(src, 5, dst );
std::cout << "<< cv::subtract(src, 5, dst ); >>" << std::endl;
std::cout << "<< dst >>"
<< " cols=" << dst.cols
<< " rows=" << dst.rows << std::endl;
std::cout << dst << std::endl;
cv::subtract(src, cv::Scalar(5), dst );
std::cout << "<< cv::subtract(src, cv::Scalar(5), dst ); >> " << std::endl;
std::cout << "<< dst >>"
<< " cols=" << dst.cols
<< " rows=" << dst.rows << std::endl;
std::cout << dst << std::endl;
cv::subtract(src, cv::Scalar(5,0,0,0), dst );
std::cout << "<< cv::subtract(src, cv::Scalar(5,0,0,0), dst ); >> " << std::endl;
std::cout << "<< dst >>"
<< " cols=" << dst.cols
<< " rows=" << dst.rows << std::endl;
std::cout << dst << std::endl;
return 0;
}Result is python result diff --git a/modules/python/test/test_misc.py b/modules/python/test/test_misc.py
index 3d1c5c4b94..ac2127b9ed 100644
--- a/modules/python/test/test_misc.py
+++ b/modules/python/test/test_misc.py
@@ -882,6 +882,10 @@ class Arguments(NewOpenCVTests):
# Special case, vector length is not same as color channel of src mat
## bgr image
+ dst = cv.subtract(src1_mat, 32)
+ np.testing.assert_equal(dst[0][0], [32,32,32], "subtract(mat, vec1) [0][0]")
+ np.testing.assert_equal(dst[4][4], [32,32,32], "subtract(mat, vec1) [4][4]")
+
dst = cv.subtract(src1_mat, (32))
np.testing.assert_equal(dst[0][0], [32,64,64], "subtract(mat, vec1) [0][0]")
np.testing.assert_equal(dst[4][4], [32,64,64], "subtract(mat, vec1) [4][4]") |
I think this problem is resolved, and they are same behavirour now. |
modules/python/src2/hdr_parser.py
Outdated
| @@ -595,6 +602,8 @@ def parse_func_decl(self, decl_str, mat="Mat", docstring=""): | |||
|
|
|||
| if arg_type == "InputArray": | |||
| arg_type = mat | |||
| if is_arithm_op_func and arg_name in { "src1", "src2" }: | |||
There was a problem hiding this comment.
This check arg_name in { "src1", "src2" } is really necessary?
Name of the argument shouldn't impact its properties.
There was a problem hiding this comment.
I belive it is needed. Because these functions have not only src1 src2 but also mask as InputArray.
I think that the mask image should be removed from the correction target.
https://docs.opencv.org/4.x/d2/de8/group__core__array.html#ga10ac1bfb180e2cfda1701d06c24fdbd6
void cv::add (
InputArray src1,
InputArray src2,
OutputArray dst,
InputArray mask = noArray(),
int dtype = -1
)
Python:
cv.add( src1, src2[, dst[, mask[, dtype]]] ) -> dst
There was a problem hiding this comment.
It's true, but the solution looks very fragile at this point, because it relies on things that cannot be checked rather during runtime nor generation time...
There was a problem hiding this comment.
Thank you for your review, I don't think there's much of some problem without checking for arg_names.
Due to future implications, I will remove the verification code.
|
It looks like |
|
For the same test code, the test results are inconsistent between Python2 (NG) and Python3 (OK). memo: However, on linux this test didn't work well with python2 and python3 too. I'll investigate tommorow. |
|
I hope it works well. Test failed on Linux64Currently, precommit test server for linux64 is running with Ubuntu 16.04 And it uses numpy 1.11.0, it doesn't include following fix. So np.rad() causes fail. I replaced to np.append() and np.fill(). In my ubuntu 16.04 environment, it is OK. Test failed on python2The behavious of For example, 5 / 2
I think a very simple workaround would be to multiply by 1.0 and not do integer arithmetic. |
|
It seems that calicuration results are differ between release and debug. Maybe they has different optimization levels. I'll continue to investigate. |
|
I am very sorry that my investigation took so long. The inconsistency of calculation results in the Neon implementation of cv::divide() needs to be resolved. For example, In addition, when dividing by 0 in armv6(arm11) /armv7(cortex a17), there is also a problem that the element of dst becomes 255 instead of 0, unlike the description in the specification. If After the situation is sorted out, I would like to issue a separate issue or comment on a related issue. |
|
@Kumataro thank you for investigation. |
|
Thank you and I'm sorry for waiting. I pushed it for it. |
|
Hi, CI tells us this PR increases the build time. I compare "Build OpenCV Contrib" time for this PR and other PR . The correction that comes to mind is that this PR contains changing Is there anything I can do ? This PR) https://github.com/opencv/opencv/actions/runs/6047365813/job/16417304163?pr=24074
|
|
@VadimLevin @Kumataro I re-triggered the build and everything pass now. What are the next steps here? |
Python: support tuple src for cv::add()/subtract()/... opencv#24074 fix opencv#24057 ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] 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 - [ x The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [x] The feature is well documented and sample code can be built with the project CMake
fix #24057
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.