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

Combine withEffect PathEffect definitions.#15515

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

Merged
timhoffm merged 1 commit intomatplotlib:masterfromanntzer:witheffect
Mar 14, 2020

Conversation

anntzer
Copy link
Contributor

PR Summary

could be used in#15458 as well.

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Copy link
Member

@timhoffmtimhoffm left a comment
edited
Loading

Choose a reason for hiding this comment

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

For some reason, docstring inheritance does not work for thedraw_path() method of the generated class. Compare
https://matplotlib.org/devdocs/api/patheffects_api.html#matplotlib.patheffects.withStroke.draw_path
and
https://9440-7439715-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/api/patheffects_api.html#matplotlib.patheffects.withStroke.draw_path

Apart from that, I have the concern that this is obscuring things more.withStroke = _make_with_effect(Stroke) does in no way indicate thatwithStroke is actually a class. This needs better naming and documentation.

@anntzer
Copy link
ContributorAuthor

Re docs: good catch, this is actually a limitation of the implementation of inspect.getdoc (which can be fixed at least in this case), I'll open a bug report on the python tracker. In the meantime I just inherited the docstring manually.
Re naming:_make_with_effect_patheffectsubclass? Happy for more proposals.

@timhoffm
Copy link
Member

What aboutwithStroke = _subclass_with_normal(effect_class=Stroke)?

@anntzer
Copy link
ContributorAuthor

Sounds good, changed accordingly.

@anntzer
Copy link
ContributorAuthor

(the docstring inheritance issue is athttps://bugs.python.org/issue38603)


withEffect.__name__ = f"with{effect_class.__name__}"
withEffect.__doc__ = f"""
Adds a `.{effect_class.__name__}` and then draws the original Artist to
Copy link
Member

Choose a reason for hiding this comment

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

I've been struggling to understand what thewith... classes are and what they are good for (path effect newbie here). Therefore I'm a bit picky on docs. I propose something like this (Please check ReST syntax and line length. This is a bit difficult in the github interface):

"""A shortcut PathEffect for applying `.{effect_class.__name__}` and thendrawing the original Artist.Examples--------With this class you can use ::    artist.set_path_effects([path_effects.with{effect_class.__name__}()])as a shortcut for ::    artist.set_path_effects([path_effects.{effect_class.__name__}(),                             path_effects.Normal()])"""

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Taking for example the path effect proposed at#15458, using justpath_effects=[TickedStroke(...)] wouldonly draw the ticks, not the "original" path; usingpath_effects=[withTickedStroke(...)] draws both the original path and not the Stroke. Your suggestion is fine, adding it.

@timhoffm
Copy link
Member

Bonus points if you add a link tohttps://matplotlib.org/3.1.1/tutorials/advanced/patheffects_guide.html in the module docstring.

@anntzer
Copy link
ContributorAuthor

all suggestions integrated.

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.

Plus bonus points for the tutorial reference 😄.

super().draw_path(renderer, gc, tpath, affine, rgbFace)
renderer.draw_path(gc, tpath, affine, rgbFace)

withEffect.__name__ = f"with{effect_class.__name__}"
Copy link
Member

Choose a reason for hiding this comment

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

So... if you have to do this big song and dance for the docs, is this change really worth it?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

It's more worth it if we keep adding new patheffects and repeat this code pattern again and again, e.g.#15458.

@anntzer
Copy link
ContributorAuthor

rebased

@timhoffmtimhoffm added this to thev3.3.0 milestoneMar 14, 2020
@timhoffmtimhoffm merged commit32f08df intomatplotlib:masterMar 14, 2020
@anntzeranntzer deleted the witheffect branchMarch 14, 2020 16:29
@QuLogicQuLogic mentioned this pull requestFeb 3, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jklymakjklymakjklymak left review comments

@QuLogicQuLogicQuLogic approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

4 participants
@anntzer@timhoffm@QuLogic@jklymak

[8]ページ先頭

©2009-2025 Movatter.jp