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.

Sign up for GitHub

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

Jump to bottom

Remove deprecated Series.applymap. #11031

Merged
merged 9 commits into from
Aug 26, 2022
Merged

Remove deprecated Series.applymap. #11031

merged 9 commits into from
Aug 26, 2022

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jun 2, 2022 •
edited
Loading

This PR removes the deprecated Series.applymap function. This function does not exist in pandas. Users should switch to using Series.apply. (Note that DataFrame.applymap does exist in both pandas and cudf.) Deprecated in #10497.

@bdice bdice added Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function breaking Breaking change labels Jun 2, 2022
@bdice bdice self-assigned this Jun 2, 2022
@bdice bdice requested a review from a team as a code owner June 2, 2022 15:26
bdice
bdice commented Jun 2, 2022
View reviewed changes
python/cudf/cudf/tests/test_transform.py
@@ -22,14 +22,13 @@ def _generic_function(a):
(lambda x: x in [1, 2, 3, 4], lambda ser: np.isin(ser, [1, 2, 3, 4])),
],
)
def test_applymap_python_lambda(dtype, udf, testfunc):
def test_apply_python_lambda(dtype, udf, testfunc):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test file is really short and might be a candidate for removal / integration into another test file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be in support of removing this. I'm a little surprised the test for np.isin works since this is a numpy API I wouldn't expect to be supported by the cuda target. Will sniff around here too.

@brandon-b-miller
Copy link
Contributor

Is there a case to just remove the applymap tests completely rather than switching them over to test apply instead? I'm fairly sure apply is a superset of features relative to applymap.

@bdice
Copy link
Contributor Author

bdice commented Jun 2, 2022

Is there a case to just remove the applymap tests completely rather than switching them over to test apply instead? I'm fairly sure apply is a superset of features relative to applymap.

I think that’s a great question but I am not sure of the answer. I have not looked closely at the tests or test coverage yet but that’s part of why I tagged you for review. If you have ideas for specific pieces to remove, please weigh in!

@brandon-b-miller
Copy link
Contributor

Made a few comments and noticed a few touchups that need to be put in around apply:

  • **kwargs in Series.apply shouldn't work
  • We might be missing some operators on MaskedType that could be probably easily added
  • Missing tests for operators that applymap did not support but that apply should such as trig functions (this is related to the previous item)

Let's make sure those are addressed and then rotate back to this.

@github-actions
Copy link

github-actions bot commented Jul 3, 2022

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

@bdice bdice changed the base branch from branch-22.08 to branch-22.10 July 28, 2022 03:25
@bdice
Copy link
Contributor Author

bdice commented Aug 16, 2022

Tasks for later, discussed with @brandon-b-miller:

  • Add notes on global variables, either to apply docstring or UDFs guide notebook:

    cudf/python/cudf/cudf/core/series.py

    Line 2384 in 0c4b319

    * Global variables need to be re-defined explicitly inside
  • Investigate coverage of test_transform.py.

@bdice bdice requested a review from brandon-b-miller August 16, 2022 19:00
@bdice
Copy link
Contributor Author

bdice commented Aug 16, 2022

rerun tests

@brandon-b-miller
Copy link
Contributor

Looks like there's a test failure because we're missing an implementation of the float operator on our MaskedType extension type. This should be a fairly easy fix, I can put in a PR.

@brandon-b-miller brandon-b-miller mentioned this pull request Aug 22, 2022
Merged
rapids-bot bot pushed a commit that referenced this pull request Aug 25, 2022
@brandon-b-miller
Add casting operators to masked UDFs (#11578)
096bbc4 
While working through #11031 it was discovered that we were missing the ability to "cast" between python "classes" (`int`, `float`, and `bool`) within UDFs. This PR introduces the equivalent syntax into masked UDFs. These operations shall be interpreted as mapping to `int64`, `float64` and `bool` types, following numpy and numba's existing handling for scalar types.

Authors:
  - https://github.com/brandon-b-miller

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Matthew Roeschke (https://github.com/mroeschke)

URL: #11578
@codecov
Copy link

codecov bot commented Aug 25, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@096bbc4). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.10   #11031   +/-   ##
===============================================
  Coverage                ?   86.41%           
===============================================
  Files                   ?      145           
  Lines                   ?    22972           
  Branches                ?        0           
===============================================
  Hits                    ?    19852           
  Misses                  ?     3120           
  Partials                ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bdice
Copy link
Contributor Author

bdice commented Aug 26, 2022

@brandon-b-miller @galipremsagar @isVoid This should be ready for review, CI is passing now.

@galipremsagar galipremsagar added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Aug 26, 2022
@galipremsagar
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 48dc168 into rapidsai:branch-22.10 Aug 26, 2022
@brandon-b-miller brandon-b-miller mentioned this pull request Aug 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandon-b-miller brandon-b-miller brandon-b-miller approved these changes

@isVoid isVoid isVoid approved these changes

@galipremsagar galipremsagar galipremsagar approved these changes

Assignees

@bdice bdice

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bdice @brandon-b-miller @galipremsagar @isVoid