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 update dedeprecation message for np.ptp #28665 #6581 #30401

Closed

Conversation

hasnain2808
Copy link
Contributor

@hasnain2808 hasnain2808 commented Dec 22, 2019

@hasnain2808 hasnain2808 changed the title update dedeprecation message for np.ptp DEPR #28665 #6581 DEPR update dedeprecation message for np.ptp #28665 #6581 Dec 22, 2019
@jbrockmendel
Copy link
Member

The fix here is not to update the warning but to remove Series.ptp entirely

@hasnain2808
Copy link
Contributor Author

hasnain2808 commented Dec 22, 2019

Here in #28665
#28665 (comment)
It was suggested that till the time this resolved we should update the message

@gfyoung gfyoung added the Deprecate Functionality to remove in pandas label Dec 22, 2019
@@ -10198,7 +10198,10 @@ def nanptp(values, axis=0, skipna=True):
nmin = nanops.nanmin(values, axis, skipna)
warnings.warn(
"Method .ptp is deprecated and will be removed "
"in a future version. Use numpy.ptp instead.",
"in a future version. Use numpy.ptp instead."
"if you are already using numpy.ptp and still getting this message"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"if you are already using numpy.ptp and still getting this message"
"if you are already using numpy.ptp and still getting this message, "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"in a future version. Use numpy.ptp instead.",
"in a future version. Use numpy.ptp instead."
"if you are already using numpy.ptp and still getting this message"
"please call to_numpy() to avoid this message in future calls"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"please call to_numpy() to avoid this message in future calls"
"please call to_numpy() to avoid this message in future calls. "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"in a future version. Use numpy.ptp instead."
"if you are already using numpy.ptp and still getting this message"
"please call to_numpy() to avoid this message in future calls"
"eg : np.ptp(pd.Series([1, 2, 3]).to_numpy())",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"eg : np.ptp(pd.Series([1, 2, 3]).to_numpy())",
"For example: np.ptp(pd.Series([1, 2, 3]).to_numpy())",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

agree with @jbrockmendel let's just remove this entirely. it has been deprecated for a while and we are removing code for 1.0

@hasnain2808
Copy link
Contributor Author

agree with @jbrockmendel let's just remove this entirely. it has been deprecated for a while and we are removing code for 1.0

so, for now, I have updated the message,
I believe it will take some time for getting rid of np,ptp() till then this message should help.

@jreback
Copy link
Contributor

jreback commented Dec 23, 2019

agree with @jbrockmendel let's just remove this entirely. it has been deprecated for a while and we are removing code for 1.0

so, for now, I have updated the message,
I believe it will take some time for getting rid of np,ptp() till then this message should help.

no, for 1.0 we are going to remove this code, if you want to repurpose this PR to do it great.

@hasnain2808
Copy link
Contributor Author

Ohhk no isssues
I would love to do it
As I am new to the codebase it would be really nice if you can give me some pointers around achieving this

@jreback
Copy link
Contributor

jreback commented Dec 23, 2019

Ohhk no isssues
I would love to do it
As I am new to the codebase it would be really nice if you can give me some pointers around achieving this

ok, you remove the code & associated tests, see https://dev.pandas.io/docs/whatsnew/v1.0.0.html#removal-of-prior-version-deprecations-changes for things that we have removed, so add an entry liek this

@hasnain2808
Copy link
Contributor Author

hasnain2808 commented Dec 23, 2019

so I removed code and tests for ptp
remove the part where series only functions are added
removed the entire series only function because ptp was the only series only function
deleted tests for pandas ptp function
but the test written for np.ptp gives me a future warning

`
____________________________________________________________ TestSeriesAnalytics.test_ptp ____________________________________________________________

self = <pandas.tests.series.test_analytics.TestSeriesAnalytics object at 0x7f666e0267b8>

def test_ptp(self):
    # GH21614
    N = 1000
    arr = np.random.randn(N)
    ser = Series(arr)
    with tm.assert_produces_warning(FutureWarning, check_stacklevel=False):
      assert np.ptp(ser) == np.ptp(arr)

pandas/tests/series/test_analytics.py:861:


self = <contextlib._GeneratorContextManager object at 0x7f666e03ad30>, type = None, value = None, traceback = None

def __exit__(self, type, value, traceback):
    if type is None:
        try:
          next(self.gen)

E AssertionError: Did not see expected warning of class 'FutureWarning'

/usr/lib/python3.6/contextlib.py:88: AssertionError
================================================ 1 failed, 163 passed, 1 skipped, 3 xfailed in 1.96s =================================================
`

``numpy.ndarray`` method ``ptp``.""",
nanptp,
)
# @classmethod
Copy link
Member

Choose a reason for hiding this comment

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

delete all of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -4390,7 +4390,7 @@ def to_period(self, freq=None, copy=True):
["index"], docs={"index": "The index (axis labels) of the Series."},
)
Series._add_numeric_operations()
Series._add_series_only_operations()
# Series._add_series_only_operations()
Copy link
Member

Choose a reason for hiding this comment

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

delete. basically never leave commented-out code in a PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually made this commit to get some help on getting this future warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I removed code and tests for ptp
remove the part where series only functions are added
removed the entire series only function because ptp was the only series only function
deleted tests for pandas ptp function
but the test written for np.ptp gives me a future warning

`
____________________________________________________________ TestSeriesAnalytics.test_ptp ____________________________________________________________

self = <pandas.tests.series.test_analytics.TestSeriesAnalytics object at 0x7f666e0267b8>

def test_ptp(self):
    # GH21614
    N = 1000
    arr = np.random.randn(N)
    ser = Series(arr)
    with tm.assert_produces_warning(FutureWarning, check_stacklevel=False):
      assert np.ptp(ser) == np.ptp(arr)

pandas/tests/series/test_analytics.py:861:

self = <contextlib._GeneratorContextManager object at 0x7f666e03ad30>, type = None, value = None, traceback = None

def __exit__(self, type, value, traceback):
    if type is None:
        try:
          next(self.gen)

E AssertionError: Did not see expected warning of class 'FutureWarning'

/usr/lib/python3.6/contextlib.py:88: AssertionError
================================================ 1 failed, 163 passed, 1 skipped, 3 xfailed in 1.96s =================================================
`

Copy link
Member

Choose a reason for hiding this comment

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

Remove the assert_produces_warning, since it shouldn't anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it works but can someone please explain to me the reason behind it so that I learn

@hasnain2808
Copy link
Contributor Author

tests are not failing at local no clue why azure pipelines are failing

`
(pandas-dev) moha@moha-HP-Pavilion-Notebook:~/projects/pandas-hasnain2808$ pytest pandas/tests/series/test_analytics.py
============================= test session starts ==============================
platform linux -- Python 3.6.9, pytest-5.3.2, py-1.8.0, pluggy-0.13.1
rootdir: /home/moha/projects/pandas-hasnain2808, inifile: setup.cfg
plugins: cov-2.8.1, forked-1.1.3, xdist-1.30.0, hypothesis-4.54.0
collected 168 items

pandas/tests/series/test_analytics.py ..........................s....... [ 20%]
........................................................................ [ 63%]
..............................................x.....x.....x... [100%]

================== 164 passed, 1 skipped, 3 xfailed in 3.26s ===================
`

@hasnain2808 hasnain2808 reopened this Dec 24, 2019
@hasnain2808 hasnain2808 reopened this Dec 24, 2019
@jbrockmendel
Copy link
Member

What was happening in the failing builds is that when we call np.ptp(ser) numpy first checks if ser has a ptp method, in which case it calls that. This PR removes (removed) that method, so numpy is falling back to whatever it tries next, probably involving either __array__ or __array_func__ or something. Whatever that something is, it's returning a Series when it should return a scalar.

@hasnain2808
Copy link
Contributor Author

What was happening in the failing builds is that when we call np.ptp(ser) numpy first checks if ser has a ptp method, in which case it calls that. This PR removes (removed) that method, so numpy is falling back to whatever it tries next, probably involving either __array__ or __array_func__ or something. Whatever that something is, it's returning a Series when it should return a scalar.

So I actually had tried what is numpy returning on my local machine. Over there it's returning float
And also it would be nice if you could guide me a bit more towards the solution of this numpy issue

…e and values of ptp to understand why azure build failing
@hasnain2808
Copy link
Contributor Author

hasnain2808 commented Dec 25, 2019

What was happening in the failing builds is that when we call np.ptp(ser) numpy first checks if ser has a ptp method, in which case it calls that. This PR removes (removed) that method, so numpy is falling back to whatever it tries next, probably involving either __array__ or __array_func__ or something. Whatever that something is, it's returning a Series when it should return a scalar.

As I am unable to reproduce this on my local machine I tried to push a commit with the print statements which are ignored by the pipeline.
Also only builds on some platforms are failing not all
link to the azure build

mac os python 3.6
linux python 3.6 minimum version
linux python 3.6 32 bit
windows python 3.7 np 141

checking the logs show me that the series returned by numpy is all True boolean series

2019-12-25T01:57:55.8017700Z self = 0 True
2019-12-25T01:57:55.8017950Z 1 True
2019-12-25T01:57:55.8018210Z 2 True
2019-12-25T01:57:55.8018480Z 3 True
2019-12-25T01:57:55.8018720Z 4 True
2019-12-25T01:57:55.8018980Z ...
2019-12-25T01:57:55.8019230Z 995 True
2019-12-25T01:57:55.8019480Z 996 True
2019-12-25T01:57:55.8019740Z 997 True
2019-12-25T01:57:55.8019990Z 998 True
2019-12-25T01:57:55.8020250Z 999 True
2019-12-25T01:57:55.8020500Z Length: 1000, dtype: bool
2019-12-25T01:57:55.8020730Z

if it would have been returning a series with just the correct values it would have been simple to extract it, but the returned series is not correct.

@hasnain2808
Copy link
Contributor Author

At last all tests passing
Requesting some reviewer to check it

@hasnain2808 hasnain2808 mentioned this pull request Dec 25, 2019
@jreback jreback added this to the 1.0 milestone Dec 25, 2019
@jreback
Copy link
Contributor

jreback commented Dec 25, 2019

thanks for the effort @hasnain2808 I had already merged #30458

happy to take a contribution on other issues.

@jreback jreback closed this Dec 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants