DOC: Exposed arguments in plot.kde #19229
Conversation
|
Hello @tommyod! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on February 01, 2018 at 14:12 Hours UTC |
|
Ran the |
TomAugspurger
left a comment
TomAugspurger
left a comment
There was a problem hiding this comment.
Thanks. Could fixup the linting errors, and add a release note in whatsnew/v0.23.0.txt?
pandas/plotting/_core.py
Outdated
| sample_range = np.nanmax(y) - np.nanmin(y) | ||
| ind = np.linspace(np.nanmin(y) - 0.5 * sample_range, | ||
| np.nanmax(y) + 0.5 * sample_range, 1000) | ||
| elif isinstance(self.ind, (int, np.int)): |
There was a problem hiding this comment.
Use isinstance(self.ind, numbers.Integral). It should catch both of these (may have to import numbers).
There was a problem hiding this comment.
instead of this, just use is_integer, which is our version of this and handles all the cases.
import from pandas.core.dtypes.common (if not already imported)
| pytest.skip("mpl is not supported") | ||
|
|
||
| from numpy import linspace | ||
| _check_plot_works(self.ts.plot.kde, bw_method=None, ind=20) |
There was a problem hiding this comment.
Could you add a test with np.int(20) as well.
There was a problem hiding this comment.
can you add a use with bw_method as a string.
Codecov Report
@@ Coverage Diff @@
## master #19229 +/- ##
==========================================
- Coverage 91.67% 91.67% -0.01%
==========================================
Files 148 148
Lines 48553 48556 +3
==========================================
Hits 44513 44513
- Misses 4040 4043 +3
Continue to review full report at Codecov.
|
|
@jreback, @TomAugspurger - I have addressed the comments.
|
|
@jreback Friendly reminder to consider this PR, in case it has been forgotten. If there is anything else that needs to be done, please let me know. |
pandas/plotting/_core.py
Outdated
| The method used to calculate the estimator bandwidth. This can be | ||
| 'scott', 'silverman', a scalar constant or a callable. | ||
| If None (default), 'scott' is used. | ||
| See :scipy:class:`stats.gaussian_kde` for more information. |
There was a problem hiding this comment.
Have you verified that this link works? I thought it would be
:func:`scipy.stats.gaussian_kde`.
There was a problem hiding this comment.
Agreed. Changed it.
doc/source/whatsnew/v0.23.0.txt
Outdated
| - :func: `DataFrame.plot` now raises a ``ValueError`` when the ``x`` or ``y`` argument is improperly formed (:issue:`18671`) | ||
| - Bug in formatting tick labels with ``datetime.time()`` and fractional seconds (:issue:`18478`). | ||
| - | ||
| - The arguments ``ind`` and ``bw_method`` are added to the docstring of :meth:`Series.plot.kde` (:issue:`18461`). The argument ``ind`` may now also be an integer (number of sample points). |
There was a problem hiding this comment.
:meth:`Series.plot.kde` has exposed the args......in the doc-string.
| pytest.skip("mpl is not supported") | ||
|
|
||
| from numpy import linspace | ||
| _check_plot_works(self.ts.plot.kde, bw_method=None, ind=20) |
There was a problem hiding this comment.
can you add a use with bw_method as a string.
pandas/tests/plotting/test_series.py
Outdated
| if not self.mpl_ge_1_5_0: | ||
| pytest.skip("mpl is not supported") | ||
|
|
||
| from numpy import linspace |
There was a problem hiding this comment.
don't import here, rather use np.linspace
|
|
||
| from numpy import linspace | ||
| _check_plot_works(self.ts.plot.kde, bw_method=None, ind=20) | ||
| _check_plot_works(self.ts.plot.kde, bw_method=None, ind=np.int(20)) |
There was a problem hiding this comment.
ideally would parametrize this test instead of repeating.
pandas/plotting/_core.py
Outdated
| spaced points are used. | ||
| `**kwds` : optional | ||
| Keyword arguments to pass on to :py:meth:`pandas.DataFrame.plot`. | ||
| Keyword arguments to pass on to :py:meth:`pandas.Series.plot`. |
There was a problem hiding this comment.
is this change right? This the DataFrame one
There was a problem hiding this comment.
Good catch. My bad. It should be pandas.DataFrame.plot, and I have made the change.
|
@jreback I believe this PR is good to go now. |
|
Thanks @tommyod! |
* Exposed arguments in plot.kde, added number of sample points as option * Added a test for plot.kde with as an integer * Added whatsnew. Fixed flake8 errors. Used is_integer to infer type. * Updated scipy reference * Added test, rewrote whatsnew, removed import * Changed from Series to DataFrame in doc * Fixed PEP8 errors in test file * Fixed typo which made tests crash
4 participants
The documentation for
plot.kdedid not show thebw_methodandindarguments, which are specific toplot.kde(andplot.density, which refers to the same method).There is also a change in the actual code, and a corresponding test added. I added an option for
indto be an integer number of sample points, instead of necessarily anp.array. I image that a user typically just wants a number of equidistant sample points, and does not want to construct an array.git diff upstream/master -u -- "*.py" | flake8 --diffComments very welcome.