Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

DEPR: deprecate Index.is_numeric #50769

Merged
merged 35 commits into from
Feb 2, 2023
Merged

DEPR: deprecate Index.is_numeric #50769

merged 35 commits into from
Feb 2, 2023

Conversation

ABCPAN-rank
Copy link
Contributor

@ABCPAN-rank ABCPAN-rank changed the title warm user is_numeric will be deprecated warn user is_numeric will be deprecated Jan 16, 2023
@ABCPAN-rank ABCPAN-rank changed the title warn user is_numeric will be deprecated DEPR: deprecate Index.is_numeric Jan 16, 2023
@ABCPAN-rank
Copy link
Contributor Author

@topper-123 hellow friend , i have changed all is_numeric . But it still have a check failing . I want to know where lead to this failing . Can you help me to pass this check ?

@topper-123
Copy link
Contributor

Hi @ABCPAN-rank. Thanks for this. You're getting close. A couple issues:

  1. You need to add a test add a test that this is deprecated, named test_is_numeric_is_deprecated or similar. Look at the other PRs how this is done.
  2. The method is still used in some places, check the Doc Build and Upload / Doc Build and Upload (pull_request) failures. Usage of this method should removed entirely from the code base, except from the test that it is deprecated.
  3. You use is_numeric_dtype(x) and not is_complex_dtype(x) and not is_bool_dtype(x) consistently. Can you check if this is necessary or if this check should be relaxed in some places.

Can you look at the above, especially point 3. If those three checks in point 3 are really needed as much as indicated here, can you add a helper function in pandas.core.dtypes.common for this, so we only have to use a single function instead of 3.

@topper-123 topper-123 added Deprecate Functionality to remove in pandas Index Related to the Index class or subclasses labels Jan 17, 2023
@topper-123 topper-123 added this to the 2.0 milestone Jan 17, 2023
@ABCPAN-rank
Copy link
Contributor Author

@topper-123 :

  1. I think test_is_numeric_deprecatedwhich is same as other PR had been added in pandas/tests/indexes/common.py.
  2. I think all .is_numeric() was changed(except test) after i use git grep "is_numeric()".But check is failing. at Doc Build and Upload It have 5 warnings in Doc Build and Upload . I cannot find the warning path
    3..is_numeric() (Only real number) is different from is_numeric_dtype()(including boolean and complex) in number field. if we use is_numeric_dtype() instead of .is_numeric(), we need a helper function . so what's name should be called about this helper function ? is_real_dtypes ?

@topper-123
Copy link
Contributor

  1. Yes I had missed that, sorry
  2. I looked at it, and I think it is is an external package that used the method (dask), so probably is ok as-is. I've restarting the doc build test, maybe the failure was a fluke and it'll pass on a second try.
  3. Yes, but could you check if we really should check for bools and complex numbers in all those locations. it may have been done without considering the difference and now is a good time to check if using is_numeric_dtype only is sufficient. If they're needed, is_real_dtypes is probably not a good name (complex numbers are not real as far as I remember my math), but e.g. is_any_numeric_dtype or similar is fine.

@ABCPAN-rank
Copy link
Contributor Author

ABCPAN-rank commented Jan 18, 2023

@topper-123 ok, i will check it .
But is_numeric() will return False when index include complex or boolean. I think if we want to use a helper function instead of is_numeric() , we can use is_real_dtype() . Because is_numeric() is check real number in actually .It will return True when input is only int or float.

@ABCPAN-rank
Copy link
Contributor Author

@topper-123 If I use a helper function to instead of is_numeric(), should I write some tests for helper function? Where can write the tests? Or I can use old test about is_numeric() .

@topper-123
Copy link
Contributor

No, That's not needed, this is just internal and very simple. is_real_dtype is not good for a name, if it's checking other types than real numbers.

@ABCPAN-rank
Copy link
Contributor Author

@topper-123 Ok i will write a helpful function called is_any_numeric_dtype .
Should I change doc/source/whatsnew/v2.0.0.rst for is_any_numeric_dtype?

@topper-123
Copy link
Contributor

Should I change doc/source/whatsnew/v2.0.0.rst for is_any_numeric_dtype?

No, this should be a purely internal detail, so a whatsnew entry is not needed.

Copy link
Contributor

@topper-123 topper-123 left a comment

Choose a reason for hiding this comment

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

In general, it looks good, just needs to pass the remaining tests.

For the errors in Doc Build and Upload / Doc Build and Upload I'm not 100 % if it is the problem, but try to add a :okwarning: below the relevant .. ipython:: python sections. You can see where the doc tests complain about dask using is_numeric in the test logs.

Copy link
Contributor

@topper-123 topper-123 left a comment

Choose a reason for hiding this comment

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

LGTM.

@topper-123 topper-123 merged commit ac5bc67 into pandas-dev:main Feb 2, 2023
@topper-123
Copy link
Contributor

Thanks @ABCPAN-rank. Very nice.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Deprecate Functionality to remove in pandas Index Related to the Index class or subclasses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants