Skip to content

Allow src=float, dst=double combination for filter2D#22256

Closed
catree wants to merge 1 commit intoopencv:3.4from
catree:fix_22242_filter2d_float_double
Closed

Allow src=float, dst=double combination for filter2D#22256
catree wants to merge 1 commit intoopencv:3.4from
catree:fix_22242_filter2d_float_double

Conversation

@catree
Copy link
Copy Markdown
Contributor

@catree catree commented Jul 16, 2022

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

Try to fix #22242

Reading the doc, I also understand that CV_32F / CV_64F combination should be allowed.

Comment on lines +3291 to +3293
if( sdepth == CV_32F && ddepth == CV_64F )
return makePtr<Filter2D<float,
Cast<double, double>, FilterNoVec> >(kernel, anchor, delta);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OpenCV library should stay efficient.
Current binary code is really large. We should not explode it by adding of extra variants.

OpenCV has 7+ basic types. So we still need to have 7 variants of implementations.
The most part of operations doesn't allow to mix input/output types - this is done for a reason.
Support of mixed types would make problem with binary size much worse:

  • 2 inputs => 7*7 = 49 variants
  • 3 different inputs => 343 variants

So, there is a policy to avoid processing of mixed types. Or process them through conversion to common "working" type.
If user want double accuracy, then convert inputs to doubles first.

I believe we should fix the documentation instead.

/cc @vpisarev @asmorkalov

@vpisarev
Copy link
Copy Markdown
Contributor

@catree, I would agree with @alalek. Do we have a real use case for such a combination? Normally, filters are not huge and so the accumulation rounding error is comparable with the accuracy that F32 delivers. Is it possible to use a combination cv::filter2D() and Mat::convertTo()?

@catree
Copy link
Copy Markdown
Contributor Author

catree commented Jul 16, 2022

@alalek @vpisarev

Thanks for the explanations.
I will open another PR later and close this one after. Instead I will add a note for this special case or fix the documentation if CV_32F / CV_64F is not supported for all the functions on this page.

@catree
Copy link
Copy Markdown
Contributor Author

catree commented Aug 12, 2022

Replaced by #22370

@catree catree closed this Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants