Skip to content

Conversation

@kuloPo
Copy link
Contributor

@kuloPo kuloPo commented Oct 26, 2021

This function calculates the Hough Space of a given image in any range.
Hough Transform is a discrete implementation of Radon Transform.

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

* with type CV_32SC1 by default.
*
*/
CV_EXPORTS_W void HoughSpaceTransform(InputArray src,
Copy link
Member

Choose a reason for hiding this comment

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

What is wrong with HoughLines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HoughLines only returns lines detected in images. Hough Space can be used in other ways like comparing similarity between images, and it has some advantages over comparing histograms or pixels directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a search I found Hough Detection is using Radon Transform to detect shape in image. (Described here https://www.osti.gov/doepatents/biblio/4746348 ). What I implemented is more like Radon Transform (https://en.wikipedia.org/wiki/Radon_transform)
Should I change my function name and commit to the same pull request?

@kuloPo kuloPo requested a review from alalek October 28, 2021 05:52
@AleksandrPanov
Copy link
Contributor

AleksandrPanov commented Nov 1, 2021

I compared the current version of Radon Transform with the scikit-image radon version based on scikit-image example.

  • The current version always crops the image when it is rotated.
  • The current version gives a slightly different result compared to the scikit-image version (this may be due to different interpolation or different rounding.). Need add more tests to check the meaning of the result (for example tests from radon.py.txt).
  • It might be helpful to add CV_32FC1 for src/dst.

radon.py.txt
radon

@kuloPo kuloPo changed the title Add hough space transform function to ximgproc Add Radon transform function to ximgproc Nov 7, 2021
@kuloPo
Copy link
Contributor Author

kuloPo commented Nov 7, 2021

Updates:

  • Change function name to RadonTransform
  • Fix always cropping corner when rotate
  • transpose the output - now each column is the integral
  • add support for float number

The small difference may caused by:

  • rounding from rad to deg in scikit
  • different rotation method
  • copying image to a bigger background to capture the rotated image

I wrote two more cases to test data in corners
radon.py.txt

@AleksandrPanov
Copy link
Contributor

AleksandrPanov commented Nov 8, 2021

Thank you, I think it got better after your update:
radon2
:

@AleksandrPanov
Copy link
Contributor

LGTM + minor comments:

  • this patch should go into 4.x branch (only bug fixes get into 3.4 branch).

  • rebase your commits from 3.4 onto 4.x branch. For example:
    git rebase -i --onto upstream/3.4 upstream/master
    (check list of your commits, save and quit (Esc + "wq" + Enter)
    where upstream is configured by following this GitHub guide and fetched (git fetch upstream).

  • push rebased commits into source branch of your fork (with --force option)

Note: no need to re-open PR, apply changes "inplace".

@kuloPo kuloPo force-pushed the add-hough_space_transform branch from 2a7f0ba to 36275ca Compare November 8, 2021 22:22
@kuloPo kuloPo changed the base branch from 3.4 to 4.x November 8, 2021 22:23
@kuloPo
Copy link
Contributor Author

kuloPo commented Nov 8, 2021

The branch is rebased to 4.x

Copy link
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.

@kuloPo Thank you for updates!

Please take a look on the comments below.

namespace opencv_test {
namespace {

typedef tuple<Size, MatType> RadonTransformPerfTestParam;
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid indentation in namespaces


int main() {
cv::Mat src = cv::imread("peilin_plane.png", cv::IMREAD_GRAYSCALE);
cv::Mat radon;
Copy link
Member

Choose a reason for hiding this comment

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

Please use 4 spaces for indentation (no tabs)

#include <opencv2/ximgproc/radon_transform.hpp>

int main() {
cv::Mat src = cv::imread("peilin_plane.png", cv::IMREAD_GRAYSCALE);
Copy link
Member

Choose a reason for hiding this comment

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

cv::

In OpenCV samples we add using namespace cv; after includes and removing all cv:: prefixes.

Mat radon;
cv::ximgproc::RadonTransform(src, radon);

ASSERT_EQ(radon.rows, 363);
Copy link
Member

Choose a reason for hiding this comment

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

_EQ

uses different order of parameters: (expected, actual).

Signature for _EQ is ASSERT_EQ(expected_reference, actual_result);.
Correct order of parameters are required to emit valid error messages.

Reference: https://github.com/opencv/opencv/blob/4.0.0/modules/ts/include/opencv2/ts/ts_gtest.h#L8196-L8200

GTEST_API_ AssertionResult EqFailure(const char* expected_expression,
                                     const char* actual_expression,
                                     const std::string& expected_value,
                                     const std::string& actual_value,
                                     bool ignoring_case);


declare.in(src, WARMUP_RNG);

TEST_CYCLE_N(3)
Copy link
Member

Choose a reason for hiding this comment

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

TEST_CYCLE_N

is misused here. No need to specify amount of inner samples here.

Please use:

  • TEST_CYCLE()
  • or PERF_SAMPLE_BEGIN() + PERF_SAMPLE_END()

Mat radon;
cv::ximgproc::RadonTransform(src, radon);

ASSERT_EQ(radon.at<int>(0, 0), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Before using .at<int> it makes sense to validate type.

ASSERT_EQ(CV_32SC1, radon.type());

EXPECT_EQ(0, radon.at<int>(0, 0));

(prefer to use EXPECT_ for non-critical failures - e.g. code below would not crash)

@kuloPo
Copy link
Contributor Author

kuloPo commented Nov 9, 2021

All the issues are fixed.

Copy link
Contributor

@AleksandrPanov AleksandrPanov left a comment

Choose a reason for hiding this comment

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

👍

@alalek alalek merged commit e94ec40 into opencv:4.x Nov 9, 2021
@alalek alalek mentioned this pull request Dec 30, 2021
@alalek alalek mentioned this pull request Feb 22, 2022
@mshabunin mshabunin mentioned this pull request Dec 6, 2024
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