Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
DOCS: update collections.py docstrings to current doc conventions#17575
Uh oh!
There was an error while loading.Please reload this page.
Conversation
2afbe5f
tof87cc48
CompareThere was a problem hiding this 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.
lib/matplotlib/collections.py Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.Artist
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this true though? WhileCollection
inherits 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 reasonCollection
doesn'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.).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 😅 |
e1cba73
toe98db08
CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
46d4a4b
to92c7399
Comparebrunobeltran commentedJun 5, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@timhoffm, round two of comments done, let me know if you have any more gripes with the changes (maybe after you get some sleep!) :) |
92c7399
to8bbdf68
CompareUh oh!
There was an error while loading.Please reload this page.
%(Collection)s | ||
where *onoffseq* is an even length tuple of on and off ink lengths |
There was a problem hiding this comment.
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?
brunobeltranJun 9, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this 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 😄.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
8bbdf68
tob98990c
Compareb98990c
toeff114a
Comparebrunobeltran commentedJun 9, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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-length |
eff114a
to7194e35
CompareQuLogic commentedJun 9, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
7194e35
toefd009f
CompareThere was a problem hiding this 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.
Thanks for bringing this forward and patiently working through all my comments. |
Uh oh!
There was an error while loading.Please reload this page.
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 on
master
, so I went ahead and opened the PR to see if readthedocs has better luck than I.PR Checklist