-
-
Notifications
You must be signed in to change notification settings - Fork 7.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 our terms of service and privacy 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
Conversation
2afbe5f
to
f87cc48
Compare
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.
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.
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 | |
A Collection represents a sequence of `.Artist`\s. It is more efficient to | |
draw compared to using many individual `.Artist` objects. For example, | |
when a single path is being drawn repeatedly at |
Note: You additionally have to make the docstring a raw string r```
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? While Collection
inherits from Artist
, it re-implements basically the entire Patch
interface with plural method/property names. In addition, its properties default to patches.
values from rc
.
My impression from reading the code was that the only reason Collection
doesn't inherit from Patch
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 Python Collection
object is 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 of Patches
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 individual draw_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 with Patch
. I was thinking of LineCollection
which rather mimics a list of Line2D
and thus went for the more general Artist
. This is all a bit messy.
Is it reasonable to stick to Patch
and just mention in LineCollection
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 up draw_path_collection
into individual draw_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 plt
from matplotlib.lines import Line2D
from matplotlib.collections import LineCollection
import numpy as np
xs = np.linspace(0, 1, 1000)
%%timeit
fig, 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)
%%timeit
fig, 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)
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
to
e98db08
Compare
46d4a4b
to
92c7399
Compare
@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
to
8bbdf68
Compare
|
||
%(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?
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 in Line2D.set_dashes
and Line2D.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.
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 😄.
8bbdf68
to
b98990c
Compare
b98990c
to
eff114a
Compare
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
to
7194e35
Compare
|
7194e35
to
efd009f
Compare
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.
On-off sequence descriptions can be fixed as a separate PR.
Thanks for bringing this forward and patiently working through all my comments. |
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