update RANSAC, ratio of inliers #2415

Borda · 2016-12-24T08:52:47Z

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

codecov-io · 2016-12-24T09:59:10Z

Codecov Report

Merging #2415 into master will increase coverage by 4.66%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2415      +/-   ##
=========================================
+ Coverage   86.04%   90.7%   +4.66%     
=========================================
  Files         337     304      -33     
  Lines       27050   21536    -5514     
  Branches        0    1861    +1861     
=========================================
- Hits        23274   19534    -3740     
+ Misses       3776    1660    -2116     
- Partials        0     342     +342

Impacted Files	Coverage Δ
skimage/measure/tests/test_fit.py	`99.43% <100%> (-0.57%)`	⬇️
skimage/measure/fit.py	`81.88% <100%> (-14.58%)`	⬇️
skimage/_shared/tests/test_interpolation.py	`0% <0%> (-100%)`	⬇️
skimage/_shared/tests/test_geometry.py	`0% <0%> (-100%)`	⬇️
skimage/_shared/tests/test_safe_as_int.py	`0% <0%> (-100%)`	⬇️
skimage/_shared/tests/test_version_requirements.py	`0% <0%> (-95.66%)`	⬇️
skimage/_shared/tests/test_testing.py	`0% <0%> (-94.55%)`	⬇️
skimage/_shared/tests/test_utils.py	`0% <0%> (-90%)`	⬇️
skimage/_shared/tests/__init__.py	`0% <0%> (-60%)`	⬇️
skimage/_shared/version_requirements.py	`43.39% <0%> (-41.51%)`	⬇️
... and 265 more

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 d5de268...43b1cbf. Read the comment docs.

sciunto · 2016-12-24T12:31:14Z

skimage/measure/fit.py

            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.


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

sciunto · 2016-12-24T12:31:29Z

skimage/measure/fit.py

            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.


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

sciunto · 2016-12-24T12:31:44Z

skimage/measure/fit.py

            A[1, :] = -(y - yc) / d
            # same for all iterations, so not changed in each iteration
-            #A[2, :] = -1
+            # A[2, :] = -1


Why this commented line?

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

Got it. Please rewrite the comment as

# A[2, :] = -1 is the same for all iterations, not re-affected.

sciunto · 2016-12-24T12:33:02Z

skimage/measure/fit.py

+            xc0 = x.mean()
+            yc0 = y.mean()
+            r0 = dist(xc0, yc0).mean()
+            init_params = (xc0, yc0, r0)


Concatenate these 4 lines into a single one.

sciunto · 2016-12-24T12:38:21Z

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:


This is inconsistent with the docstring (zero included).

sciunto · 2016-12-24T12:38:55Z

skimage/measure/fit.py

        by `np.random`.
-    
+    init_inliers : [list, tuple of] (N) array of bool or None, optional
+        Initial samples selection for model estimation


estimation.

sciunto · 2016-12-24T14:27:52Z

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

Borda · 2016-12-24T19:42:44Z

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

sciunto · 2016-12-26T19:05:10Z

skimage/measure/fit.py

    random_state = check_random_state(random_state)

+    if isinstance(min_samples, float):
+        if not (0 < min_samples <= 1):


0 <= min_samples <= 1

sciunto · 2016-12-26T19:07:57Z

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

Borda · 2016-12-26T20:32:26Z

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 · 2016-12-27T07:55:35Z

@Borda I guess it just got overwhelmed :).

Borda · 2017-01-02T08:04:10Z

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

ahojnnes · 2017-01-02T22:17:45Z

I have some concerns regarding this contribution:

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.
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 · 2017-01-02T23:23:34Z

@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 · 2017-01-03T07:30:54Z

@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 · 2017-01-30T17:44:09Z

it seems it crashs on building doc for scikit-image/doc/source/api/skimage.filters.rank.rst

sciunto · 2017-01-30T19:35:01Z

random failures due to travis. No worries.

Borda · 2017-08-14T14:18:10Z

@sciunto @ahojnnes any update? :)

stefanv

Looking at the diff, I am a bit confused how it implements the description from the PR?

However, these changes also seem fine to me, so no objection; just please update the commit message when merging the PR.

stefanv · 2017-08-14T18:34:45Z

skimage/measure/tests/test_fit.py

@@ -1,4 +1,5 @@
 import numpy as np
+from scipy import spatial


Why is spatial added here?

Borda · 2017-08-17T02:48:25Z

@sciunto it seems that there are some errors which came with merge upstream/master

appveyor: Cython.Compiler.Errors.CompileError: C:\projects\scikit-image\skimage\measure_ccomp.pyx
shippable: ```
if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then source tools/travis_osx_install.sh ; fi
ccache -s /root/102194d2-8a8b-4b47-9362-5695898aa651.sh: line 73: ccache: command not found
travis: The executable /usr/bin/python3.6 (from --python=/usr/bin/python3.6) does not exist

soupault · 2018-01-13T18:42:20Z

I think it's time to let this PR go 😄 .
@Borda maybe try to select the related commits from the list in https://github.com/scikit-image/scikit-image/pull/2371/commits ? We could then make a rebase onto master and cherry pick them.

Borda · 2018-01-13T22:19:48Z

May I ask what happens with Travis CI, I am not able to build even the master...
https://travis-ci.org/Borda/scikit-image/builds/321960550

soupault · 2018-01-13T22:38:43Z

@Borda your master diverges from the upstream one:

/home/travis/.travis/job_stages: line 57: tools/travis/before_install.sh: No such file or directory

Borda · 2018-01-14T00:16:44Z

ok, this seems to be solved...
but there appears an issue with space in doctest such as (this is from travis)

381     >>> np.round(ellipse.params, 2)
Expected:
    array([ 10.  ,  15.  ,   4.  ,   8.  ,   0.52])
Got:
    array([10.  , 15.  ,  4.  ,  8.  ,  0.52])

soupault · 2018-01-14T06:34:40Z

@Borda yet, that is known. See #2935

Borda · 2018-01-14T10:53:38Z

I see I was thinking about correcting these test with correcting the output and set # doctest: +NORMALIZE_WHITESPACE and do it as new PR from master... what do you think?

soupault · 2018-01-15T08:47:31Z

From what I understood, it is not just a matter of whitespaces - see https://github.com/numpy/numpy/blob/master/doc/release/1.14.0-notes.rst#many-changes-to-array-printing-disableable-with-the-new-legacy-printing-mode . Floating point precision could be different, bool ndarrays dtype is not printed anymore, etc.

Borda · 2018-01-15T09:37:44Z

yes, I noticed it also while I tried to solve it in #2938, but the difficulty comes with the configurations because in some configurations seems to be used the old formatting...

# Conflicts: # .coveragerc # DEPENDS.txt # TASKS.txt # appveyor.yml # bento.info # doc/ext/sphinx_gallery/LICENSE # doc/ext/sphinx_gallery/README.txt # doc/ext/sphinx_gallery/__init__.py # doc/ext/sphinx_gallery/_static/broken_example.png # doc/ext/sphinx_gallery/_static/broken_stamp.svg # doc/ext/sphinx_gallery/_static/gallery.css # doc/ext/sphinx_gallery/_static/no_image.png # doc/ext/sphinx_gallery/backreferences.py # doc/ext/sphinx_gallery/docs_resolv.py # doc/ext/sphinx_gallery/downloads.py # doc/ext/sphinx_gallery/gen_gallery.py # doc/ext/sphinx_gallery/gen_rst.py # doc/ext/sphinx_gallery/notebook.py # doc/ext/sphinx_gallery/py_source_parser.py # optional_requirements.txt # skimage/io/_plugins/freeimage_plugin.ini # skimage/io/_plugins/freeimage_plugin.py # skimage/util/montage.py # tools/appveyor/install.ps1 # tools/appveyor/run_with_env.cmd

Borda · 2018-03-22T09:46:24Z

It seems that I lost the commits... :-/

Borda changed the title ~~copy last version from PR #2371~~ add init params for Ellipse & Circle and RANSAC Dec 24, 2016

This was referenced Dec 24, 2016

add init params for Ellipse & Circle and RANSAC #2371

Closed

stable ellipse fitting #2394

Merged

soupault added ⏩ type: Enhancement Improve existing features 🧐 Needs review labels Dec 24, 2016

sciunto suggested changes Dec 24, 2016

View reviewed changes

Borda force-pushed the init_params_fit_models branch from f6f4105 to ed06419 Compare December 24, 2016 16:03

sciunto reviewed Dec 26, 2016

View reviewed changes

Borda force-pushed the init_params_fit_models branch from 674160e to 5c62f8e Compare December 29, 2016 17:57

sciunto added this to the 0.14 milestone Jan 2, 2017

Borda changed the title ~~add init params for Ellipse & Circle and RANSAC~~ update RANSAC, ratio of inliers Jan 27, 2017

sciunto requested a review from ahojnnes January 30, 2017 19:36

Borda force-pushed the init_params_fit_models branch 6 times, most recently from 0fe32ff to cec7b4e Compare February 5, 2017 12:42

stefanv reviewed Aug 14, 2017

View reviewed changes

skimage/measure/tests/test_fit.py Outdated

@@ -1,4 +1,5 @@

import numpy as np

from scipy import spatial

Copy link

Member

stefanv Aug 14, 2017

Choose a reason for hiding this comment

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

Why is spatial added here?

git merge upstream/master

217969b

Borda force-pushed the init_params_fit_models branch from cec7b4e to 76da430 Compare August 17, 2017 02:30

Borda force-pushed the init_params_fit_models branch from ee73666 to d8e16c8 Compare January 13, 2018 18:39

Borda mentioned this pull request Jan 14, 2018

Fix doctest space #2938

Closed

Borda force-pushed the init_params_fit_models branch from d8e16c8 to 217969b Compare January 28, 2018 05:50

Borda closed this Mar 22, 2018

Uh oh!

update RANSAC, ratio of inliers #2415

update RANSAC, ratio of inliers #2415

Uh oh!

Conversation

Borda commented Dec 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

codecov-io commented Dec 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Borda Dec 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

sciunto commented Dec 24, 2016

Uh oh!

Borda commented Dec 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sciunto commented Dec 26, 2016

Uh oh!

Borda commented Dec 26, 2016

Uh oh!

soupault commented Dec 27, 2016

Uh oh!

Borda commented Jan 2, 2017

Uh oh!

ahojnnes commented Jan 2, 2017

Uh oh!

Borda commented Jan 2, 2017

Uh oh!

ahojnnes commented Jan 3, 2017

Uh oh!

Borda commented Jan 30, 2017

Uh oh!

sciunto commented Jan 30, 2017

Uh oh!

Borda commented Aug 14, 2017

Uh oh!

stefanv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

soupault commented Jan 13, 2018

Uh oh!

Borda commented Jan 13, 2018

Uh oh!

soupault commented Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Borda commented Jan 14, 2018

Uh oh!

soupault commented Jan 14, 2018

Uh oh!

Borda commented Jan 14, 2018

Uh oh!

soupault commented Jan 15, 2018

Borda commented Dec 24, 2016 •

edited

Loading

codecov-io commented Dec 24, 2016 •

edited

Loading

Borda Dec 24, 2016 •

edited

Loading

Borda commented Aug 17, 2017 •

edited

Loading

soupault commented Jan 13, 2018 •

edited

Loading