-
Notifications
You must be signed in to change notification settings - Fork 359
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
Introduce plotting.backend configuration with Plotly support #1639
Conversation
@DumbMachine |
I have the version required by (base) ➜ koalas git:(plotly-support) ✗ black --version
black, version 19.10b0 |
databricks/koalas/config.py
Outdated
@@ -220,6 +220,12 @@ def validate(self, v: Any) -> None: | |||
"'plotting.sample_ratio' should be 1.0 >= value >= 0.0.", | |||
), | |||
), | |||
Option( | |||
key="plotting.backend", | |||
doc=("Backend to use for plotting. Default is matplotlib."), |
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.
Could you add a list of currently supported backends?
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.
Also check_func
to avoid setting invalid backend?
I have some trouble understanding the error here. Some tests have problems with trailing |
Let me try to fix it. |
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.
black
seems to autodetect the python version as per each file, so the files can't include new syntax.
Btw, I'm wondering how the other backends, |
Awesome! @DumbMachine |
databricks/koalas/plot.py
Outdated
""" | ||
# function copied from pandas.plotting._core | ||
|
||
import pkg_resources # Delay import for performance. |
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.
Could we move this comment to the above of import pkg_resources
to keep the consistency with other comments if there is no special reason ?
_backends[entry_point.name] = entry_point.load() | ||
|
||
try: | ||
return _backends[backend] |
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.
Can we check if "backend" is in "_backends" and raise an KeyError
manually rather than using try
??
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 use the try
because the _backends
initially does not have any modules. _backends
gets modules assigned to it when the modules are loaded, upon the user's request of the plotting module. The use of:
return _backends[backend]
is that if the use has the module already saved in the _backends
dict, there will be no need for reimporting the package. The image below will example better:
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.
@DumbMachine Thanks for the explanation. let's keep this as it as
databricks/koalas/plot.py
Outdated
@@ -1507,6 +1615,37 @@ class KoalasFramePlotMethods(PandasObject): | |||
``df.plot(kind='hist')`` is equivalent to ``df.plot.hist()`` | |||
""" | |||
|
|||
@staticmethod | |||
def _get_args_map(backend_name, data, kind, kwargs): | |||
"""Appropriate call args and data preprocessing mapping for the backend |
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.
Can we have a simple example for this methods ?
@DumbMachine Thanks for the work for this !! 👍 |
The tests fail I believe due to |
@DumbMachine you can add the libraries you need to the |
The tests that are failing, fail due to lack of |
I'll fix the conda builds. |
ah, |
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.
Otherwise, LGTM. Awesome work!
@HyukjinKwon @itholic Could you take another look?
databricks/koalas/plot.py
Outdated
# Values not being same as default implies user is explicitly passing the arguments | ||
for arg, def_val in positional_args: | ||
if arg in kwargs and kwargs[arg] == def_val: | ||
kwargs.pop(arg, 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.
I guess we should apply this before Line 1274, otherwise ploty specific arguments could have different values?
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.
Made the changes. Now we try to map if possible and remove arguements if default.
@DumbMachine Can I ask you another favor while we are here? e.g.,: kdf = ...
kdf.plot(...) figure ks.option.plotting.backend = ...
kdf.plot(...) figure ... |
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.
Looks good to me too.
I will double check by myself tomorrow and make a followup to clean up. |
Sorry for the late. |
Always happy to help. |
@DumbMachine I'm not sure about the original motivation for having two plotter classes, but it'd be great if we could simplify the plotters. Would you like to work on it? cc @HyukjinKwon |
Yes, I'd like to work on it. I will open a PR for it, soon.
…On Fri, 17 Jul, 2020, 12:00 am Takuya UESHIN, ***@***.***> wrote:
@DumbMachine <https://github.com/DumbMachine> I'm not sure about the
original motivation for having two plotter classes, but it'd be great if we
could simplify the plotters. Would you like to work on it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1639 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFSMMCD42M6NR5XYOVC6GZ3R35BLBANCNFSM4OU5LA7A>
.
|
Sure, nice! |
Aims to fix #1626
Each backend returns the
figure
in their own format, allowing for further editing or customization if required.How to use?: