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

DocTestFailure on skimage.measure.fit.ransac #6479

Open
alexdesiqueira opened this issue Aug 19, 2022 · 5 comments
Open

DocTestFailure on skimage.measure.fit.ransac #6479

alexdesiqueira opened this issue Aug 19, 2022 · 5 comments
Assignees
Labels
🔰 Good first issue A good task for those new to the library 📄 type: Documentation Updates, fixes and additions to documentation

Comments

@alexdesiqueira
Copy link
Member

Description

Job linux-cp3.9-pre returns FAILED scikit-image/skimage/measure/fit.py::skimage.measure.fit.ransac, with the following:

=================================== FAILURES ===================================
_____________________ [doctest] skimage.measure.fit.ransac _____________________
746     >>> model = EllipseModel()
747     >>> model.estimate(data)
748     True
749     >>> np.round(model.params)  # doctest: +SKIP
750     array([ 72.,  75.,  77.,  14.,   1.])
751 
752     Estimate ellipse model using RANSAC:
753 
754     >>> ransac_model, inliers = ransac(data, EllipseModel, 20, 3, max_trials=50)
755     >>> abs(np.round(ransac_model.params))
Expected:
    array([20., 30., 10.,  6.,  2.])
Got:
    array([76., 77., 82., 17.,  1.])

/home/runner/work/scikit-image/scikit-image/skimage/measure/fit.py:755: DocTestFailure
@alexdesiqueira alexdesiqueira self-assigned this Aug 19, 2022
@alexdesiqueira
Copy link
Member Author

alexdesiqueira commented Aug 19, 2022

Using the following test code:

import numpy as np
from skimage import __version__, measure

print(f"* skimage version: {__version__}")

t = np.linspace(0, 2 * np.pi, 50)
xc, yc = 20, 30
a, b = 5, 10
x = xc + a * np.cos(t)
y = yc + b * np.sin(t)
data = np.column_stack([x, y])
rng = np.random.default_rng(203560)  # do not copy this value
data += rng.normal(size=data.shape)

data[0] = (100, 100)
data[1] = (110, 120)
data[2] = (120, 130)
data[3] = (140, 130)

model = measure.EllipseModel()
model.estimate(data)
print(f"* model.params: {np.round(model.params)}")
ransac_model, inliers = measure.ransac(data, measure.EllipseModel, 20, 3, max_trials=50)
print(f"* ransac_model.params: {abs(np.round(ransac_model.params))}")

Testing in three different versions:

* skimage version: 0.18.1
* model.params: [71. 75. 77. 13.  1.]
* ransac_model.params: [20. 30. 10.  6.  2.]
* skimage version: 0.19.3
* model.params: [71. 75. 77. 13.  1.]
* ransac_model.params: [20. 30. 10.  6.  2.]
* skimage version: 0.20.0.dev0+git20220819.d66e86012
* model.params: [71. 75. 77. 13.  1.]
* ransac_model.params: [20. 30. 10.  6.  2.]

... 🤔

@github-actions
Copy link

Hey, there hasn't been any activity on this issue for more than 180 days. For now, we have marked it as "dormant" until there is some new activity. You are welcome to reach out to people by mentioning them here or on our forum if you need more feedback! If you think that this issue is no longer relevant, you may close it by yourself; otherwise, we may do it at some point (either way, it will be done manually). In any case, thank you for your contributions so far!

@github-actions github-actions bot added the 😴 Dormant No recent activity label Feb 16, 2023
@stefanv
Copy link
Member

stefanv commented Feb 16, 2023

The code seems to be doing exactly what it should: the ellipse fit on all data is a bad fit, and the ransac fit is a better fit.

You can see that the ransac fit, and a fit on data[4:] correspond very well.

@stefanv
Copy link
Member

stefanv commented Feb 16, 2023

Perhaps the confusion is why the input parameters and output parameters differ. I wonder if different parametrizations can result in similar ellipses?

@stefanv
Copy link
Member

stefanv commented Feb 16, 2023

OK, here's the long and short of it:

(1) Noise is added do data, so we should not expect to recover the original model exactly.
(2) The alpha parameter (rotation) sometimes is estimated as np.pi/2, which means the a and b parameters swap around. I'm pretty sure there's an issue about this somewhere, but I can't find it.

Given the above, let's look at the results again.

Original model parameters:

[20 30  5 10  0]

Parameters estimated from noisy (but not outlier) data:

[20.201672158710714, 30.10345264649179, 9.800028697299355, 5.61137569675901, 1.5768798765880527]

RANSAC estimates:

[20.201672158710714, 30.10345264649179, 9.800028697299355, 5.61137569675901, 1.5768798765880527]

All correct, but it makes for a poor example in the docstring!

Recommended course of action: change the docstring to show that model.estimate(data[4:]) matches ransac model—that's good enough.

@stefanv stefanv added 📄 type: Documentation Updates, fixes and additions to documentation 🔰 Good first issue A good task for those new to the library and removed 😴 Dormant No recent activity labels Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔰 Good first issue A good task for those new to the library 📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants