Skip to content

Relax accuracy requirements in the OpenCL sqrt perf arithmetic test.#19810

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
aarongreig:aaron/core/relaxClArithmTest
Apr 6, 2021
Merged

Relax accuracy requirements in the OpenCL sqrt perf arithmetic test.#19810
opencv-pushbot merged 1 commit intoopencv:3.4from
aarongreig:aaron/core/relaxClArithmTest

Conversation

@aarongreig
Copy link
Copy Markdown
Contributor

Also bring perf_imgproc CornerMinEigenVal accuracy requirements in line with
the test_imgproc accuracy requirements on that test and fix indentation on
the latter.

Partially addresses issue #9821

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 other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to 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


SANITY_CHECK(dst, 1e-6, ERROR_RELATIVE);
if (ocl::Device::getDefault().isIntel())
Near(1e-6, true);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is it intentional that this is different from the number in test_imgproc.cpp?

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.

Yes.
Perf tests should not verify accuracy in the accurate way (or even they should not perform that at all)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is checking for more accuracy than the test_imgproc.cpp test, I can make the number here the same as test_imgproc? or I could remove the if and leave only SANITY_CHECK(dst, 0.1, ERROR_RELATIVE);

@aarongreig aarongreig force-pushed the aaron/core/relaxClArithmTest branch 2 times, most recently from 1946458 to 4d2e035 Compare March 31, 2021 10:11
// defined accuracy. We know intel devices have accurate native_sqrt, but
// otherwise stick to a relaxed sanity check. For types larger than 32 bits
// we can do the accuracy check for all devices as normal.
if ((CV_MAT_DEPTH(type) == CV_32F && ocl::Device::getDefault().isIntel()) ||
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.

Need to properly handle case with disabled OpenCL (use strict variant of check tolerance):

#ifdef HAVE_OPENCL
bool strictCheck = !ocl::useOpenCL() || ocl::Device::getDefault().isIntel();
#else
bool strictCheck = true;
#endif
if (... strictCheck ...)
    strict;
else
    relax;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

looks like this whole file is wrapped in #ifdef HAVE_OPENCL

Copy link
Copy Markdown
Member

@alalek alalek Apr 1, 2021

Choose a reason for hiding this comment

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

ocl::useOpenCL() check is still necessary. #ifdef HAVE_INTEL doesn't help in all cases (compile with OpenCL, but run with OPENCV_OPENCL_RUNTIME=disabled)

In such scenario CPU code is run, so we need to apply strict "old" check.
Similar regression is introduced by this PR: #19793

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Aahh makes sense, sorry about that. Should be fixed now

@aarongreig aarongreig force-pushed the aaron/core/relaxClArithmTest branch from 4d2e035 to 08e8de6 Compare April 1, 2021 15:12
@aarongreig aarongreig force-pushed the aaron/core/relaxClArithmTest branch from 08e8de6 to bf8860f Compare April 6, 2021 10:29
Also bring perf_imgproc CornerMinEigenVal accuracy requirements in line with
the test_imgproc accuracy requirements on that test and fix indentation on
the latter.

Partially addresses issue opencv#9821
@aarongreig aarongreig force-pushed the aaron/core/relaxClArithmTest branch from bf8860f to f3f4609 Compare April 6, 2021 16:33
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you 👍

@opencv-pushbot opencv-pushbot merged commit 3a81540 into opencv:3.4 Apr 6, 2021
This was referenced Apr 8, 2021
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.

3 participants