fix rotating for ellipse fit by Borda · Pull Request #2482 · scikit-image/scikit-image

Borda · 2017-01-30T20:02:58Z

Description

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

Checklist

References

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

sciunto · 2017-01-31T19:56:59Z

skimage/measure/fit.py

+        try:
+            M = np.linalg.inv(C1).dot(
+                S1 - np.dot(S2, np.linalg.inv(S3)).dot(S2.T))
+        except:  # LinAlgError: Singular matrix


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

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

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

sciunto · 2017-01-31T19:59:05Z

skimage/measure/fit.py

@@ -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]


move this comment after the try statement.

sciunto · 2017-01-31T20:02:18Z

Has the bug been introduced in your last patch? namely

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

Borda · 2017-01-31T23:42:13Z

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

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

jni · 2017-02-03T05:10:14Z

skimage/measure/fit.py

+
        xc, yc, a, b, theta = params
+        if isinstance(theta, complex):
+            theta = np.real(theta)


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?

jni · 2017-02-03T05:15:10Z

skimage/measure/fit.py

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


Minor: fix indent to match open paren

sciunto · 2017-02-14T20:10:57Z

skimage/measure/fit.py

        Ellipse model parameters in the following order `xc`, `yc`, `a`, `b`,
        `theta`.

+    Example


sciunto · 2017-02-14T20:11:29Z

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

codecov-io · 2017-02-14T20:37:24Z

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 · 2017-02-14T20:37:43Z

@sciunto @jni corrected according your suggestions :)

jni · 2017-02-15T03:30:20Z

skimage/measure/fit.py

        `theta`.

+    Examples
+    -------


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".

* fix crashes * add doctest example * update coverage * update review * update complex

stefanv · 2017-02-21T01:23:05Z

Good work, thanks!

Borda changed the title ~~fix for rotating ellipses & covered by tests~~ fix rotating for ellipse fit Jan 30, 2017

sciunto added the bug label Jan 31, 2017

sciunto added this to the 0.13 milestone Jan 31, 2017

Borda force-pushed the test_ellipse_fit branch 4 times, most recently from 55128a0 to 36a1614 Compare January 31, 2017 18:50

sciunto reviewed Jan 31, 2017

View reviewed changes

Borda force-pushed the test_ellipse_fit branch from 36a1614 to 3cb694c Compare February 2, 2017 19:44

jni reviewed Feb 3, 2017

View reviewed changes

Borda force-pushed the test_ellipse_fit branch 5 times, most recently from f9a170e to d60ebe5 Compare February 5, 2017 11:01

sciunto reviewed Feb 14, 2017

View reviewed changes

skimage/measure/fit.py Outdated

Ellipse model parameters in the following order `xc`, `yc`, `a`, `b`,

`theta`.

Example

Copy link

Member

sciunto Feb 14, 2017

Choose a reason for hiding this comment

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

Examples

Borda force-pushed the test_ellipse_fit branch from d60ebe5 to 4d6f110 Compare February 14, 2017 20:36

jni reviewed Feb 15, 2017

View reviewed changes

fix for rotating ellipses & covered by tests

d61e810

* fix crashes * add doctest example * update coverage * update review * update complex

Borda force-pushed the test_ellipse_fit branch from 4d6f110 to d61e810 Compare February 15, 2017 16:40

jni approved these changes Feb 20, 2017

View reviewed changes

stefanv merged commit d136942 into scikit-image:master Feb 21, 2017

Borda deleted the test_ellipse_fit branch February 21, 2017 06:26

stefanv mentioned this pull request May 2, 2024

Set LDFLAGS and CPPFLAGS for keg-only libomp in CI config #7407

Closed

		@@ -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]

Uh oh!

Conversation

Borda commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sciunto commented Jan 31, 2017

Uh oh!

Borda commented Jan 31, 2017

Uh oh!

jni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sciunto commented Feb 14, 2017

Uh oh!

codecov-io commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Borda commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanv commented Feb 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Borda commented Jan 30, 2017 •

edited

Loading

codecov-io commented Feb 14, 2017 •

edited

Loading