Skip to content

Implement (DataFrame|Series).plot.pie in plotly #1971

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 18, 2020
Merged

Implement (DataFrame|Series).plot.pie in plotly #1971

merged 2 commits into from
Dec 18, 2020

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Dec 15, 2020 •
edited
Loading

This PR implements DataFrame.plot.pie in plotly as below:

from databricks import koalas as ks
kdf = ks.DataFrame(
    {'a': [1, 2, 3, 4, 5, 6],
     'b': [100, 200, 300, 400, 500, 600]},
    index=[10, 20, 30, 40, 50, 60])
ks.options.plotting.backend = 'plotly'
kdf.plot.pie(y="b")

Screen Shot 2020-12-17 at 3 28 12 PM

Binder to test: https://mybinder.org/v2/gh/HyukjinKwon/koalas/plotly-pie?filepath=docs%2Fsource%2Fgetting_started%2F10min.ipynb

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Dec 15, 2020

This PR is dependent on #1970

@HyukjinKwon HyukjinKwon marked this pull request as draft December 15, 2020 07:04
@HyukjinKwon HyukjinKwon changed the title Implement DataFrame.plot.pie in plotly c90be59 Implement DataFrame.plot.pie in plotly Dec 15, 2020
@HyukjinKwon HyukjinKwon mentioned this pull request Dec 15, 2020
Closed
10 tasks
@HyukjinKwon HyukjinKwon changed the title Implement DataFrame.plot.pie in plotly Implement (DataFrame|Series).plot.pie in plotly Dec 15, 2020
@codecov-io
Copy link

codecov-io commented Dec 15, 2020 •
edited
Loading

Codecov Report

Merging #1971 (b3cd2a6) into master (37f7e50) will increase coverage by 0.00%.
The diff coverage is 97.36%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1971   +/-   ##
=======================================
  Coverage   94.59%   94.60%           
=======================================
  Files          49       50    +1     
  Lines       10870    10905   +35     
=======================================
+ Hits        10283    10317   +34     
- Misses        587      588    +1
Impacted Files Coverage Δ
databricks/koalas/plot/plotly.py 94.73% <94.73%> (ø)
databricks/koalas/plot/core.py 91.72% <100.00%> (+0.17%) ⬆️
...bricks/koalas/tests/plot/test_frame_plot_plotly.py 100.00% <100.00%> (ø)
...ricks/koalas/tests/plot/test_series_plot_plotly.py 96.61% <100.00%> (+0.31%) ⬆️

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 37f7e50...b3cd2a6. Read the comment docs.

@HyukjinKwon HyukjinKwon marked this pull request as ready for review December 17, 2020 01:47
ueshin
ueshin reviewed Dec 17, 2020
View reviewed changes
Copy link
Collaborator

@ueshin ueshin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like matplotlib backend shows the index instead of its values as the legends:

Screen Shot 2020-12-16 at 6 48 27 PM

whereas:

Screen Shot 2020-12-16 at 6 50 46 PM

Should we follow it?

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Dec 17, 2020

Ah .. I don't know why I missed #1971 (review). Sure, let me address it.

@itholic
Copy link
Contributor

itholic commented Dec 17, 2020

Have a little question.

Seems like pandas plot.pie doesn't work as same way Koalas' as below. Is it expected ?

Screen Shot 2020-12-17 at 1 13 58 PM

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Dec 17, 2020

Yeah, pandas doesn't support. This is something Koalas only supports.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Dec 17, 2020

Oh, sorry I misread. Okay, that seems a bit different. Let me take a look too.

@HyukjinKwon HyukjinKwon marked this pull request as draft December 17, 2020 05:38
@HyukjinKwon HyukjinKwon marked this pull request as ready for review December 17, 2020 08:03
HyukjinKwon
HyukjinKwon commented Dec 17, 2020
View reviewed changes
databricks/koalas/tests/plot/test_frame_plot_plotly.py
kdf1 = self.kdf1
check_pie_plot(kdf1)

# TODO: support multi-index columns
Copy link
Member Author

@HyukjinKwon HyukjinKwon Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not supported in plotly .. 😢

xinrong-meng
xinrong-meng reviewed Dec 17, 2020
View reviewed changes
databricks/koalas/plot/core.py
return self(kind="area", x=x, y=y, **kwds)

def pie(self, y=None, **kwds):
def pie(self, **kwds):
Copy link
Contributor

@xinrong-meng xinrong-meng Dec 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we annotate the return type here?

Copy link
Member Author

@HyukjinKwon HyukjinKwon Dec 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add them all in a separate PR.

Copy link
Contributor

@xinrong-meng xinrong-meng Dec 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

ueshin
ueshin approved these changes Dec 17, 2020
View reviewed changes
Copy link
Collaborator

@ueshin ueshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Dec 18, 2020

Thanks guys!

@HyukjinKwon HyukjinKwon merged commit b81afcc into databricks:master Dec 18, 2020
@itholic
Copy link
Contributor

itholic commented Dec 18, 2020

Great

@xinrong-meng
Copy link
Contributor

xinrong-meng commented Dec 18, 2020

Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ueshin ueshin ueshin approved these changes

@itholic itholic Awaiting requested review from itholic

+1 more reviewer

@xinrong-meng xinrong-meng xinrong-meng left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@HyukjinKwon @codecov-io @itholic @xinrong-meng @ueshin