Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Clean up and enhance plot method docstrings #5285
Clean up and enhance plot method docstrings #5285
Changes from 10 commits
eae252d
d83a6b0
2777557
83a645c
f7bd01e
c2ab27d
7c188c6
88897eb
16e1ac9
fb96fe2
6c543de
f0c6bbb
8216d3c
4328a72
78c77ed
9827e33
130287d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@keewis would you mind opining on the best format for literals in docstrings?
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 mean,
str
literals? We're currently not consistent in our code base so I think we're free to choose. If we choose to use double-backticked quoted strings (which looks better in the rendered version and slightly decreases the readability inpydoc
/ source code) I would vote for double quotes ("
) instead of single quotes'
because that's what we use in the code.Also, would it make sense to replace
str
with{"discrete", "continuous"}
?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 a case for single-quoted strings (+ double-backticked) in docs is that Python uses single quotes for the repr of strings. But in this PR I just picked one and tried to make it consistent. I am happy to change to double.
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 tweaked the discrete vs continuous thing a bit in 78c77ed if anyone wants to check what I wrote for
hue_style
.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.
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.
How about:
the :py:class:`~matplotlib.colors.Normalize` instance
?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.
wouldn't it be easier to just refer to the object by name? I don't think we need the link because it's already in the type spec.
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.
Yeah, probably better to keep it simple.
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 guess "must be
None
" isn't really true, because it can also be the same value as in the norm. But maybe that isn't important. But maybe there is a better way to word this description.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 don't think that's right,
color-like
doesn't have to be astr
, it can also be a tuple of floats describing RGB or RGBA values: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.
From
pyplot.contour
:I think I tried and found it to be the case with
xarray.plot.contour
as well. That is, it can be a string, or it can be an array-like of color-like values.Edit: just checked it again. For example,
colors=(0, 0, 0)
raisesValueError: Invalid RGBA argument: 0
, butcolors=[(0, 0, 0)]
orcolors="black"
are fine.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.
ds.plot.scatter(..., colors=(1, 0, 0))
does not complain, but I can't get it to change the color of the scatter marks (not even withcolors="r"
) so I might be missing something.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.
Yeah, it seems like the
colors
argument is not used inds.plot.scatter
. But no error is raised,colors
is just silently dropped (ax.scatter
would raiseAttributeError
if it weren't) in most cases. I did find that you can get the marker colors to change by passingcolor
orc
with a discretehue_style
.However, if you have a continuous
hue_style
, passingc
is ignored (since this is done internally), and passingcolor
raisesValueError
since it conflicts withc
.It seems like at the moment,
colors
is really only intended to be used with contour(f) for levels, like how it is in Matplotlib. Maybe in the future it could be used to allow passing colors to be used in the color cycle for discretehue_style
, but it doesn't currently do that. So for now, maybe in the docstring we should note this current behavior (doing nothing or raising error).actually I was able to getlevels
is also unused in_dsplot
functions, maybe could be removedcolors
to sort of work with discretehue_style
by also providinglevels
, butlevels
only makes sense for numeric type: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 we can address this by raising appropriate errors (or implementing it) in a future 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.
Ok, I am interested in working on this
colors
stuff. If that is ok, I will open new issue from my comment above and go from there.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.
sounds great.