New Feature - PolygonSelector Widget by bduick · Pull Request #8403 · matplotlib/matplotlib

bduick · 2017-03-30T02:29:17Z

Hello matplotlib community! Please checkout the PolygonSelector widget. The demo is pretty straight forward, and basically a copy of the LassoSelector demo. I'm open to any feedback, or suggestions for improvement. Thanks for your consideration.

phobson

This looks really cool! I haven't taken a deep look yet, but here are some comments that should get the CI builds to pass.

phobson · 2017-03-30T03:01:26Z

lib/matplotlib/tests/test_widgets.py

+
+    # Move first vertex before completing the polygon.
+    expected_result = [(75, 50), (150, 50), (50, 150)]
+    event_sequence = [*polygon_place_vertex(50, 50),


unfortunately, we have to support python 2.7 and 3.4, so this won't work :/

phobson · 2017-03-30T03:02:53Z

lib/matplotlib/tests/test_widgets.py

+    ax = get_ax()
+
+    ax._onselect_cnt = 0
+    def onselect(vertices):


PEP8 wants a blank like here

bduick · 2017-03-30T05:07:10Z

Thanks phobson. Glad you like it.

…r additional coverage.

tacaswell · 2017-04-02T03:33:56Z

lib/matplotlib/widgets.py

+        When a polygon is completed or modified after completion,
+        the `onselect` function is called and passed a list of the vertices as
+        ``(xdata, ydata)`` tuples.
+    useblit : bool, optional


Interactive is not documented.

tacaswell · 2017-04-02T03:34:10Z

lib/matplotlib/widgets.py

+        The markers for the vertices of the polygon are drawn with
+        `markerprops`. The default is ``dict(marker='o', markersize=7, mec='k',
+        mfc='k', alpha=0.5)``.
+    vertex_select_radius : int


should be optional

and would this be better as a float?

tacaswell · 2017-04-02T03:37:46Z

This looks useful 👍

attn @blink1073 I have completely lost track of where you are at on other selector work.

should be modulo doc changes

tacaswell

Modulo minor documentation changes.

bduick · 2017-04-02T15:37:21Z

@tacaswell i can add support for non-interactive. i think i will implement it so that the only difference with interactive mode is that the polygon is automatically cleared after the selection is made. alternatively, does anyone think that i should also inhibit the move_vertex and move_all functions that are currently allowed before the polygon has been completed? in other words, the way it currently works is that if you haven't closed the polygon yet, you can move all the vertices or move a single vertex by holding shift or ctrl. should i disable this in non-interactive to be more consistent with the other widgets?

tacaswell · 2017-04-03T00:05:17Z

On a second reading, just drop interactive as a kwarg.

blink1073 · 2017-04-03T01:01:07Z

This is great! I had tried and given up on a similar tool in another PR.

bduick · 2017-04-03T22:39:02Z

My bad on the interactive kwarg. Didn't even realize until after your latest comment that I had accidentally left it in the signature.

bduick · 2017-04-03T22:55:22Z

@blink1073, glad you like the tool. i found your PR after thomas tagged you. the PR makes a lot of good points. i did find that i had to put in some special logic to work around logic in _SelectorWidget that wasn't applicable for the PolygonSelector.

tacaswell · 2017-04-04T01:54:54Z

@bduick Ah, sorry my comments were cryptic. Could you add to the docstring that 'vertex_select_radius' is optional?

I think that is the only thing standing between this and merging :)

bduick · 2017-04-04T02:30:01Z

I should've understood. Anyways, fixed and committed!

QuLogic · 2017-04-04T02:28:16Z

examples/widgets/polygon_selector_demo.py

+    """Select indices from a matplotlib collection using `PolygonSelector`.
+
+    Selected indices are saved in the `ind` attribute. This tool highlights
+    selected points by fading them out (i.e., reducing their alpha values).


Doesn't it fade out the points that are not selected?

QuLogic · 2017-04-04T02:30:20Z

examples/widgets/polygon_selector_demo.py

+        if len(self.fc) == 0:
+            raise ValueError('Collection must have a facecolor')
+        elif len(self.fc) == 1:
+            self.fc = np.tile(self.fc, self.Npts).reshape(self.Npts, -1)


Isn't this np.repeat(self.fc, self.Npts)?

Actually, no, I just tried it out and it's not, but it is equal to np.tile(x, (self.Npts, 1)) if I've understood it correctly.

QuLogic · 2017-04-04T02:31:59Z

examples/widgets/polygon_selector_demo.py

+
+    def onselect(self, verts):
+        path = Path(verts)
+        self.ind = np.nonzero([path.contains_point(xy) for xy in self.xys])[0]


Does path.contains_points(self.xys) work here?

QuLogic · 2017-04-04T02:33:56Z

lib/matplotlib/tests/test_widgets.py

+        where the first element of the tuple is an etype (e.g., 'onmove',
+        'press', etc.), and the second element of the tuple is a dictionary of
+         the arguments for the event (e.g., xdata=5, key='shift', etc.).
+    expected_result : list or vertices (xdata, ydata)


QuLogic · 2017-04-04T02:34:33Z

lib/matplotlib/tests/test_widgets.py

    assert slider.val == slider.valmax
+
+
+def check_polygon_selector(event_sequence, expected_result, selections_cnt):


Don't see any reason to abbreviate count.

QuLogic · 2017-04-04T02:38:25Z

lib/matplotlib/widgets.py

+        ``(xdata, ydata)`` tuples.
+    useblit : bool, optional
+    lineprops : dict, optional
+        The line for the sides of the polygon is drawn with `lineprops`. The


"... drawn with the properties given by lineprops."

(Also, references do not work to arguments.)

Can you give me some more info re: "references do not work to arguments"? I'm not sure what you mean.

@QuLogic, is there anything else I need to do to address this comment?

QuLogic · 2017-04-04T02:38:47Z

lib/matplotlib/widgets.py

+        The line for the sides of the polygon is drawn with `lineprops`. The
+        default is ``dict(color='k', linestyle='-', linewidth=2, alpha=0.5)``.
+    markerprops : dict, optional
+        The markers for the vertices of the polygon are drawn with


"... drawn with the properties given by ..."

QuLogic · 2017-04-04T02:40:18Z

lib/matplotlib/widgets.py

+        """Button press event handler"""
+        # Check for selection of a tool handle.
+        if ((self._polygon_completed or 'move_vertex' in self.state)
+            and len(self._xs) > 0):


Should be indented one more time.

QuLogic · 2017-04-04T02:44:42Z

lib/matplotlib/widgets.py

+        # Add back the pending vertex if leaving the 'move_vertex' or
+        # 'move_all' mode (by checking the released key)
+        if (not self._polygon_completed
+            and (event.key == self.state_modifier_keys.get('move_vertex')


Should be indented once more.

QuLogic · 2017-04-04T02:45:02Z

lib/matplotlib/widgets.py

+        # if the polygon is completed or the user is locked on to the start
+        # vertex.
+        if (self._polygon_completed
+            or (len(self._xs) > 3


Should be indented once more.

bduick · 2017-04-05T02:23:27Z

@QuLogic, thanks for the thorough review. Your 3 comments on the polygon_selector_demo.py also apply to lasso_selector_demo.py. Can I commit the fixes for the lasso demo in this PR, or should I create a new one?

tacaswell · 2017-04-05T04:34:48Z

@bduick which ever is easier for you.

…ector demos.

tacaswell · 2017-04-06T00:40:38Z

failures look to be due to changing DNS for matplotlib.org and it taking time for those changes to propogate

jrmlhermitte · 2017-04-06T20:21:04Z

very cool!

tacaswell · 2017-04-10T03:54:45Z

attn @phobson

bduick added 3 commits March 28, 2017 19:32

Added PolygonSelector to lib/matplotlib/widgets.py

9ff6eef

Added demo for PolygonSelector.

f1648e4

Added tests for PolygonSelector.

db0625b

phobson requested changes Mar 30, 2017

View reviewed changes

Minor updates to test for PolygonSelector.

5cd12d9

Minor updates to test for PolygonSelector v2

889cc8d

dstansby added this to the 2.1 (next point release) milestone Mar 30, 2017

Fixed crash if 'escape' is pressed while not inaxes. Add test case fo…

1d5aeae

…r additional coverage.

tacaswell reviewed Apr 2, 2017

View reviewed changes

tacaswell previously approved these changes Apr 2, 2017

View reviewed changes

tacaswell approved these changes Apr 2, 2017

View reviewed changes

Removed unused kwarg 'interactive' from PolygonSelector init.

50aa28a

Corrected errors in PolygonSelector doc string.

31055bf

QuLogic reviewed Apr 4, 2017

View reviewed changes

PEP8 fixes, docstring wording fixes, and minor simplifications to sel…

2d8bec9

…ector demos.

phobson approved these changes Apr 13, 2017

View reviewed changes

phobson added the topic: widgets/UI label Apr 13, 2017

phobson merged commit 239276e into matplotlib:master Apr 13, 2017

		assert slider.val == slider.valmax


		def check_polygon_selector(event_sequence, expected_result, selections_cnt):

Uh oh!

Conversation

bduick commented Mar 30, 2017

Uh oh!

phobson 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!

bduick commented Mar 30, 2017

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!

tacaswell commented Apr 2, 2017

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

bduick commented Apr 2, 2017

Uh oh!

tacaswell commented Apr 3, 2017

Uh oh!

blink1073 commented Apr 3, 2017

Uh oh!

bduick commented Apr 3, 2017

Uh oh!

bduick commented Apr 3, 2017

Uh oh!

tacaswell commented Apr 4, 2017

Uh oh!

bduick commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic Apr 4, 2017 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

bduick commented Apr 5, 2017

Uh oh!

tacaswell commented Apr 5, 2017

Uh oh!

tacaswell commented Apr 6, 2017

Uh oh!

jrmlhermitte commented Apr 6, 2017

Uh oh!

tacaswell commented Apr 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

QuLogic Apr 4, 2017 •

edited

Loading