Skip to content

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

Merged
alalek merged 4 commits intoopencv:3.4from
everton1984:new_integral_tests
Sep 4, 2019
Merged

Adding all possible data type interactions to the perf tests since some use SIMD acceleration and others do not.#15440
alalek merged 4 commits intoopencv:3.4from
everton1984:new_integral_tests

Conversation

@everton1984
Copy link
Copy Markdown
Contributor

@everton1984 everton1984 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.

CV_32F64F,
CV_64F64F};

CV_ENUM(SqSizes, CV_32S32S, CV_32S32F, CV_32S64F, CV_32F32F, CV_32F64F, CV_64F64F);
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.

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::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)
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.

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

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

SANITY_CHECK(sum, 1e-6);
SANITY_CHECK(sqsum, 1e-6);
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.

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

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.

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

performance tests for the integral function.

SANITY_CHECK_NOTHING();
SANITY_CHECK_NOTHING();

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.

One macro should be enough

testing::Values(CV_8UC1, CV_8UC2, CV_8UC3, CV_8UC4),
testing::Values(CV_32S, CV_32F, CV_64F)
)
)
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.

Lets avoid unrelated changes.

SZ_32F64F,
SZ_64F64F};

CV_ENUM(IntegralOutputDepths, SZ_32S32S, SZ_32S32F, SZ_32S64F, SZ_32F32F, SZ_32F64F, SZ_64F64F);
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.

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?

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}};
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.

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
Copy link
Copy Markdown
Contributor Author

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

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 👍

@alalek alalek merged commit 76e403c into opencv:3.4 Sep 4, 2019
@alalek alalek mentioned this pull request Sep 5, 2019
@everton1984 everton1984 deleted the new_integral_tests branch September 9, 2019 16:18
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.

2 participants