Skip to content
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

DEPR: Series.ptp() #21614

Merged
merged 9 commits into from Jul 6, 2018
Merged

DEPR: Series.ptp() #21614

merged 9 commits into from Jul 6, 2018

Conversation

@KalyanGokhale
Copy link
Contributor

@KalyanGokhale KalyanGokhale commented Jun 24, 2018

xref #18262

  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@codecov
Copy link

@codecov codecov bot commented Jun 24, 2018

Codecov Report

Merging #21614 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #21614      +/-   ##
==========================================
+ Coverage    91.9%   91.93%   +0.03%     
==========================================
  Files         154      159       +5     
  Lines       49562    49734     +172     
==========================================
+ Hits        45549    45722     +173     
+ Misses       4013     4012       -1
Flag Coverage Δ
#multiple 90.31% <100%> (ø) ⬆️
#single 41.99% <0%> (+0.14%) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 96.21% <100%> (-0.01%) ⬇️
pandas/core/indexes/datetimes.py 95.45% <0%> (-0.22%) ⬇️
pandas/core/indexes/timedeltas.py 91.05% <0%> (-0.2%) ⬇️
pandas/core/dtypes/cast.py 88.36% <0%> (-0.13%) ⬇️
pandas/core/indexes/accessors.py 90.09% <0%> (-0.1%) ⬇️
pandas/core/internals.py 95.53% <0%> (-0.07%) ⬇️
pandas/tseries/offsets.py 97.15% <0%> (-0.02%) ⬇️
pandas/core/frame.py 97.19% <0%> (ø) ⬆️
pandas/core/arrays/__init__.py 100% <0%> (ø) ⬆️
... and 31 more

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 36422a8...cede496. Read the comment docs.

@@ -83,7 +83,7 @@ Deprecations
~~~~~~~~~~~~

- :meth:`DataFrame.to_stata`, :meth:`read_stata`, :class:`StataReader` and :class:`StataWriter` have deprecated the ``encoding`` argument. The encoding of a Stata dta file is determined by the file type and cannot be changed (:issue:`21244`).
-
- :meth:`Series.ptp` is deprecated. Use numpy.ptp instead (:issue:`21614`)
Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

you can put a reference to numpy.ptp as well

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jun 27, 2018

Choose a reason for hiding this comment

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

done - updated for all comments Thanks

@@ -8871,8 +8871,15 @@ def _add_series_only_operations(cls):
axis_descr, name, name2 = _doc_parms(cls)

def nanptp(values, axis=0, skipna=True):
"""
.. deprecated:: 0.24.0
Use numpy.ptp instead
Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

I am not sure this is the right format for the doc-str, cc @jorisvandenbossche

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jun 25, 2018

Choose a reason for hiding this comment

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

Thanks - will wait for confirmation on right format from @jorisvandenbossche

Based on the other deprecation PRs I checked, this seemed to be the expected format - for other messages I had noted earlier I got Travis failures (example below from different PR submitted)

Check for deprecated messages without sphinx directive
pandas/core/indexes/multi.py:        DEPRECATED: to_hierarchical will be removed in a future version.
Check for deprecated messages without sphinx directive DONE
...
The command "ci/lint.sh" exited with 1.

Copy link
Member

@jorisvandenbossche jorisvandenbossche Jun 25, 2018

Choose a reason for hiding this comment

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

Yes, this is the correct format. But, shouldn't this rather be in cls.ptp docstring passed to _make_stat_function below?
(didn't really check, but make sure that pd.Series.ptp? gives the correct docstring)

Copy link
Member

@jorisvandenbossche jorisvandenbossche Jun 26, 2018

Choose a reason for hiding this comment

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

Did a quick check, and indeed adding the deprecation message does not update the actual docstring of Series.ptp, you need to add it to the argument passed to _make_stat_function a few lines below

s = Series([3, 5, np.nan, -3, 10])

# GH21614
Copy link
Contributor

@jreback jreback Jun 25, 2018

Choose a reason for hiding this comment

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

make this another method. you need to catch any warnings for calling s.ptp() anywhere in the codebase, or just call np.ptp

@@ -8871,8 +8871,15 @@ def _add_series_only_operations(cls):
axis_descr, name, name2 = _doc_parms(cls)

def nanptp(values, axis=0, skipna=True):
"""
.. deprecated:: 0.24.0
Use numpy.ptp instead
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jun 26, 2018

Choose a reason for hiding this comment

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

Did a quick check, and indeed adding the deprecation message does not update the actual docstring of Series.ptp, you need to add it to the argument passed to _make_stat_function a few lines below

@KalyanGokhale
Copy link
Contributor Author

@KalyanGokhale KalyanGokhale commented Jun 26, 2018

Thanks @jorisvandenbossche have moved the docstring to _make_stat_function

@jreback jreback added this to the 0.24.0 milestone Jun 28, 2018
Copy link
Contributor

@jreback jreback left a comment

can you remove from doc/api.rst as well, otherwise lgtm.

@KalyanGokhale
Copy link
Contributor Author

@KalyanGokhale KalyanGokhale commented Jun 28, 2018

@jreback thanks - done - all green

@jreback
Copy link
Contributor

@jreback jreback commented Jun 29, 2018

N = 1000
arr = np.random.randn(N)
ser = Series(arr)
assert np.ptp(ser) == np.ptp(arr)

# GH11163
s = Series([3, 5, np.nan, -3, 10])
assert s.ptp() == 13
assert pd.isna(s.ptp(skipna=False))
with tm.assert_produces_warning(FutureWarning, check_stacklevel=False):
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jul 5, 2018

Choose a reason for hiding this comment

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

Is the check_stacklevel=False needed?

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jul 6, 2018

Choose a reason for hiding this comment

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

Yes - it seems to be required, not for this particular check(i.e. assert s.ptp() == 13), but for e.g. the following

tm.assert_series_equal(s.ptp(level=0), expected)

without it, this test was failing with AssertionError: Warning not set with correct stacklevel.

I also did trial-and-error with different stacklevels, without any success (though stacklevel=4 seems to be appropriate for where the warning is raised)

Any suggestions? Thanks

"""
.. deprecated:: 0.24.0
Use numpy.ptp instead
Returns the difference between the maximum value and the
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jul 5, 2018

Choose a reason for hiding this comment

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

This needs a blank line above this one. Can you also put the deprecated part below the text?

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jul 6, 2018

Choose a reason for hiding this comment

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

Thanks - Done - let me know if the deprecated note below main text needs any edits, have also added the line Use numpy.ptp instead again

``numpy.ndarray`` method ``ptp``.""",
``numpy.ndarray`` method ``ptp``.
.. deprecated:: 0.24.0
Copy link
Contributor

@jreback jreback Jul 6, 2018

Choose a reason for hiding this comment

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

hmm this is duplicated here

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jul 6, 2018

Choose a reason for hiding this comment

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

This is based on comment from @jorisvandenbossche
Can you also put the deprecated part below the text? - waiting for his reply if the edit was as expected

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jul 6, 2018

Choose a reason for hiding this comment

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

My bad - misinterpreted his review and duplicated in hurry - fixing now

Copy link
Contributor Author

@KalyanGokhale KalyanGokhale Jul 6, 2018

Choose a reason for hiding this comment

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

Done thanks

2nd update removed duplicated deprecation comment
@jorisvandenbossche jorisvandenbossche merged commit f8977a4 into pandas-dev:master Jul 6, 2018
3 checks passed
@KalyanGokhale KalyanGokhale deleted the PTP branch Jul 6, 2018
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this issue Oct 1, 2018
@dragoljub
Copy link

@dragoljub dragoljub commented Oct 25, 2018

Just curious why Series.ptp() was deprecated? It was very useful shorthand for finding the period range of a DateTime column.

df.DATE_TIME.ptp()

Timedelta('56 days 01:04:15')

@jreback
Copy link
Contributor

@jreback jreback commented Oct 25, 2018

sure but it’s just one more method to maintain

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants