Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix setting artists properties of selectors#20693

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

ericpre
Copy link
Member

@ericpreericpre commentedJul 20, 2021•
edited
Loading

PR Summary

Fix#20618 by addingset_props andset_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 ofSpanSelector._rect and other, because it is already accessible throughartists[0], maybe this is still better to keep a separate attribute?
importmatplotlib.pyplotaspltfrommatplotlib.widgetsimportSpanSelectorimportnumpyasnpvalues=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:

  • useprops andhandle_props setter
  • add methods to get artists, something along the line of:get_main_artistget_handle_artists, which could use asspan.get_main_artist().set(facecolor='green'), forget_handle_artists, it would be necessary to loop over the tuple

PR Checklist

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

@ericpre
Copy link
MemberAuthor

@larsoner, does this API look good to you?

@larsoner
Copy link
Contributor

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

@ericpre
Copy link
MemberAuthor

Great, thanks@larsoner!

@jklymakjklymak added this to thev3.5.0 milestoneJul 21, 2021
@ericpreericpreforce-pushed theadd_set_props_handle_props_selector branch 2 times, most recently fromd73f751 to6a93943CompareJuly 27, 2021 12:25
@ericpre
Copy link
MemberAuthor

This is ready for review

@timhoffm
Copy link
Member

  • Remove the use ofSpanSelector._rect and other, because it is already accessible throughartists[0], maybe this is still better to keep a separate attribute?

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

Should we keep them internal (and have publicset_props andset_handle_props) or follow

  • add methods to get artists, something along the line of:get_main_artistget_handle_artists, which could use asspan.get_main_artist().set(facecolor='green'), forget_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 andset_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?

@ericpreericpreforce-pushed theadd_set_props_handle_props_selector branch from190c96c to9e6394fCompareJuly 28, 2021 08:41
@ericpre
Copy link
MemberAuthor

  • Remove the use ofSpanSelector._rect and other, because it is already accessible throughartists[0], maybe this is still better to keep a separate attribute?

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

Agreed, I have added a_selection_artists property.

  • add methods to get artists, something along the line of:get_main_artistget_handle_artists, which could use asspan.get_main_artist().set(facecolor='green'), forget_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 andset_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!

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(),
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 aLine2D, but we enforce the vertical placement using a transform, so this is not user-settable.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

importmatplotlib.pyplotaspltfig,ax=plt.subplots()line=ax.axvline(1.5)props=line.properties()delprops['transform']line.set(**props)

Will give the following error:

File"/home/eric/Dev/others/matplotlib/lib/matplotlib/artist.py",line115,in<lambda>cls.set=lambdaself,**kwargs:Artist.set(self,**kwargs)File"/home/eric/Dev/others/matplotlib/lib/matplotlib/artist.py",line1155,insetreturnself.update(kwargs)File"/home/eric/Dev/others/matplotlib/lib/matplotlib/artist.py",line1060,inupdateraiseAttributeError(f"{type(self).__name__!r} object "AttributeError:'Line2D'objecthasnoproperty'children'

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Essentially looks good.

Since for now, we seem to need the update,set_props() andset_handle_props() are the right way to go. In that case, do people still need access to the propertiesartists,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.

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(),
Copy link
Member

Choose a reason for hiding this comment

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

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 aLine2D, but we enforce the vertical placement using a transform, so this is not user-settable.

@ericpreericpreforce-pushed theadd_set_props_handle_props_selector branch fromb0545dc to73ac533CompareAugust 5, 2021 10:13
@ericpre
Copy link
MemberAuthor

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

@ericpreericpreforce-pushed theadd_set_props_handle_props_selector branch from8f0e823 toa871ca6CompareAugust 18, 2021 08:19
@ericpreericpreforce-pushed theadd_set_props_handle_props_selector branch from382bc1d tocee023eCompareAugust 21, 2021 11:05
@ericpre
Copy link
MemberAuthor

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

@timhoffmtimhoffm added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelAug 21, 2021
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@ericpre
Copy link
MemberAuthor

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

@timhoffm
Copy link
Member

Thanks@ericpre!

QuLogic added a commit that referenced this pull requestAug 24, 2021
…693-on-v3.5.xBackport PR#20693 on branch v3.5.x (Fix setting artists properties of selectors)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@QuLogicQuLogicQuLogic approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.topic: widgets/UI
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

BUG: Lost functionality of interactive selector update
6 participants
@ericpre@larsoner@timhoffm@QuLogic@anntzer@jklymak
[8]ページ先頭
©2009-2025 Movatter.jp