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

Fix corners #6

Merged
merged 2 commits into from
Aug 29, 2018
Merged

Fix corners #6

merged 2 commits into from
Aug 29, 2018

Conversation

scott-trinkle
Copy link
Contributor

Addresses issue #5.

Slicing image.shape with [0:1] only selects the first dimension. So all four elements of the corners array were always scaled by the first dimension of the image, resulting in a potentially incorrect value for maxRadius depending on the center, which also leads to different image sizes for the polar image. Slicing with [0:2] uses both image dimensions and creates accurate corners values and fixes these issues.

I created a test, TestPolarConversion.test_final_radius(), that fails with the old slicing and succeeds with the new. With this particular image, the center used in test_default ([401, 365]) leads to the same value independent of slicing, so I used an arbitrary center of [350, 365] instead.

I noticed all of the tests were failing because the "horizontalLines.png" and "checkerboard.png" images are not included in the repository, so I commented those out.

Also, the indentation of the main function call in "test_polarTransform.py" was indented under theTestPointConversion class, so I don't think all of the tests were running. I changed that as well.

Let me know if you see any issues here!

@addisonElliott
Copy link
Owner

LGTM 👍

I am going to enable Travis CI and try to fix the tests issue so it's hopefully easier to test changes.

I made this library in like a week for a project. As more and more people find use in it, my motivation to maintain it will increase :)

Eventually I would like to make the tests a bit better and more comprehensive.

@addisonElliott addisonElliott merged commit 0216611 into addisonElliott:master Aug 29, 2018
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