Skip to content

/ matplotlib

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

Jump to bottom

Fix setting artists properties of selectors #20693

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix setting artists properties of selectors #20693

wants to merge 5 commits into from

Conversation

@ericpre
Copy link
Member

@ericpre ericpre commented Jul 20, 2021 •
edited

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
Copy link
Member Author

@ericpre ericpre commented Jul 20, 2021

@larsoner, does this API look good to you?
@larsoner
Copy link
Contributor

@larsoner larsoner commented Jul 20, 2021

Yep, and I tested it and it seems to work for us!
@ericpre
Copy link
Member Author

@ericpre ericpre commented Jul 20, 2021

Great, thanks @larsoner!
@jklymak jklymak added this to the v3.5.0 milestone Jul 21, 2021
ericpre added 2 commits Jul 20, 2021
@ericpre
Protect Selector.artists attribute
e3980fe
@ericpre
Add set_props and set_handle_props to selectors.
15beb47
@ericpre ericpre force-pushed the ericpre:add_set_props_handle_props_selector branch from f448e36 to d73f751 Jul 27, 2021
@ericpre
Fix deprecated attribute and add new feature entry
6a93943
@ericpre ericpre force-pushed the ericpre:add_set_props_handle_props_selector branch from d73f751 to 6a93943 Jul 27, 2021
@ericpre
Copy link
Member Author

@ericpre ericpre commented Jul 27, 2021

This is ready for review
@timhoffm
Copy link
Member

@timhoffm timhoffm commented Jul 27, 2021

  • 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 ericpre force-pushed the ericpre:add_set_props_handle_props_selector branch from 190c96c to 9e6394f Jul 28, 2021
@ericpre
Copy link
Member Author

@ericpre ericpre commented Jul 28, 2021

  • 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!
@ericpre
ericpre reviewed Jul 28, 2021
View changes
lib/matplotlib/widgets.py Outdated
def add_artist(self, artist):
"""Add artist to the selector."""
self._artists.append(artist)

@property
def artists(self):
"""Tuple of the artists of the selector."""
return tuple(self._artists)
Comment on lines 2010 to 2017

This comment has been minimized.

Sign in to view
@ericpre

ericpre Jul 28, 2021
Author Member

I would like to revisit this part in a follow up PR to discuss how to use artists which get updated in a callback, need to drawn at the same time as the selector artists and play well with blitting.
@timhoffm
timhoffm reviewed Jul 28, 2021
View changes
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
@ericpre
ericpre reviewed Jul 29, 2021
View changes
lib/matplotlib/widgets.py Outdated
self._direction = direction
self.new_axes(self.ax)
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(),

This comment has been minimized.

Sign in to view
@ericpre

ericpre Jul 29, 2021
Author Member

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

This comment has been minimized.

Sign in to view
@timhoffm

timhoffm Jul 30, 2021
Member

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.

This comment has been minimized.

Sign in to view
@ericpre

ericpre Aug 5, 2021
Author Member

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
timhoffm reviewed Jul 30, 2021
View changes
Copy link
Member

@timhoffm timhoffm left a comment

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.
lib/matplotlib/widgets.py Outdated
self._direction = direction
self.new_axes(self.ax)
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(),

This comment has been minimized.

Sign in to view
@timhoffm

timhoffm Jul 30, 2021
Member

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
Simplify access to selector artists
73ac533
@ericpre ericpre force-pushed the ericpre:add_set_props_handle_props_selector branch from b0545dc to 73ac533 Aug 5, 2021
@ericpre
Copy link
Member Author

@ericpre ericpre commented Aug 5, 2021

This is ready for review, I have updated the description of the PR to reflect the current state.
@anntzer
anntzer reviewed Aug 5, 2021
View changes
lib/matplotlib/widgets.py
handle.set(**handle_props)
if self.useblit:
self.update()
self._handle_props.update(handle_props)

This comment has been minimized.

Sign in to view
@anntzer

anntzer Aug 5, 2021
Contributor

You need to normalize properties here (see normalize_kwargs), otherwise with e.g.

from matplotlib.widgets import *
ax = gca()
ss = SpanSelector(ax, print, "horizontal", interactive=True)
ss.set_handle_props(lw=.5); ss.set_handle_props(linewidth=3); ss.direction = "vertical"

the handles' linewidth end up being 0.5, not 3 (also because linewidth is an explicit kwarg to Line2D whereas lw is implicitly handled as a generic property, but again I think the correct fix is to just normalize here.)

Likewise for props, probably.

This comment has been minimized.

Sign in to view
@ericpre

ericpre Aug 5, 2021
Author Member

Yes, this should be fixed in 8f0e823. Thanks!
@ericpre
Normalise kwargs when setting selector properties
8f0e823
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer

@timhoffm timhoffm

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

Assignees
No one assigned
Labels
status: needs review topic: widgets/UI
Projects
None yet
Milestone
v3.5.0
Linked issues

Successfully merging this pull request may close these issues.

BUG: Lost functionality of interactive selector update
5 participants
@ericpre @larsoner @timhoffm @anntzer @jklymak