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

DOCS: update collections.py docstrings to current doc conventions#17575

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

Conversation

brunobeltran
Copy link
Contributor

@brunobeltranbrunobeltran commentedJun 5, 2020
edited
Loading

PR Summary

Meant to just add the appropriate:rc: links to thecollections.py docs, but then realized that the docs in there were reasonably out of sync with our style conventions, so I tried my best to get these sync'd up to match what I understand we would want the docs to look like if they were new contributions.

There are some warnings when I compile the docs locally, but then again, I also get warnings onmaster, so I went ahead and opened the PR to see if readthedocs has better luck than I.

PR Checklist

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

@brunobeltranbrunobeltranforce-pushed thecollections-rcparams-links branch 2 times, most recently from2afbe5f tof87cc48CompareJune 5, 2020 02:36
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.

Thanks for taking this up!

This is a lot to do and more to be done. I'm trying to strike a balance between making everything right (you probably have noticed that I have opinions on docstrings) and not commenting on every bit (some of which are not even your changes but just inherited from the current code). Please be patient and notify me when it get's too much - I might have a second or third round when reading this again.

Note: Code suggestions made here may need to be wrapped to hold the 79 char limit. It's hard to do this right in the github interface.

Comment on lines 33 to 35
A Collection represents a sequence of `~.patches.Patch` objects that the
library believes would be more efficient to draw together than
individually. For example, when a single path is being drawn repeatedly at
Copy link
Member

@timhoffmtimhoffmJun 5, 2020
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
ACollectionrepresentsasequenceof`~.patches.Patch`objectsthatthe
librarybelieveswouldbemoreefficienttodrawtogetherthan
individually.Forexample,whenasinglepathisbeingdrawnrepeatedlyat
ACollectionrepresentsasequenceof`.Artist`\s.Itismoreefficientto
drawcomparedtousingmanyindividual`.Artist`objects.Forexample,
whenasinglepathisbeingdrawnrepeatedlyat

