-
Notifications
You must be signed in to change notification settings - Fork 915
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
Deprecate merge_sorted
, change dask cudf usage to internal method
#10713
Deprecate merge_sorted
, change dask cudf usage to internal method
#10713
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #10713 +/- ##
================================================
+ Coverage 86.36% 86.43% +0.07%
================================================
Files 142 143 +1
Lines 22302 22448 +146
================================================
+ Hits 19261 19403 +142
- Misses 3041 3045 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @isVoid ! Moving merge_sorted
to a private funciton makes sense to me.
Can we update the merge_sorted
tests in test_reshape.py
to point to the correct _merge_sorted
function now (and maybe wrap the deprecated usage in a with pytest.warns
block in one place)?
Yes, let's update the tests here if possible. That will make @bdice's job of removing warnings from tests easier. I'll admit that given the large number of warnings our tests currently throw I have been lax about making these changes in my own deprecation PRs, but in those cases the tests were getting entirely removed in the next release so there was less value in putting in the effort. In this case we expect to continue maintaining (and therefore testing) |
rerun tests |
@rjzamora let us know if this looks good to you now. |
Ack sorry pushed the wrong button sorry. Did not mean to close. |
I also updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @isVoid !
@bdice will thank you. |
@gpucibot merge |
This removes the public API of `cudf.merge_sorted`. This has been replaced by an internal API and does not need to be exposed in the cudf public API. Deprecated in #10713. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) URL: #11032
This PR deprecates non-pandas conform method
cudf.merge_sorted
and change dask cudf usage to internal method_merge_sorted
.I also updated msg keyword in pytest.skip in multiple tests to reason, this removes 1000+ test warnings.
cc @vyasr @rjzamora