Fix setting artists properties of selectors by ericpre · Pull Request #20693 · matplotlib/matplotlib

ericpre · 2021-07-20T14:10:36Z

PR Summary

Fix #20618 by adding set_props and set_handle_props method. ~~This PR needs a bit of tidying up but I would like to know if we are happy with this approach!~~

A few others changes:

selector.artists is now a property returning a tuple of all artists to disallow changing the artists through a public API
Add _selection_artist and _handles_artists to access the corresponding artists internally. ~~Remove the use of SpanSelector._rect and other, because it is already accessible through artists[0], maybe this is still better to keep a separate attribute?~~

import matplotlib.pyplot as plt
from matplotlib.widgets import SpanSelector
import numpy as np

values = np.arange(-10, 100)

fig = plt.figure()
ax = fig.add_subplot()
ax.plot(values, values)

span = SpanSelector(ax, print, "horizontal", interactive=True)

span.set_props(color='green')
span.set_handle_props(color='green')

Alternative could be:

use props and handle_props setter
add methods to get artists, something along the line of: get_main_artist get_handle_artists, which could use as span.get_main_artist().set(facecolor='green'), for get_handle_artists, it would be necessary to loop over the tuple

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
[n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

ericpre · 2021-07-20T14:12:29Z

@larsoner, does this API look good to you?

larsoner · 2021-07-20T16:41:49Z

Yep, and I tested it and it seems to work for us!

ericpre · 2021-07-20T20:22:46Z

Great, thanks @larsoner!

ericpre · 2021-07-27T14:09:25Z

This is ready for review

timhoffm · 2021-07-27T23:29:59Z

Remove the use of SpanSelector._rect and other, because it is already accessible through artists[0], maybe this is still better to keep a separate attribute?

I don't think this is a good idea. Having artists[0] implicitly be the selection artist is confusing. We should give these concepts names: selection_artist and handle_artists.

Should we keep them internal (and have public set_props and set_handle_props) or follow

add methods to get artists, something along the line of: get_main_artist get_handle_artists, which could use as span.get_main_artist().set(facecolor='green'), for get_handle_artists, it would be necessary to loop over the tuple
?

That depends: How much control do we want to give to the user? set_props and set_handle_props is quite a limited API, which is good in a way. Do we anticipate that getters will be needed as well? Do we anticipate that handles may become more complex, e.g. a combination of markers and lines, that a single setter cannot address?

ericpre · 2021-07-28T08:44:29Z

Remove the use of SpanSelector._rect and other, because it is already accessible through artists[0], maybe this is still better to keep a separate attribute?

I don't think this is a good idea. Having artists[0] implicitly be the selection artist is confusing. We should give these concepts names: selection_artist and handle_artists.

Agreed, I have added a _selection_artists property.

add methods to get artists, something along the line of: get_main_artist get_handle_artists, which could use as span.get_main_artist().set(facecolor='green'), for get_handle_artists, it would be necessary to loop over the tuple
?

That depends: How much control do we want to give to the user? set_props and set_handle_props is quite a limited API, which is good in a way. Do we anticipate that getters will be needed as well? Do we anticipate that handles may become more complex, e.g. a combination of markers and lines, that a single setter cannot address?

I would lean toward keeping it simple for now!

lib/matplotlib/widgets.py

ericpre · 2021-07-29T10:17:55Z

lib/matplotlib/widgets.py

            if self._interactive:
-                self._setup_edge_handle(self._edge_handles._line_props)
+                line = self._edge_handles.artists[0]
+                line_props = dict(alpha=line.get_alpha(),


Is there a way to get the properties relevant to axvline (the kwargs of axvline)? line.properties() returns too many properties.

Not directly. https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.axvline.html#matplotlib.axes.Axes.axvline says

Valid keyword arguments are Line2D properties, with the exception of 'transform'.

That's because it's a Line2D, but we enforce the vertical placement using a transform, so this is not user-settable.

Yes, indeed, but there are also other properties (internal properties?), which are not listed in https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.axvline.html#matplotlib.axes.Axes.axvline:

import matplotlib.pyplot as plt fig, ax = plt.subplots() line = ax.axvline(1.5) props = line.properties() del props['transform'] line.set(**props)

Will give the following error:

File "/home/eric/Dev/others/matplotlib/lib/matplotlib/artist.py", line 115, in <lambda> cls.set = lambda self, **kwargs: Artist.set(self, **kwargs) File "/home/eric/Dev/others/matplotlib/lib/matplotlib/artist.py", line 1155, in set return self.update(kwargs) File "/home/eric/Dev/others/matplotlib/lib/matplotlib/artist.py", line 1060, in update raise AttributeError(f"{type(self).__name__!r} object " AttributeError: 'Line2D' object has no property 'children'

timhoffm

Essentially looks good.

Since for now, we seem to need the update, set_props() and set_handle_props() are the right way to go. In that case, do people still need access to the properties artists, selection_artist, handle_artists? If not, I'd start with keeping them private to keep the public API small. We can always make them public later if needed.

timhoffm · 2021-07-30T12:40:42Z

lib/matplotlib/widgets.py

            if self._interactive:
-                self._setup_edge_handle(self._edge_handles._line_props)
+                line = self._edge_handles.artists[0]
+                line_props = dict(alpha=line.get_alpha(),


Not directly. https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.axvline.html#matplotlib.axes.Axes.axvline says

Valid keyword arguments are Line2D properties, with the exception of 'transform'.

That's because it's a Line2D, but we enforce the vertical placement using a transform, so this is not user-settable.

ericpre · 2021-08-05T11:12:46Z

This is ready for review, I have updated the description of the PR to reflect the current state.

lib/matplotlib/widgets.py

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

…elector.

ericpre · 2021-08-21T11:08:26Z

Thanks @QuLogic for the review, all comments should be sorted!

lib/matplotlib/tests/test_widgets.py

lib/matplotlib/widgets.py

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

ericpre · 2021-08-24T18:38:56Z

Thanks @timhoffm, all done - CI had some hiccups and I restarted it.

…tors

timhoffm · 2021-08-24T20:47:19Z

Thanks @ericpre!

…693-on-v3.5.x Backport PR #20693 on branch v3.5.x (Fix setting artists properties of selectors)

jklymak added the topic: widgets/UI label Jul 21, 2021

jklymak added this to the v3.5.0 milestone Jul 21, 2021

ericpre force-pushed the add_set_props_handle_props_selector branch 2 times, most recently from d73f751 to 6a93943 Compare July 27, 2021 12:25

ericpre force-pushed the add_set_props_handle_props_selector branch from 190c96c to 9e6394f Compare July 28, 2021 08:41

ericpre commented Jul 28, 2021

View reviewed changes

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

timhoffm reviewed Jul 28, 2021

View reviewed changes

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

ericpre commented Jul 29, 2021

View reviewed changes

timhoffm reviewed Jul 30, 2021

View reviewed changes

ericpre force-pushed the add_set_props_handle_props_selector branch from b0545dc to 73ac533 Compare August 5, 2021 10:13

ericpre added the status: needs review label Aug 5, 2021

anntzer reviewed Aug 5, 2021

View reviewed changes

lib/matplotlib/widgets.py Show resolved Hide resolved

QuLogic added the status: needs rebase label Aug 17, 2021

ericpre added 5 commits August 18, 2021 09:18

Protect Selector.artists attribute

233a05d

Add set_props and set_handle_props to selectors.

9d5f71f

Fix deprecated attribute and add new feature entry

0c06766

Simplify access to selector artists

b6e4e88

Normalise kwargs when setting selector properties

a871ca6

ericpre force-pushed the add_set_props_handle_props_selector branch from 8f0e823 to a871ca6 Compare August 18, 2021 08:19

ericpre removed the status: needs rebase label Aug 18, 2021

QuLogic reviewed Aug 21, 2021

View reviewed changes

ericpre and others added 2 commits August 21, 2021 11:52

Apply suggestions from code review: improve normalisation kwargs

768c583

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

Add api change note for the deprecation of line attribute of PolygonS…

cee023e

…elector.

ericpre force-pushed the add_set_props_handle_props_selector branch from 382bc1d to cee023e Compare August 21, 2021 11:05

timhoffm added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 21, 2021

QuLogic approved these changes Aug 23, 2021

View reviewed changes

timhoffm reviewed Aug 24, 2021

View reviewed changes

Simplify syntax and improve readability

893e7cf

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

timhoffm approved these changes Aug 24, 2021

View reviewed changes

timhoffm merged commit 4d648d6 into matplotlib:master Aug 24, 2021

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 24, 2021

Backport PR matplotlib#20693: Fix setting artists properties of selec…

3d180e7

…tors

meeseeksmachine mentioned this pull request Aug 24, 2021

Backport PR #20693 on branch v3.5.x (Fix setting artists properties of selectors) #20890

Merged

timhoffm removed the status: needs review label Aug 24, 2021

QuLogic added a commit that referenced this pull request Aug 24, 2021

Merge pull request #20890 from meeseeksmachine/auto-backport-of-pr-20…

d6ad47e

…693-on-v3.5.x Backport PR #20693 on branch v3.5.x (Fix setting artists properties of selectors)

Uh oh!

Conversation

ericpre commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

ericpre commented Jul 20, 2021

Uh oh!

larsoner commented Jul 20, 2021

Uh oh!

ericpre commented Jul 20, 2021

Uh oh!

ericpre commented Jul 27, 2021

Uh oh!

timhoffm commented Jul 27, 2021

Uh oh!

ericpre commented Jul 28, 2021

Uh oh!

Uh oh!

Uh oh!

ericpre Jul 29, 2021

Choose a reason for hiding this comment

Uh oh!

timhoffm Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

ericpre Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

timhoffm Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

ericpre commented Aug 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericpre commented Aug 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericpre commented Aug 24, 2021

Uh oh!

timhoffm commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ericpre commented Jul 20, 2021 •

edited

Loading