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

[DIP] Add benchmark for Buddy & OpenCV resize2d operation. #61

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

taiqzheng
Copy link
Contributor

@taiqzheng taiqzheng commented May 13, 2023

The command line:
./image-processing-resize-benchmark <image path> <Scale option> <RowNum> <ColNum> <InterpolationOption>

  • image path provides path of the image to be processed.
  • Scale option available are SCALE_FACTOR, SCALE_LENGTH.
  • RowNum and ColNum are the scale_factors/scale_length for row and col.
  • Interpolation option available are NEAREST_NEIGHBOUR_INTERPOLATION, BILINEAR_INTERPOLATION.

An example:
./image-processing-resize-benchmark ../../benchmarks/ImageProcessing/Images/YuTu.png SCALE_FACTOR 0.33 0.35 NEAREST_NEIGHBOUR_INTERPOLATION

Problem:
When using SCALE_FACTOR option, the output.size should be [input.row * RowNum, input.col * ColNum], but result shape ended in [input.row / RowNum, input.col / ColNum]. In the benchmark implementation, the factors(factorsOutputBuddyResize2D) were sent directly to the func below:
dip::Resize2D(&input, dip::INTERPOLATION_TYPE::NEAREST_NEIGHBOUR_INTERPOLATION, factorsOutputBuddyResize2D)
So, the problem was not caused by this implementation.

@meshtag
Copy link
Member

meshtag commented May 21, 2023

When using SCALE_FACTOR option, the output.size should be [input.row * RowNum, input.col * ColNum], but result shape ended in [input.row / RowNum, input.col / ColNum].

I think the latter is the expected behaviour. The way scaling ratios are defined is

scaling_ratio = Input_Image_Dimension / Output_Image_Dimension

So one would expect

Output_Image_Dimension = Input_Image_Dimension / scaling_ratio

This can be referenced from this comment here.

Copy link
Member

@meshtag meshtag left a comment

Choose a reason for hiding this comment

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

I am busy with my final semester exams atm, will do a more in-depth review as soon as I get some time.

Thanks for your work!

benchmarks/ImageProcessing/BuddyResize2DBenchmark.cpp Outdated Show resolved Hide resolved
benchmarks/ImageProcessing/CMakeLists.txt Outdated Show resolved Hide resolved
benchmarks/ImageProcessing/CMakeLists.txt Outdated Show resolved Hide resolved
benchmarks/ImageProcessing/CMakeLists.txt Show resolved Hide resolved
@taiqzheng
Copy link
Contributor Author

This benchmark will compare the resize operation for both Buddy & OpenCV implementation. Here is the problem, when using the SCALE_FACTOR option,Buddy used division operation;OpenCV used multiplication operation.

The command I use:
./image-processing-resize-benchmark ../../benchmarks/ImageProcessing/Images/YuTu.png SCALE_FACTOR 0.5 0.5 NEAREST_NEIGHBOUR_INTERPOLATION

Result for OpenCV, 513*513 pixels.
OpenCVResize_0 5_0 5_NNI

Result for Buddy, 2052*2052 pixels.
BuddyResize_0 5_0 5_NNI

@taiqzheng
Copy link
Contributor Author

taiqzheng commented May 21, 2023

Another problem, when using SCALE_LENGTH option with the command like this:
./image-processing-resize-benchmark ../../benchmarks/ImageProcessing/Images/YuTu.png SCALE_LENGTH 400 300 NEAREST_NEIGHBOUR_INTERPOLATION
OR like this:
./image-processing-resize-benchmark ../../benchmarks/ImageProcessing/Images/YuTu.png SCALE_LENGTH 400 300 BILINEAR_INTERPOLATION

Result for OpenCV contains 400 * 300 pixels, result for Buddy contains 300 * 400 pixels. The dimension order is incorrect when handling with SCALE_LENGTH option. I also tried the SCALE_FACTOR option, this one have the right dimension order with both interpolation option.

@taiqzheng taiqzheng force-pushed the resize_dev branch 2 times, most recently from 2911725 to e17d20d Compare May 21, 2023 06:33
@taiqzheng taiqzheng changed the title [DIP] Add benchmark for buddy resize2d operation. [DIP] Add benchmark for Buddy & OpenCV resize2d operation. May 21, 2023
@meshtag
Copy link
Member

meshtag commented May 29, 2023

Here is the problem, when using the SCALE_FACTOR option,Buddy used division operation;OpenCV used multiplication operation.

Result for OpenCV contains 400 * 300 pixels, result for Buddy contains 300 * 400 pixels.

Both of these things can be changed if you want it to become exactly like OpenCV. Shall I change them?

@taiqzheng
Copy link
Contributor Author

taiqzheng commented May 31, 2023

Here is the problem, when using the SCALE_FACTOR option,Buddy used division operation;OpenCV used multiplication operation.

Result for OpenCV contains 400 * 300 pixels, result for Buddy contains 300 * 400 pixels.

Both of these things can be changed if you want it to become exactly like OpenCV. Shall I change them?

I highly recommend to modify this part, thanks a lot!
This will make the benchmark design more concise and understandable without ambiguity.

@taiqzheng taiqzheng requested a review from meshtag June 6, 2023 16:15
@taiqzheng taiqzheng marked this pull request as draft June 8, 2023 06:53
@taiqzheng
Copy link
Contributor Author

Will delete the OpenCV enum class after exams(before the weekend).

@meshtag
Copy link
Member

meshtag commented Jun 12, 2023

Please mark the PR as "Ready for review" once you think it is ready.

- Use clang-format for both resize benchmark files.
@taiqzheng taiqzheng marked this pull request as ready for review June 12, 2023 08:14
@meshtag
Copy link
Member

meshtag commented Jun 16, 2023

I think it makes sense to benchmark this function directly instead of the user interface itself.

The user interface is just a single layer of syntactic sugar which makes it easier for users to utlize the underlying implementation without worrying about the output. It also involves costly "pass by value" situations which are very bad from the performance perspective. I wish to highlight the fact that this is not inherently required by the implementation itself, but is just added for more user convenience.

Same goes for #63 as well.

What is your opinion on this? (/cc @zhanghb97 @FlagerLee )

EllisLambda pushed a commit to EllisLambda/buddy-benchmark that referenced this pull request Sep 21, 2023
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.

2 participants