-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
adding keyword plotting #8566
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
adding keyword plotting #8566
Conversation
examples/misc/keyword_plotting.py
Outdated
====================== | ||
|
||
There are some instances where you have data in a format that lets you | ||
access particular variables with strings. For example, with a ``recarray`` |
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.
At the risk of doing the rst markup police again and again: optimally, recarray
and DataFrame
should be in single backquotes. See e.g. https://raw.githubusercontent.com/anntzer/mplcursors/master/doc/index.rst, look for
:mod:`pandas`’ :class:`DataFrame <pandas.DataFrame>`\s,
rendered at http://mplcursors.readthedocs.io/en/latest/.
(Also check https://github.com/anntzer/mplcursors/blob/master/doc/conf.py#L349).
See also https://mail.zope.org/pipermail/grok-dev/2008-December/006823.html, https://thomas-cokelaer.info/tutorials/sphinx/rest_syntax.html#inline-markup-and-special-characters-e-g-bold-italic-verbatim.
fd1ef5d
to
0ed3497
Compare
that's a good point @anntzer - I actually noticed that it didn't seem like matplotlib had intersphinx activated. Any particular reason for that? Latest push activates the intersphinx extension and adds in some common packages. Also fixes the rst |
0ed3497
to
b97d6ec
Compare
Probably because matplotlib's sphinx setup may predate intersphinx? (it is certainly based on a very old version of sphinx). |
Not anymore! ;-) |
doc/conf.py
Outdated
@@ -88,6 +88,13 @@ def _check_deps(): | |||
|
|||
autodoc_docstring_signature = True | |||
|
|||
intersphinx_mapping = { | |||
'python': ('http://docs.python.org/', None), | |||
'numpy': ('http://docs.scipy.org/doc/numpy-dev/', None), |
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.
We should link to the stable version of numpy docs (https://docs.scipy.org/doc/numpy/)
doc/conf.py
Outdated
intersphinx_mapping = { | ||
'python': ('http://docs.python.org/', None), | ||
'numpy': ('http://docs.scipy.org/doc/numpy-dev/', None), | ||
'scipy': ('http://scipy.github.io/devdocs/', None), |
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.
Same as above, this should be the stable version of the docs (https://docs.scipy.org/doc/scipy/reference/)
examples/misc/keyword_plotting.py
Outdated
@@ -0,0 +1,24 @@ | |||
""" | |||
Plotting with keywords |
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 should have a line of =
signs above the title too I think
examples/misc/keyword_plotting.py
Outdated
np.random.seed(19680801) | ||
|
||
data = dict([(label, np.random.randn(50)) for label in ['a', 'b', 'c', 'd']]) | ||
data['d'] = np.abs(data['d']) * 100 |
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.
Hmm, as much as it's more work in some ways, I feel like an example with a clear structure, so something like:
data = { 'a' : [1,2,3,4,5,5,6,7],
'b' :[4,5, 6,2,3, 4, 5],
'c': [3,1,2,3,4,5,6,3],
'd':[1,2,3,4,5,6,7,8]}
might work better since the point of this is to really clearly see what the kwarg does.
tutorials/01_introductory/pyplot.py
Outdated
# the ``data`` keyword argument. If provided, then you may generate plots with | ||
# the strings corresponding to these variables. | ||
|
||
data = dict([(label, np.random.randn(50)) for label in ['a', 'b', 'c']]) |
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.
same as above
b97d6ec
to
512b4eb
Compare
comments addressed! |
ah I see - you mean using |
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.
👍
doc/conf.py
Outdated
@@ -88,6 +88,13 @@ def _check_deps(): | |||
|
|||
autodoc_docstring_signature = True | |||
|
|||
intersphinx_mapping = { | |||
'python': ('http://docs.python.org/', None), |
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.
https also; would be nice if this intersphinx change were a separate commit.
512b4eb
to
c3f5715
Compare
latest version has curly brackets + https + intersphinx in a diff commit |
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 putting up w/ my nitpicking. Same down here please.
down where? @story645 |
tutorials/01_introductory/pyplot.py
Outdated
# the ``data`` keyword argument. If provided, then you may generate plots with | ||
# the strings corresponding to these variables. | ||
|
||
data = dict([('a', np.arange(50)), |
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.
Down here?
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.
oops - this is how you know I should probably stop coding for the day ;-)
12b80fe
to
a24ca89
Compare
doc/conf.py
Outdated
'python': ('https://docs.python.org/', None), | ||
'numpy': ('https://docs.scipy.org/doc/numpy/', None), | ||
'scipy': ('https://docs.scipy.org/doc/scipy/reference/', None), | ||
'pandas': ('https://pandas.pydata.org/pandas-docs/stable', None) |
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 one's not accessible over https, unfortunately. Also, why two-space indent?
examples/misc/keyword_plotting.py
Outdated
|
||
fig, ax = plt.subplots() | ||
ax.scatter('a', 'b', c='c', s='d', data=data) | ||
ax.set(xlabel='a', ylabel='b') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reas F438 on will be displayed to describe this comment to others. Learn more.
Just to be clear, this doesn't try to somehow pull something out of data
; maybe use something like 'Entry a'/'Column a' or similar to avoid implying that.
examples/misc/keyword_plotting.py
Outdated
:class:`numpy.recarray` or :class:`pandas.DataFrame`. | ||
|
||
Matplotlib allows you provide such an object with | ||
the ``data`` keyword argument. If provided, then you may generate plots with |
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.
Slightly over-eager wrapping.
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 do you mean? Doesn't this need to be double ticks since it's rST?
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 mean the line above this is wrapped at like 50 chars instead of 79.
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.
oh - ok I removed the extra whitespace. is that such a big deal? I prefer to have extra whitespace as it makes it easier to reword etc without having to bump everything down a bunch of lines
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.
Well, it's not major, but I just think it looks odd when one line is broken much earlier than the rest; it didn't even break at the end of a sentence.
a24ca89
to
2e9a8c2
Compare
ok comments addressed |
Sorry, clicked on wrong thing and thought you hadn't changed both examples. |
2e9a8c2
to
4de797f
Compare
no you were right - I had missed the tutorial! |
There seems to be some problems building the docs, but I'm not sure if it's this change or some external problem. |
4de797f
to
57aa332
Compare
I think it's because the tutorial was using |
57aa332
to
b7e31f3
Compare
Seems to be all working now! |
🍻 |
BTW, wherever you originally wrote the commit and where you updated it seem to disagree on your name/email. |
ah that's annoying - I made a commit from my lab computer, must have been set up w a different address. thanks for noticing |
@@ -19,7 +19,7 @@ body { | |||
} | |||
|
|||
a { | |||
color: #CA7900; | |||
color: #CA7900 !important; |
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.
Why make this change? Links no longer change colour on hover.
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.
the reason for this was to make the inline links show up within sphinx-gallery pages (they weren't showing up before). I didn't realize this was going to mess up the other links as well. A better way for me to have done this would be to add a new rule instead:
div.highlight-python a {
color: #CA7900 !important;
}
want me to add this commit to the toolkits 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.
This is fixed in the toolkit PR, works fine now and should be merged soon! Good find
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.
Would that require a :hover
rule as well?
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.
yep there's a new hover in there too
Two short little snippets of code to demo plotting with keywords. In this case I'm using a dictionary but mention that you could use a
DataFrame
if you wanted. Mentioned in #8555