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

Polygon approximation #2002

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Polygon approximation #2002

wants to merge 8 commits into from

Conversation

Seth-Park
Copy link

addressing issue #1552

Some of the issues in the previous implementation:

  1. The previous implementation always included the starting and end points of the input, which resulted in a suboptimal behavior mentioned in issue measure.approximate_polygon does not approximate first and last point #1552. This also means that if the starting and end points change, the algorithm will give different results although the shape of the polygon itself stays the same, which is also a suboptimal behavior.
  2. Usage of arctan2() to calculate perpendicular distance might introduce unnecessary bugs if unexpected degree/radian values are given.

Possible Fix:

  1. Instead of always having the algorithm include the starting point and the end point, users can use a flag called "closed" to choose how the algorithm approximates the polygon. If closed=True, the algorithm assumes that the given polygon is closed and thus starts approximating by first finding the furthest two points. If closed=False, it assumes the polygon to be open and simply applies the previous implementation.
    In the following examples, the top plots in each figure are the results when setting closed=True while the bottom plots are the results when setting closed=False.

tolerance4
tolerance8

  1. Replace with a code that calculates the perpendicular distance using an equation that does not involve any trig functions.

Open to many feedbacks! Thank you :)

stefanv
stefanv reviewed Mar 15, 2016
View reviewed changes
skimage/measure/_polygon.py
@@ -18,6 +19,12 @@ def approximate_polygon(coords, tolerance):
Maximum distance from original points of polygon to approximated
polygonal chain. If tolerance is 0, the original coordinate array
is returned.
closed : boolean
If true, the algorithm assumes the given polygon is closed
(the first and last vertices are connected) and approximates using the
Copy link
Member

Choose a reason for hiding this comment

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

true -> True

Describe "approximating using the first and last point" a bit more clearly.

@soupault soupault added the ⏩ type: Enhancement Improve existing features label Mar 15, 2016
soupault
soupault reviewed Apr 18, 2016
View reviewed changes
skimage/measure/_polygon.py Outdated
@@ -1,8 +1,11 @@
import numpy as np
from scipy import signal
from scipy.spatial import distance_matrix
from numpy import unravel_index
import pdb
Copy link
Member

Choose a reason for hiding this comment

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

Unused import here.

@sciunto
Copy link
Member

sciunto commented Aug 21, 2016

This PR looks great. I iterated on stefan's comment. After that, I think it will be ready. There are some test failures due to #2237

sciunto
sciunto suggested changes Sep 18, 2016
View reviewed changes
Copy link
Member

@sciunto sciunto left a comment

Choose a reason for hiding this comment

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

There is only minor things to fix, then it will be ready for me.

skimage/measure/_polygon.py
@@ -18,6 +20,13 @@ def approximate_polygon(coords, tolerance):
Maximum distance from original points of polygon to approximated
polygonal chain. If tolerance is 0, the original coordinate array
is returned.
closed : boolean
Copy link
Member

Choose a reason for hiding this comment

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

boolean, optional

skimage/measure/tests/test_polygon.py
@@ -11,18 +11,30 @@


def test_approximate_polygon():
Copy link
Member

Choose a reason for hiding this comment

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

I would split this test in several ones, with a name that reflects the idea behind the each test.

Base automatically changed from master to main February 18, 2021 18:22
@mkcor mkcor mentioned this pull request Feb 22, 2021
Closed
@rfezzani
Copy link
Member

Hello @Seth-Park and sorry for the tooooo long feedback 🙏
Would you be interested in completing this PR? I would be happy to help on this, I find your approach intersting 😉

@mkcor mkcor mentioned this pull request Feb 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sciunto sciunto sciunto requested changes

At least 1 approving review is required to merge this pull request.

Assignees

@stefanv stefanv

@jni jni

Labels
⚖️ Needs decision ⏩ type: Enhancement Improve existing features 🧐 Needs review 🙏 Feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Seth-Park @sciunto @rfezzani @stefanv @soupault @jni @grlee77