Add some examples for plot implementations in their docstrings #847

itholic · 2019-09-30T04:15:00Z

Basically we finished the rough implementation of all plots (except 2 in DataFrame).

So it should be better to have some examples for them.

codecov-io · 2019-09-30T06:17:43Z

Codecov Report

Merging #847 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #847      +/-   ##
==========================================
+ Coverage   94.27%   94.28%   +<.01%     
==========================================
  Files          34       34              
  Lines        6150     6154       +4     
==========================================
+ Hits         5798     5802       +4     
  Misses        352      352

Impacted Files	Coverage Δ
databricks/koalas/plot.py	`94.28% <ø> (ø)`	⬆️
databricks/conftest.py	`97.87% <100%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1003df7...eaa981e. Read the comment docs.

databricks/koalas/plot.py

HyukjinKwon · 2019-09-30T06:48:27Z

databricks/koalas/plot.py

+            :context: close-figs
+
+            >>> s = ks.Series([1, 3, 2])
+            >>> ax = s.plot.line()  # doctest: +SKIP


Why should we skip?

@HyukjinKwon This doctest continuously failed on my local test bed like below:

But it actually works well without exception:

so i wanna discussion with others members.

could you test this on your local if when available or have some insight of above failure?

I just undo this skip and pushed again for travis build check. (maybe it will pass on travis server..?)

Failed again. 😢 Do you have any guesswork ??

Well i just try this doctest again with exactly same test in test_series_plot. But still failed.

above passed, but below not.

Anyway, it works! Thanks ! 😸

close-figs doesn't seem to close figures.

995 996 Examples 997 -------- 998 Basic plot. 999 1000 .. plot:: 1001 :context: close-figs 1002 1003 >>> import matplotlib.pyplot as plt 1004 >>> plt.get_fignums() Expected nothing Got: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21] /Users/harutaka/Desktop/projects/koalas/databricks/koalas/plot.py:1004: DocTestFailure

@charlesdong1991 yeah i totally agree with it. I think i should be find better solution for solve this issue to keep consistency & readable of docs.
@harupy you're right. i think maybe close-figs doesn't work properly either. im going to figure out why this happened 😭

Does close-figs has any effect when running tests?

@harupy Oops.. sorry i missed your reply
Anyway, yes it seems have any effect, because they are showing same result when i remove all close-figs options.

databricks/koalas/plot.py

HyukjinKwon · 2019-09-30T06:49:14Z

This is nice. Thanks for working on this @itholic

itholic · 2019-09-30T07:56:23Z

@HyukjinKwon my pleasure.

databricks/koalas/plot.py

ueshin

Can we also add for Series.plot.barh and Series.plot.pie?

ueshin · 2019-09-30T18:19:09Z

databricks/koalas/plot.py

+            :context: close-figs
+
+            >>> import matplotlib.pyplot as plt
+            >>> plt.close('all')


Do we still need this?
Seems like others don't have this, otherwise we need to add for the others?

@ueshin at above discussions, maybe there is an issue that :context: close-figs doesn't work properly. so now it wouldn't work if we remove the line plt.close('all') to explicitly close all plots. i'm going to figure out how can we solve this problem.

@itholic
I think :context: close-figs is just for Sphinx, and does nothing when pytest runs tests.

So when running tests, all the plots are drawn onto the same ax and get messed up.

@harupy Thanks for comment!! It really makes sense. Then i think maybe we need to skip these all examples on pytest, and added them separate unittest (actually most of them already tested in unittest suite though)

but I'm not sure if this is right because plt.close("all") is also called on tests which are not related to plots.

ah? sorry i can't get it perfectly 😢 maybe could you let me know where they are using for tests not related to plots?

plt.close is called on test functions where we don't need to call plt.close.

got it :)
but maybe i think they are only called when we explicitly write the :context: close-figs in doctest, ain't they?

ueshin · 2019-09-30T21:25:28Z

Btw, seems like dev/lint-python will fail after docs are built since some python files are generated in ./docs/build/html/reference/api.

starting pycodestyle test...
pycodestyle checks failed:
./docs/build/html/reference/api/databricks-koalas-frame-DataFrame-plot-bar-1.py:1:25: E231 missing whitespace after ':'
./docs/build/html/reference/api/databricks-koalas-frame-DataFrame-plot-bar-1.py:1:48: E231 missing whitespace after ':'
./docs/build/html/reference/api/databricks-koalas-frame-DataFrame-plot-hist-1.py:3:12: E251 unexpected spaces around keyword / parameter equals
./docs/build/html/reference/api/databricks-koalas-frame-DataFrame-plot-hist-1.py:3:14: E251 unexpected spaces around keyword / parameter equals
./docs/build/html/reference/api/databricks-koalas-frame-DataFrame-plot-pie-1.py:1:40: E203 whitespace before ','
./docs/build/html/reference/api/databricks-koalas-frame-DataFrame-plot-line-1.py:5:4: E121 continuation line under-indented for hanging indent
./docs/build/html/reference/api/databricks-koalas-frame-DataFrame-plot-line-1.py:7:4: E123 closing bracket does not match indentation of opening bracket's line
./docs/build/html/reference/api/databricks-koalas-frame-DataFrame-plot-barh-1.py:1:25: E231 missing whitespace after ':'
./docs/build/html/reference/api/databricks-koalas-frame-DataFrame-plot-barh-1.py:1:48: E231 missing whitespace after ':'

Can we ignore those files when running pycodestyle?

itholic · 2019-10-01T02:24:22Z

@ueshin Thanks for comment!
I think maybe we already have examples for Series.plot.barh and Series.plot.pie , but maybe is there a problem with that examples or wanna make additional ones? 😃

itholic · 2019-10-01T02:24:41Z

@ueshin And I'm going to make them ignore in pycodestyle! Thanks for review :)

ueshin · 2019-10-01T03:50:08Z

@itholic I mean, when I generated the doc, I couldn't see the plots for Series.plot.barh and Series.plot.pie. I thought we need

        .. plot::
            :context: close-figs

tags for them as well, then we can see the plots?

itholic · 2019-10-01T03:55:04Z

@ueshin oh i got it :) yes we can see the plots in generated docs by adding .. plot::

i'm going to add them to Series.plot.barh & Series.plot.pie either for visualize plots in docs 😄

ueshin · 2019-10-01T15:47:56Z

databricks/koalas/plot.py

+        .. plot::
+            :context: close-figs
+
+        >>> df = ks.DataFrame({'mass': [0.330, 4.87, 5.97],
        ...                    'radius': [2439.7, 6051.8, 6378.1]},
        ...                   index=['Mercury', 'Venus', 'Earth'])
        >>> plot = df.mass.plot.pie(figsize=(5, 5))


nit: indent should be one more level?

Thanks for comment!! fixed it :)

ueshin · 2019-10-01T15:50:55Z

databricks/koalas/plot.py

+@pytest.fixture(autouse=True)
+def close_figs():
+    yield
+    plt.close("all")


We should move this to conftest.py?

Thanks again, i moved this to conftest.py :)

ueshin

LGTM, pending tests.

ueshin · 2019-10-01T17:02:23Z

oh, I forgot about #847 (comment).
Seems like pycodestyle still fails after creating docs.

starting pycodestyle test...
pycodestyle checks failed:
./docs/build/html/reference/api/databricks-koalas-series-Series-plot-line-1.py:1:17: E203 whitespace before ','
./docs/build/html/reference/api/databricks-koalas-series-Series-plot-line-1.py:1:18: E231 missing whitespace after ','
./docs/build/html/reference/api/databricks-koalas-series-Series-plot-line-1.py:1:20: E203 whitespace before ','
./docs/build/html/reference/api/databricks-koalas-series-Series-plot-line-1.py:1:21: E231 missing whitespace after ','

itholic · 2019-10-02T03:58:02Z

@ueshin oops, sorry i'm going to fix them soon

itholic · 2019-10-02T05:28:55Z

@ueshin Suddenly, i have some question. Is it right way we ignore these tests rather than fix lint fails on doctest in created docs? maybe is it better let them as is to keep lint consistency in documentation?

softagram-bot · 2019-10-02T05:30:23Z

Softagram Impact Report for pull/847 (head commit: `eaa981e`)

⭐ Change Overview

(Open in Softagram Desktop for full details)

📄 Full report

Permalink: Full report for pull/847

Impact Report explained. Give feedback on this report to [email protected]

ueshin · 2019-10-02T06:23:55Z

We should definitely keep lint consistency even in documentations.
But the problem here is the generated .py files under the ./docs/build won't follow the rule.
Could you try dev/lint-python after generating docs and see what happens?
Now flake8 shows a lot of errors.
But maybe we should fix them in a separate PR. WDYT? @HyukjinKwon

itholic · 2019-10-02T06:27:44Z

@ueshin ah, i just tried, understood why we should ignore them. thanks for explanation! 👍

ueshin

I'd merge this for now.

@itholic Please submit another PR to fix lint-python for the generated codes, and address the comment below if needed.

databricks/koalas/plot.py

ueshin · 2019-10-02T21:13:38Z

Hmm, our official doc doesn't show all the plots. https://koalas.readthedocs.io/en/latest/reference/series.html#plotting
It shows bar, barh, or pie, but doesn't the others.
We might miss something for Read the Docs service.

ueshin · 2019-10-03T00:09:54Z

Unsupported class file major version 55

HyukjinKwon · 2019-10-03T02:22:37Z

@itholic can you take a look and make a fix?

itholic · 2019-10-03T03:58:22Z

@ueshin @HyukjinKwon , okay i'm going to take a look at it and summit another PR for fix them.

Related with #847 (review) , Fixed flake8 to ignore "docs/build/html/reference/api/*.py" in my local test, it works well, and all plots visible in documentation like below: <img width="927" alt="스크린샷 2019-10-03 오후 2 19 36" src="https://user-images.githubusercontent.com/44108233/66101387-a0d58800-e5e9-11e9-813d-fc48ce117327.png">

Related with databricks/koalas#847 (review) , Fixed flake8 to ignore "docs/build/html/reference/api/*.py" in my local test, it works well, and all plots visible in documentation like below: <img width="927" alt="스크린샷 2019-10-03 오후 2 19 36" src="https://user-images.githubusercontent.com/44108233/66101387-a0d58800-e5e9-11e9-813d-fc48ce117327.png">

itholic added 3 commits September 30, 2019 13:14

Add some examples for plot implementations in their docstrings

351cc6a

Revert useless changes

b6eee15

Add sphinx extension for visualize plot results

c5a89ff

HyukjinKwon reviewed Sep 30, 2019

View reviewed changes

databricks/koalas/plot.py Outdated Show resolved Hide resolved

HyukjinKwon reviewed Sep 30, 2019

View reviewed changes

databricks/koalas/plot.py Outdated Show resolved Hide resolved

Fix indentation & undo skip line test

d091b2f

Change example case for series line plot

c8ae4be

harupy reviewed Sep 30, 2019

View reviewed changes

databricks/koalas/plot.py Outdated Show resolved Hide resolved

itholic added 3 commits September 30, 2019 22:23

Fix indentation

1a87912

Close existing plot for example doctest

4a1d9ff

Fix pep8

1f538ff

ueshin reviewed Sep 30, 2019

View reviewed changes

itholic added 2 commits October 1, 2019 15:31

fix line doctest to timeseries

98bf73b

Resolve close-fig issue

0f04833

ueshin reviewed Oct 1, 2019

View reviewed changes

Move close_fig to conftest.py

5cc3e6a

ueshin approved these changes Oct 1, 2019

View reviewed changes

Resolve conflicts

0161074

fix lint in doctest

eaa981e

ueshin reviewed Oct 2, 2019

View reviewed changes

databricks/koalas/plot.py Show resolved Hide resolved

ueshin merged commit 89877ce into databricks:master Oct 2, 2019

itholic mentioned this pull request Oct 3, 2019

Add options to ignore specific cases when flake8 test #865

Merged

itholic deleted the add_examples_plot branch October 3, 2019 07:36

HyukjinKwon mentioned this pull request Oct 4, 2019

Fix plots on readthedocs. #889

Merged

Add some examples for plot implementations in their docstrings #847

Add some examples for plot implementations in their docstrings #847

Conversation

itholic commented Sep 30, 2019

codecov-io commented Sep 30, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itholic Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itholic Sep 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Sep 30, 2019

itholic commented Sep 30, 2019

ueshin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harupy Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harupy Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harupy Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

ueshin commented Sep 30, 2019

itholic commented Oct 1, 2019 • edited Loading

itholic commented Oct 1, 2019

ueshin commented Oct 1, 2019 • edited Loading

itholic commented Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itholic Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

ueshin left a comment

Choose a reason for hiding this comment

ueshin commented Oct 1, 2019 • edited Loading

itholic commented Oct 2, 2019

itholic commented Oct 2, 2019 • edited Loading

softagram-bot commented Oct 2, 2019

Softagram Impact Report for pull/847 (head commit: eaa981e)

⭐ Change Overview

📄 Full report

ueshin commented Oct 2, 2019

itholic commented Oct 2, 2019

ueshin left a comment

Choose a reason for hiding this comment

ueshin commented Oct 2, 2019 • edited Loading

ueshin commented Oct 3, 2019

HyukjinKwon commented Oct 3, 2019

itholic commented Oct 3, 2019

codecov-io commented Sep 30, 2019 •

edited

Loading

itholic Sep 30, 2019 •

edited

Loading

itholic Sep 30, 2019 •

edited

Loading

harupy Oct 1, 2019 •

edited

Loading

harupy Oct 1, 2019 •

edited

Loading

harupy Oct 1, 2019 •

edited

Loading

itholic commented Oct 1, 2019 •

edited

Loading

ueshin commented Oct 1, 2019 •

edited

Loading

itholic commented Oct 1, 2019 •

edited

Loading

itholic Oct 1, 2019 •

edited

Loading

ueshin commented Oct 1, 2019 •

edited

Loading

itholic commented Oct 2, 2019 •

edited

Loading

Softagram Impact Report for pull/847 (head commit: `eaa981e`)

ueshin commented Oct 2, 2019 •

edited

Loading