Skip to content

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Dec 14, 2020

This PR proposes to simplify plot implementation. Current Koalas implementation attempts to map the argument between other plotting backends (e.g., matplotlib vs plotly). Keeping this map is a huge maintenance cost and it's unrealistic to track their change and keep updating this map.

Pandas itself does not keep this map either:

>>> import pandas as pd
>>> pd.DataFrame([1,2,3]).plot.line(logx=1)
<AxesSubplot:>
>>> pd.options.plotting.backend = "plotly"
>>> pd.DataFrame([1,2,3]).plot.line(logx=1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/.../opt/miniconda3/envs/python3.8/lib/python3.8/site-packages/pandas/plotting/_core.py", line 1017, in line
    return self(kind="line", x=x, y=y, **kwargs)
  File "/.../opt/miniconda3/envs/python3.8/lib/python3.8/site-packages/pandas/plotting/_core.py", line 879, in __call__
    return plot_backend.plot(self._parent, x=x, y=y, kind=kind, **kwargs)
  File "/.../miniconda3/envs/python3.8/lib/python3.8/site-packages/plotly/__init__.py", line 102, in plot
    return line(data_frame, **kwargs)
TypeError: line() got an unexpected keyword argument 'logx'

@HyukjinKwon HyukjinKwon marked this pull request as draft December 14, 2020 10:43

if isinstance(data, Series):
positional_args = {
("ax", None),
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this mapping was copied from pandas, which is actually not needed in our case since we're dispatching it in each method e.g.) KoalasPlotAccessor.area.

@codecov-io
Copy link

codecov-io commented Dec 14, 2020

Codecov Report

Merging #1970 (d844086) into master (d1babcc) will decrease coverage by 0.01%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1970      +/-   ##
==========================================
- Coverage   94.60%   94.58%   -0.02%     
==========================================
  Files          49       49              
  Lines       10890    10866      -24     
==========================================
- Hits        10302    10278      -24     
  Misses        588      588              
Impacted Files Coverage Δ
databricks/koalas/config.py 99.00% <ø> (ø)
databricks/koalas/plot/core.py 91.54% <91.66%> (-1.23%) ⬇️

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 d1babcc...d844086. Read the comment docs.

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.

@ueshin
Copy link
Collaborator

ueshin commented Dec 16, 2020

Thanks! merging.

@ueshin ueshin merged commit 37f7e50 into databricks:master Dec 16, 2020
@xinrong-meng
Copy link
Contributor

Awesome!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants