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.

Sign up for GitHub

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

Jump to bottom

fix rotating for ellipse fit #2482

Merged
merged 1 commit into from
Feb 21, 2017
Merged

fix rotating for ellipse fit #2482

merged 1 commit into from
Feb 21, 2017

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Jan 30, 2017 •
edited

Description

fixing issue with estimated orientation, the equation was not implemented properly

Checklist

References

http://mathworld.wolfram.com/Ellipse.html#eqn23

@Borda Borda changed the title fix for rotating ellipses & covered by tests fix rotating for ellipse fit Jan 30, 2017
@sciunto sciunto added the bug label Jan 31, 2017
@sciunto sciunto added this to the 0.13 milestone Jan 31, 2017
@Borda Borda force-pushed the test_ellipse_fit branch 4 times, most recently from 55128a0 to 36a1614 Compare January 31, 2017 18:50
sciunto
sciunto reviewed Jan 31, 2017
View reviewed changes
skimage/measure/fit.py Outdated
try:
M = np.linalg.inv(C1).dot(
S1 - np.dot(S2, np.linalg.inv(S3)).dot(S2.T))
except: # LinAlgError: Singular matrix
Copy link
Member

Choose a reason for hiding this comment

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

Please, be more strict on the exception you catch. Otherwise, you'll catch everything and errors can't be detected.

Copy link
Member

Choose a reason for hiding this comment

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

Could you had a test that make sure that the exception will be raised and the final behavior is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, the are two points, it anything happens during slowing this matrix multiplication, goes to not proper ellipse fitting and then the estimate fails anyway... second, I found this instability while testing ransac on real data so isolating the special case is possible but not common... :)

skimage/measure/fit.py Outdated
@@ -515,7 +528,11 @@ def estimate(self, data):
C1 = np.array([[0., 0., 2.], [0., -1., 0.], [2., 0., 0.]])

# Reduced scatter matrix [eqn. 29]
Copy link
Member

Choose a reason for hiding this comment

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

move this comment after the try statement.

@sciunto
Copy link
Member

sciunto commented Jan 31, 2017

Has the bug been introduced in your last patch? namely

commit d07592c
Date: Fri Dec 23 13:02:19 2016 +0100

@Borda
Copy link
Contributor Author

Borda commented Jan 31, 2017

excuse me, not sure what you mean by have been introduced in..
I made the patch partially using an existing code, but due to not complete testing from previous code coverage the issue was no addressed, I found it on further usage, so I extended the testing and corrected code in this fix.

jni
jni reviewed Feb 3, 2017
View reviewed changes
Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

@Borda thanks for the fix! I've made some minor comments, should be quick to address!

skimage/measure/fit.py Outdated
xc, yc, a, b, theta = params
if isinstance(theta, complex):
theta = np.real(theta)
Copy link
Member

Choose a reason for hiding this comment

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

I just saw that np.real returns a 0-dimensional array. Do we want to cast to float? And do we want to do this anyway? It might be faster to pipe all values through float(np.real(theta)) than it is to check first. Thoughts?

skimage/measure/fit.py Outdated
-------

>>> xy = EllipseModel().predict_xy(np.linspace(0, 2 * np.pi, 25),
... params=(10, 15, 4, 8, np.deg2rad(30)))
Copy link
Member

Choose a reason for hiding this comment

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

Minor: fix indent to match open paren

@Borda Borda force-pushed the test_ellipse_fit branch 5 times, most recently from f9a170e to d60ebe5 Compare February 5, 2017 11:01
sciunto
sciunto reviewed Feb 14, 2017
View reviewed changes
skimage/measure/fit.py Outdated
@@ -470,6 +470,19 @@ class EllipseModel(BaseModel):
Ellipse model parameters in the following order `xc`, `yc`, `a`, `b`,
`theta`.

Example
Copy link
Member

Choose a reason for hiding this comment

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

Examples

@sciunto
Copy link
Member

sciunto commented Feb 14, 2017

I looked at the history, the answer to my question is yes. So we should merge it for 0.13.

@codecov-io
Copy link

codecov-io commented Feb 14, 2017 •
edited

Codecov Report

Merging #2482 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2482      +/-   ##
==========================================
+ Coverage   90.74%   90.76%   +0.01%     
==========================================
  Files         304      304              
  Lines       21665    21678      +13     
  Branches     1867     1870       +3     
==========================================
+ Hits        19660    19676      +16     
+ Misses       1662     1661       -1     
+ Partials      343      341       -2
Impacted Files Coverage Δ
skimage/measure/tests/test_fit.py 99.45% <100%> (+0.02%) ✅
skimage/measure/fit.py 82.28% <100%> (+1.63%) ✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e61d741...d61e810. Read the comment docs.

@Borda
Copy link
Contributor Author

Borda commented Feb 14, 2017

@sciunto @jni corrected according your suggestions :)

jni
jni reviewed Feb 15, 2017
View reviewed changes
skimage/measure/fit.py Outdated
@@ -470,6 +470,19 @@ class EllipseModel(BaseModel):
Ellipse model parameters in the following order `xc`, `yc`, `a`, `b`,
`theta`.

Examples
-------
Copy link
Member

Choose a reason for hiding this comment

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

Travis is unhappy about this docstring — I'm not sure if it's because there is a missing - at the end to make the dashes' width equal to that of "Examples".

@Borda
fix for rotating ellipses & covered by tests
d61e810 
* fix crashes
* add doctest example
* update coverage
* update review
* update complex
@stefanv stefanv merged commit d136942 into scikit-image:master Feb 21, 2017
@stefanv
Copy link
Member

stefanv commented Feb 21, 2017

Good work, thanks!

@Borda Borda deleted the test_ellipse_fit branch February 21, 2017 06:26
@stefanv stefanv mentioned this pull request May 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sciunto sciunto sciunto left review comments

@jni jni jni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Borda @sciunto @codecov-io @stefanv @jni