Note: You additionally have to make the docstring a raw stringr``` because of the plural s for.Artists.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Is this true though? WhileCollectioninherits fromArtist, it re-implements basically the entirePatch interface with plural method/property names. In addition, its properties default topatches. values fromrc.

My impression from reading the code was that the only reasonCollectiondoesn't inherit fromPatch is because we wanted plural method names (but that might be an a posteriori justification, since I obviously wasn't around for the implementation).

The point is, while the PythonCollection objectis an artist, that part can be inferred from the class structure on the relevant docs page. The part that is not clear unless you literally read all the properties of Artist/Collection/Patch and compare them manually is that Collection semantically represents a collection ofPatches to be drawn simultaneously (i.e. it has edgecolor, facecolor, etc.).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Also, I hesitate to remove the wording about "may be" more efficient, since for most renderers,draw_path_collection is not necessarily any more efficient than individualdraw_path calls (it just typically is whenever Collections are used correctly).

But I do see why you wanted to change it, the wording is pretty awkward, so I'll try changing it.

Copy link
Member

Choose a reason for hiding this comment

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

You may be mostly right withPatch. I was thinking ofLineCollection which rather mimics a list ofLine2D and thus went for the more generalArtist. This is all a bit messy.

Is it reasonable to stick toPatch and just mention inLineCollection that it's different?

Performance: I didn't check, but I assume the advantage of collections is that you save a lot of python function calls, similar to how numpy is faster than a list of floats.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I'll update the docs to refer to patch and make sure that LineCollection is clear about how it's basically the same thing but for Line2D.

I think you're right about the performance in theory, but in practice, backend base still breaks updraw_path_collection into individualdraw_path calls anyway, so you don't gain performance unless the backend supports it, which was probably the justification for the original language.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the backend is the bottleneck or creating the Artists, but in a quick test LineCollection is faster:

import matplotlib.pyplot as pltfrom matplotlib.lines import Line2Dfrom matplotlib.collections import LineCollectionimport numpy as npxs = np.linspace(0, 1, 1000)
%%timeitfig, ax = plt.subplots()for x in xs:    line = Line2D([x, x], [0, 1])    ax.add_line(line)

445 ms ± 9.8 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

%%timeitfig, ax = plt.subplots()lc = LineCollection([[(x, 0), (x, 1)] for x in xs])ax.add_collection(lc)

39.7 ms ± 1.13 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

@brunobeltran
Copy link
ContributorAuthor

Thanks@timhoffm for your detailed read! I'm happy to go through more rounds if you have more suggestions. Obviously there's a never-ending amount to do here, but if I feel like we start to get into "this should be a separate PR" territory, I'll let you know 😅

@brunobeltranbrunobeltranforce-pushed thecollections-rcparams-links branch frome1cba73 toe98db08CompareJune 5, 2020 19:31
@brunobeltranbrunobeltranforce-pushed thecollections-rcparams-links branch from46d4a4b to92c7399CompareJune 5, 2020 22:43
@brunobeltran
Copy link
ContributorAuthor

brunobeltran commentedJun 5, 2020
edited
Loading

@timhoffm, round two of comments done, let me know if you have any more gripes with the changes (maybe after you get some sleep!) :)

@brunobeltranbrunobeltranforce-pushed thecollections-rcparams-links branch from92c7399 to8bbdf68CompareJune 6, 2020 02:35

%(Collection)s
where *onoffseq* is an even length tuple of on and off ink lengths
Copy link
Member

Choose a reason for hiding this comment

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

I think odd-length sequences are allowed, see#17444. Or is that specifically not supported in collections?

Copy link
ContributorAuthor

@brunobeltranbrunobeltranJun 9, 2020
edited
Loading

Choose a reason for hiding this comment

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

This was only written to match what is currently said inLine2D.set_dashes andLine2D.set_linestyle. I see that we now accept odd-length sequences in our backends, but all the user-facing documentation says otherwise. Are we sure that there's not other third-party backends that assume even length?

I'd personally prefer to make a separate PR fixing this since I'll want to fix it in 4-5 places throughout the documentation to more correctly describe how we do dashes...

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

EDIT: 12+ places where this needs changes. Would definitely prefer to do this in a separate PR.

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.

That's all now 😄.

@brunobeltranbrunobeltranforce-pushed thecollections-rcparams-links branch from8bbdf68 tob98990cCompareJune 9, 2020 22:03
@brunobeltranbrunobeltran added this to thev3.3.0 milestoneJun 9, 2020
@brunobeltranbrunobeltranforce-pushed thecollections-rcparams-links branch fromb98990c toeff114aCompareJune 9, 2020 22:09
@brunobeltran
Copy link
ContributorAuthor

brunobeltran commentedJun 9, 2020
edited
Loading

Nice doing business with you,@timhoffm. If you're happy to one-review merge this, I'll drop another PR on top of it right now to fix all the places where the docs claim we require an even-lengthonoffseq, but let me know if you see anything new that should still be fixed!

timhoffm reacted with heart emoji

@brunobeltranbrunobeltranforce-pushed thecollections-rcparams-links branch fromeff114a to7194e35CompareJune 9, 2020 23:12
@QuLogic
Copy link
Member

QuLogic commentedJun 9, 2020
edited
Loading

This will need a rebase to fix doc builds. which youjust did...

brunobeltran reacted with thumbs up emoji

@brunobeltranbrunobeltranforce-pushed thecollections-rcparams-links branch from7194e35 toefd009fCompareJune 9, 2020 23:35
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.

On-off sequence descriptions can be fixed as a separate PR.

@timhoffmtimhoffm merged commit93c3224 intomatplotlib:masterJun 10, 2020
@timhoffm
Copy link
Member

Thanks for bringing this forward and patiently working through all my comments.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@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.

3 participants
@brunobeltran@QuLogic@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp