Update visualize docstrings#6383
Update visualize docstrings#6383mrocklin merged 3 commits intodask:masterfrom zzhengnan:fix-docstring
visualize docstrings#6383Conversation
mrocklin
left a comment
There was a problem hiding this comment.
Thank you for your work @ZhengnanZhao !
I've added a couple of comments and suggestions below.
dask/base.py
Outdated
| Examples | ||
| -------- | ||
| >>> x.visualize(filename='dask.pdf') # doctest: +SKIP | ||
| >>> x.visualize(filename='dask') # doctest: +SKIP |
There was a problem hiding this comment.
Hrm, even though we choose to add an extension, I don't think that we should encourage users to do this. I would rather that they be explicit. This seems like good practice generally, and also that way if we choose to reverse this decision in the future (which seems possible) then fewer people will be upset with us.
| >>> x.visualize(filename='dask') # doctest: +SKIP |
There was a problem hiding this comment.
Can you explain a bit more what you mean by "explicit"? Are you saying we should encourage users to use .visualize(filename='dask', format='pdf') over .visualize(filename='dask.pdf'), or is it the other way around?
dask/base.py
Outdated
| For example a Dask.array turns into a :func:`numpy.array` and a Dask.dataframe | ||
| turns into a Pandas dataframe. The entire dataset must fit into memory | ||
| For example a Dask.array turns into a :func:`numpy.array` and a Dask.dataframe | ||
| turns into a :func:`pandas.dataframe`. The entire dataset must fit into memory |
There was a problem hiding this comment.
I'm not sure I understand these changes. I think that you're trying to do cross sphinx linking. If so, I think that you actually want pandas.DataFrame (note capitalization) and numpy.ndarray. Regardless, if you want to make this change then I encourage you to try building the docs (see https://docs.dask.org/en/latest/develop.html#contributing-to-documentation) and verify that this is an improvement.
There was a problem hiding this comment.
Oh I wasn't thinking that. I added :func: around pandas.dataframe simply to keep consistency with what was already done for numpy.array on the previous line.
I'll look into the linked resource and test it out.
There was a problem hiding this comment.
@mrocklin Following instructions in the link above, I wasn't able to see the change take effect
- Are the official docs based on contents in the
docsdirectory of this repo? - If so, it looks like this particular docstring isn't referenced anywhere in the docs. I did a code search for
For example a Dask.array turns intothroughout the codebase and the only hit was this docstring; explicitly, it's not referenced in any HTML files
There was a problem hiding this comment.
- Yes
- In that case it probably doesn't make sense to use sphinx-style entires like this, and we should go with normal human-readable text like
Pandas dataframerather than:func:pandas.dataframe
There was a problem hiding this comment.
I agree. Just pushed another commit to address all your feedback. Please re-review.
dask/base.py
Outdated
| Examples | ||
| -------- | ||
| >>> x.visualize(filename='dask.pdf') # doctest: +SKIP | ||
| >>> x.visualize(filename='dask') # doctest: +SKIP |
There was a problem hiding this comment.
| >>> x.visualize(filename='dask') # doctest: +SKIP |
|
I mean the other way around. I think that it is better for users to provide
the full filename.
…On Wed, Jul 8, 2020, 9:00 PM Zhengnan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In dask/base.py
<#6383 (comment)>:
> @@ -70,7 +71,7 @@ def visualize(self, filename="mydask", format=None, optimize_graph=False, **kwar
Examples
--------
- >>> x.visualize(filename='dask.pdf') # doctest: +SKIP
+ >>> x.visualize(filename='dask') # doctest: +SKIP
Can you explain a bit more what you mean by "explicit"? Are you saying we
should encourage users to use .visualize(filename='dask', format='pdf')
over .visualize(filename='dask.pdf'), or is it the other way around?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#6383 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTDBT6LWDIX5ACYLUI3R2U6HDANCNFSM4OVEK4LQ>
.
|
|
Thank you for the fix @ZhengnanZhao ! This is in. Also, I notice that this is your first contribution to this repository. Welcome! |
To reflect the fact that the
filenamekwarg can now accept file extensions.dask/dask/dot.py
Lines 276 to 278 in 3903843
black dask/flake8 dask