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

Adding all possible data type interactions to the perf tests since some use SIMD acceleration and others do not. #15440

Merged
merged 4 commits into from Sep 4, 2019

Conversation

@everton1984
Copy link
Contributor

commented Sep 2, 2019

resolves #15439

This pullrequest changes

This PR is changing modules/imgproc/perf/perf_integral.cpp if you look at sumpixels.cpp there are several possible combinations of matrix sizes for the integral function. The performance tests do not cover all possibilities even with some of them being optimised with HAL instructions so it's impossible to analyse improvements made for those combinations.

everton1984 added 2 commits Sep 2, 2019
CV_32F64F,
CV_64F64F};

CV_ENUM(SqSizes, CV_32S32S, CV_32S32F, CV_32S64F, CV_32F32F, CV_32F64F, CV_64F64F);

This comment has been minimized.

Copy link
@alalek

alalek Sep 3, 2019

Contributor

SqSizes

This doesn't look as "sizes". Perhaps it should be IntergralOutputDepths

Also it is better to replace CV_ prefix to avoid confusion with public OpenCV API.

testing::Combine(
testing::Values(TYPICAL_MAT_SIZES),
testing::Values(CV_8UC1, CV_8UC2, CV_8UC3, CV_8UC4),
testing::Values(CV_32S, CV_32F, CV_64F)
testing::Values(0,1,2,3,4,5)

This comment has been minimized.

Copy link
@alalek

alalek Sep 3, 2019

Contributor

Avoid using of magic numbers. Use enum constants identifiers instead.


declare.in(src, WARMUP_RNG).out(sum, sqsum);
declare.time(100);

TEST_CYCLE() integral(src, sum, sqsum, sdepth);
TEST_CYCLE() integral(src, sum, sqsum, sdepth, sqdepth);

SANITY_CHECK(sum, 1e-6);
SANITY_CHECK(sqsum, 1e-6);

This comment has been minimized.

Copy link
@alalek

alalek Sep 3, 2019

Contributor

You don't have sanity data for this test (in opencv_extra repositry). Replace these 2 lines by SANITY_CHECK_NOTHING();

This comment has been minimized.

Copy link
@everton1984

everton1984 Sep 3, 2019

Author Contributor

Thanks for the feedback, just made the changes you requested.

Giving proper names, removing magic numbers and sanity checks of new
performance tests for the integral function.

SANITY_CHECK_NOTHING();
SANITY_CHECK_NOTHING();

This comment has been minimized.

Copy link
@alalek

alalek Sep 4, 2019

Contributor

One macro should be enough

testing::Values(CV_8UC1, CV_8UC2, CV_8UC3, CV_8UC4),
testing::Values(CV_32S, CV_32F, CV_64F)
)
)

This comment has been minimized.

Copy link
@alalek

alalek Sep 4, 2019

Contributor

Lets avoid unrelated changes.

SZ_32F64F,
SZ_64F64F};

CV_ENUM(IntegralOutputDepths, SZ_32S32S, SZ_32S32F, SZ_32S64F, SZ_32F32F, SZ_32F64F, SZ_64F64F);

This comment has been minimized.

Copy link
@alalek

alalek Sep 4, 2019

Contributor

SZ_

What is the meaning of this?
Depth != Size (size is usually associated with image size in pixels, not a "size" of type)

DEPTH_32S_32S?

@@ -55,6 +68,35 @@ PERF_TEST_P(Size_MatType_OutMatDepth, integral_sqsum,
SANITY_CHECK(sqsum, 1e-6);
}

int vals[6][2] = {{CV_32S, CV_32S}, {CV_32S, CV_32F}, {CV_32S, CV_64F}, {CV_32F, CV_32F}, {CV_32F, CV_64F}, {CV_64F, CV_64F}};

This comment has been minimized.

Copy link
@alalek

alalek Sep 4, 2019

Contributor

This should be close to related enumeration above.
This should be properly named.

This must be "static" or in anonymous namespace due its "local" usage (we don't want conflicts especially on such common name).

@everton1984

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Changed the names per request, made the array static and moved the definition closer to the enum.

@alalek
alalek approved these changes Sep 4, 2019
Copy link
Contributor

left a comment

Looks good to me 👍

@alalek alalek self-assigned this Sep 4, 2019

@alalek alalek added this to the 3.4.8 milestone Sep 4, 2019

@alalek alalek merged commit 76e403c into opencv:3.4 Sep 4, 2019

1 check passed

default Required builds passed
Details
@alalek alalek referenced this pull request Sep 5, 2019

@everton1984 everton1984 deleted the everton1984:new_integral_tests branch Sep 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.