add init params for Ellipse & Circle and RANSAC #2415

Open
wants to merge 4 commits into
from

Projects

None yet

5 participants

@Borda
Contributor
Borda commented Dec 24, 2016 edited

Description

Adding option to use some init parameters for model fitting and RANDSAC initial inliers, eg, you have some a priory knowledge, you do not want to use some default parameters.

NOTE: this is copy of last version PR #2371 (because I had problems with rebase and made some mistakes)

Checklist

@Borda Borda changed the title from copy last version from PR #2371 to add init params for Ellipse & Circle and RANSAC Dec 24, 2016
@codecov-io
codecov-io commented Dec 24, 2016 edited

Current coverage is 90.69% (diff: 94.87%)

Merging #2415 into master will increase coverage by 0.01%

@@             master      #2415   diff @@
==========================================
  Files           304        304          
  Lines         21417      21442    +25   
  Methods           0          0          
  Messages          0          0          
  Branches       1838       1845     +7   
==========================================
+ Hits          19421      19446    +25   
  Misses         1655       1655          
  Partials        341        341          

Powered by Codecov. Last update 89ca79b...bec72b5

skimage/measure/fit.py
"""Estimate circle model from data using total least squares.
Parameters
----------
data : (N, 2) array
N points with ``(x, y)`` coordinates, respectively.
+ init_params : tuple of 3 values, optional
+ (x_center, y_center, r) is initial guess of parameters.
+ If they are not specified initial guess use a circle model.
@sciunto
sciunto Dec 24, 2016 Member

If None, use a circle with parameters based on mean values.

skimage/measure/fit.py
@@ -378,14 +378,21 @@ def Dfun(params):
A[0, :] = -(x - xc) / d
A[1, :] = -(y - yc) / d
# same for all iterations, so not changed in each iteration
- #A[2, :] = -1
+ # A[2, :] = -1
@sciunto
sciunto Dec 24, 2016 Member

Why this commented line?

@Borda
Borda Dec 24, 2016 edited Contributor

no clue, it was there already before... I will remove it.

@sciunto
sciunto Dec 24, 2016 Member

Got it. Please rewrite the comment as

# A[2, :] = -1 is the same for all iterations, not re-affected.
skimage/measure/fit.py
+ xc0 = x.mean()
+ yc0 = y.mean()
+ r0 = dist(xc0, yc0).mean()
+ init_params = (xc0, yc0, r0)
@sciunto
sciunto Dec 24, 2016 Member

Concatenate these 4 lines into a single one.

skimage/measure/fit.py
"""Estimate circle model from data using total least squares.
Parameters
----------
data : (N, 2) array
N points with ``(x, y)`` coordinates, respectively.
+ init_params : tuple of 5 values, optional
+ (x_center, y_center, a, b, theta) is initial guess of parameters.
+ If they are not specified initial guess use a circle model.
@sciunto
sciunto Dec 24, 2016 Member

If None, use a circle with parameters based on mean values.

skimage/measure/fit.py
@@ -770,7 +786,8 @@ def ransac(data, model_class, min_samples, residual_threshold,
If RandomState instance, random_state is the random number generator;
If None, the random number generator is the RandomState instance used
by `np.random`.
-
+ init_inliers : [list, tuple of] (N) array of bool or None, optional
+ Initial samples selection for model estimation
@sciunto
sciunto Dec 24, 2016 Member

estimation.

skimage/measure/fit.py
+ if not (0 < min_samples <= 1):
+ raise ValueError("`min_samples` as ration must be in range (0, 1)")
+ min_samples = int(min_samples * len(data))
+ if min_samples <= 0:
@sciunto
sciunto Dec 24, 2016 Member

This is inconsistent with the docstring (zero included).

@sciunto
Member
sciunto commented Dec 24, 2016

Can you squash your commits and then edit the commit message to be explicit, ie add init params for Ellipse & Circle and RANSAC

@Borda
Contributor
Borda commented Dec 24, 2016

squash commits is nice feature I have not used before... :)

skimage/measure/fit.py
random_state = check_random_state(random_state)
+ if isinstance(min_samples, float):
+ if not (0 < min_samples <= 1):
@sciunto
sciunto Dec 26, 2016 Member

0 <= min_samples <= 1

@sciunto
Member
sciunto commented Dec 26, 2016

looks correct to me, but I'm not using this feature. Anyone to review this PR?

@Borda
Contributor
Borda commented Dec 26, 2016

Does anyone have an idea why it fails with "The build has been terminated" even on my personal branch Borda / scikit-image it passes?

@soupault
Member

@Borda I guess it just got overwhelmed :).

@Borda Borda add init params for Ellipse & Circle and RANSAC
note, copy last version from PR #2371

fix decimal imprecision -> np.round(, decimals=2)
review update
update explanation for A[2, :] = -1 in estimate CircleModel
update range for min number
5c62f8e
@Borda
Contributor
Borda commented Jan 2, 2017

@soupault @sciunto do you have anything else to this review?

@sciunto sciunto added this to the 0.14 milestone Jan 2, 2017
@ahojnnes
Member
ahojnnes commented Jan 2, 2017

I have some concerns regarding this contribution:

  1. It is not straightforward what to do when there are a priori inliers in RANSAC. The chosen strategy is certainly not "optimal" and could simply be replaced by 2 or 3 lines of code manually, if it is really needed. I am against adding this feature.
  2. The documentation should motivate why init_params is useful (non-linear, iterative estimation). Currently, it is not clear what the purpose of the initialization is.
@Borda
Contributor
Borda commented Jan 2, 2017

@ahojnnes regarding the 2) you are talking about description of init_params for RANSAC or also Circle? And do you mean adding just a short description or also an example?

@ahojnnes
Member
ahojnnes commented Jan 3, 2017

@Borda I mean to extend the doc string for init_params with a short one sentence description why a good set of starting parameters is useful (for convergence...).

Borda added some commits Jan 3, 2017
@Borda Borda Merge branch 'master' into init_params_fit_models
Conflicts:
	skimage/measure/fit.py
	skimage/measure/tests/test_fit.py
b575301
@Borda Borda fix not init for ellipse
d4f33d5
@Borda Borda add assert for missing params
bec72b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment