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

[REVIEW] Add information about Index.is_* method deprecation #12909

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Mar 8, 2023

Description

This PR adds additional information for the following Index APIs to match with pandas 2.0:

  • is_numeric
  • is_boolean
  • is_integer
  • is_floating
  • is_object
  • is_categorical
  • is_interval

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer improvement Improvement / enhancement to an existing function breaking Breaking change labels Mar 8, 2023
@galipremsagar galipremsagar requested a review from a team as a code owner March 8, 2023 21:26
@galipremsagar galipremsagar self-assigned this Mar 8, 2023
@galipremsagar galipremsagar requested review from shwina and brandon-b-miller and removed request for a team March 8, 2023 21:26
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Still need to clean out the implementations in index.py, right?

@galipremsagar
Copy link
Contributor Author

Still need to clean out the implementations in index.py, right?

The implementations were converted as internal APIs to be used as fast paths for is_numeric_dtype, is_bool_dtype, etc: here: https://github.com/rapidsai/cudf/pull/12820/files#diff-ef94b3e28724ebfe959cd02a9f37427cefcdc118799490453fbe1c6299dc0324R63

@vyasr
Copy link
Contributor

vyasr commented Mar 10, 2023

Ah OK I see. Now that these APIs are not public I think it would be better to remove those helper functions and implement the api.types functions in terms of a simple hash lookup (i.e. store some module-level dicts in api/types.py mapping {cudf.$INDEX_CLASS: bool}). If these APIs are no longer public I don't think the organization of having helper functions like this is very helpful, it just makes the code less readable and harder to maintain (and incurs unnecessary performance costs too with Python function calls). If you'd like to refactor that in a separate PR I'd be OK with that though.

@galipremsagar
Copy link
Contributor Author

Ah OK I see. Now that these APIs are not public I think it would be better to remove those helper functions and implement the api.types functions in terms of a simple hash lookup (i.e. store some module-level dicts in api/types.py mapping {cudf.$INDEX_CLASS: bool}). If these APIs are no longer public I don't think the organization of having helper functions like this is very helpful, it just makes the code less readable and harder to maintain (and incurs unnecessary performance costs too with Python function calls). If you'd like to refactor that in a separate PR I'd be OK with that though.

Got it, it makes sense to have this kind of a refactor done in this PR itself. I'll make those changes here. 👍

@mroeschke
Copy link
Contributor

FWIW these were only deprecated in pandas 2.0 not removed. These will be removed in pandas 3.0

@galipremsagar
Copy link
Contributor Author

FWIW these were only deprecated in pandas 2.0 not removed. These will be removed in pandas 3.0

Ah, my bad. So let's probably keep them deprecated and not drop them now. Will add comments to this PR about the delayed removal of these APIs.

@galipremsagar galipremsagar changed the title [REVIEW] Drop Index.is_* methods [REVIEW] Add information about Index.is_* method deprecation Mar 10, 2023
@galipremsagar
Copy link
Contributor Author

This should be now ready for review.

@galipremsagar galipremsagar requested a review from vyasr March 10, 2023 23:05
@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Mar 13, 2023
@galipremsagar galipremsagar merged commit d1377a5 into rapidsai:pandas_2.0_feature_branch Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants