-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: (Series|Dataframe).plot.hist() #420
Conversation
06fb1e6
to
84f816d
Compare
84f816d
to
a2ac563
Compare
import pandas as pd | ||
|
||
import bigframes.constants as constants | ||
from bigframes.operations._matplotlib.core import MPLPlot |
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.
import modules not methods/classes.
https://google.github.io/styleguide/pyguide.html#s2.2-imports
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.
Fixed. Thanks!
bigframes/operations/plot.py
Outdated
) | ||
kwargs["by"] = by | ||
kwargs["bins"] = bins | ||
return plotbackend.plot(self._parent.copy(), kind="hist", **kwargs) |
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 explain a bit more in the comments what's going on here?
Do we need to add a TODO for supporting other plotbackends, for example?
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.
Added a comment here. We may introduce other backends for interactive visualization support in the future.
@@ -58,6 +58,7 @@ | |||
"tabulate >= 0.9", | |||
"ipywidgets >=7.7.1", | |||
"humanize >= 4.6.0", | |||
"matplotlib >= 3.7.1", |
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.
Please add this to https://github.com/googleapis/python-bigquery-dataframes/blob/main/testing/constraints-3.10.txt too so that we know we always test against our advertised minimum version in at least one test session.
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.
matplotlib==3.7.1
has been added into this file earlier. The system-3.10
tests are not triggerred according to
python-bigquery-dataframes/noxfile.py
Line 57 in 60594f4
SYSTEM_TEST_PYTHON_VERSIONS = ["3.9", "3.11"] |
If so, should I add the matplotlib
to contraints-3.9.txt
also?
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.
Oh, you're right. Yes add to contraints-3.9.txt
. I was looking at the list of files sorted alphabetically.
@@ -1557,6 +1558,10 @@ def __array_ufunc__( | |||
def str(self) -> strings.StringMethods: | |||
return strings.StringMethods(self._block) | |||
|
|||
@property | |||
def plot(self): |
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.
Docstring, please.
Also, please add PlotAccessor to https://github.com/googleapis/python-bigquery-dataframes/blob/main/docs/reference/bigframes.pandas/series.rst
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.
Thanks! Added PlotAccessor
to the series.rst
and frames.rst
.
As for the docstring for def plot
, it is defined at third_party/bigframes_vendored/pandas/core/series.py
a2ac563
to
82797de
Compare
39b6c63
to
e71d19c
Compare
.. automodule:: bigframes.operations.plotting | ||
:members: | ||
:inherited-members: | ||
:undoc-members: |
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.
Per https://github.com/googleapis/python-bigquery-dataframes/actions/runs/8240146608/job/22534948373?pr=420 failure, let's add :noindex:
here.
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.
Thanks for checking! Fixed.
|
||
self.axes = hist_x_pd.plot.hist( | ||
bins=bin_edges, | ||
weights=np.array(weights_pd.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.
Is this method still not working on pandas 1.5 or did the fillna solve that?
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.
the fillna solves the issue in pandas 1.5. I can add a comment above calling.
internal bug: b/322178004