GH984 Add overload for DataFrame.clip and update those for Series.clip #1206
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dr-Irv
requested changes
Apr 29, 2025
|
Turns out no changes were needed but agreed that this time we check all the cases. |
|
I'm a little confused here, as you closed the PR and then reopened it. Should I review? |
|
Yes it is open for review, I have accidentally pressed the close button, my
mistake.
…On Wed, Apr 30, 2025 at 9:46 AM Irv Lustig ***@***.***> wrote:
*Dr-Irv* left a comment (pandas-dev/pandas-stubs#1206)
<#1206 (comment)>
I'm a little confused here, as you closed the PR and then reopened it.
Should I review?
—
Reply to this email directly, view it on GitHub
<#1206 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMQNSQBKOZWMMGWLDPGFMV324DH4LAVCNFSM6AAAAAB4BXDTWOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNBSGA2DOOJRHA>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Dr-Irv
requested changes
Apr 30, 2025
tests/test_frame.py
Outdated
Comment on lines
808
to
810
| check(assert_type(df.clip(upper=None, axis="index"), pd.DataFrame), pd.DataFrame) | ||
| check(assert_type(df.clip(upper=15, axis="index"), pd.DataFrame), pd.DataFrame) | ||
| check(assert_type(df.clip(upper=None, axis="index"), pd.DataFrame), pd.DataFrame) |
There was a problem hiding this comment.
lines 808 and 810 are the same
tests/test_frame.py
Outdated
Comment on lines
863
to
881
| assert_type(df.clip(lower=None, axis="index"), pd.DataFrame), | ||
| pd.DataFrame, | ||
| ) | ||
| check( | ||
| assert_type(df.clip(lower=5, axis="index"), pd.DataFrame), | ||
| pd.DataFrame, | ||
| ) | ||
| check( | ||
| assert_type(df.clip(lower=None, axis="index"), pd.DataFrame), | ||
| pd.DataFrame, | ||
| ) | ||
| check( | ||
| assert_type(df.clip(lower=pd.Series([1, 2]), axis="index"), pd.DataFrame), | ||
| pd.DataFrame, | ||
| ) | ||
| check( | ||
| assert_type(df.clip(lower=None, axis="index"), pd.DataFrame), | ||
| pd.DataFrame, | ||
| ) |
There was a problem hiding this comment.
lines 863 and 871 and 879 are the same test
tests/test_frame.py
Outdated
Comment on lines
847
to
859
| assert_type(df.clip(lower=None, axis="index", inplace=True), pd.DataFrame), | ||
| pd.DataFrame, | ||
| ) | ||
| check( | ||
| assert_type(df.clip(lower=5, axis="index", inplace=True), None), | ||
| type(None), | ||
| ) | ||
| check( | ||
| assert_type(df.clip(lower=pd.Series([1, 2]), axis="index", inplace=True), None), | ||
| type(None), | ||
| ) | ||
| check( | ||
| assert_type(df.clip(lower=None, axis="index", inplace=True), pd.DataFrame), |
There was a problem hiding this comment.
lines 847 and 859 are the same test
tests/test_frame.py
Outdated
Comment on lines
883
to
888
| assert_type(df.clip(lower=None, axis=0, inplace=True), pd.DataFrame), | ||
| pd.DataFrame, | ||
| ) | ||
| check(assert_type(df.clip(lower=5, axis=0, inplace=True), None), type(None)) | ||
| check( | ||
| assert_type(df.clip(lower=None, axis=0, inplace=True), pd.DataFrame), |
There was a problem hiding this comment.
lines 883 and 888 are the same test
Comment on lines
+828
to
+837
| assert_type(df.clip(lower=None, axis=None, inplace=True), pd.DataFrame), | ||
| pd.DataFrame, | ||
| ) | ||
| check( | ||
| assert_type(df.clip(lower=5, axis=None, inplace=True), None), | ||
| type(None), | ||
| ) | ||
| check( | ||
| assert_type(df.clip(lower=None, axis=None, inplace=True), pd.DataFrame), | ||
| pd.DataFrame, |
There was a problem hiding this comment.
lines 828 and 836 are the same test
|
Since clip for series does not use the axis when passing a list like
argument to lower or upper the overloads are already there. Let me clean up
the tests and add for those without upper or lower passed and we should be
in a good place, the big work was on the data frame side.
…On Wed, Apr 30, 2025 at 11:50 AM Irv Lustig ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I think you may also need similar overloads for clip in series.pyi and
then similar tests that omit the lower or upper arguments.
------------------------------
In tests/test_frame.py
<#1206 (comment)>
:
> + check(assert_type(df.clip(upper=None, axis="index"), pd.DataFrame), pd.DataFrame)
+ check(assert_type(df.clip(upper=15, axis="index"), pd.DataFrame), pd.DataFrame)
+ check(assert_type(df.clip(upper=None, axis="index"), pd.DataFrame), pd.DataFrame)
lines 808 and 810 are the same
------------------------------
In tests/test_frame.py
<#1206 (comment)>
:
> + assert_type(df.clip(lower=None, axis="index"), pd.DataFrame),
+ pd.DataFrame,
+ )
+ check(
+ assert_type(df.clip(lower=5, axis="index"), pd.DataFrame),
+ pd.DataFrame,
+ )
+ check(
+ assert_type(df.clip(lower=None, axis="index"), pd.DataFrame),
+ pd.DataFrame,
+ )
+ check(
+ assert_type(df.clip(lower=pd.Series([1, 2]), axis="index"), pd.DataFrame),
+ pd.DataFrame,
+ )
+ check(
+ assert_type(df.clip(lower=None, axis="index"), pd.DataFrame),
+ pd.DataFrame,
+ )
lines 863 and 871 and 879 are the same test
------------------------------
In tests/test_frame.py
<#1206 (comment)>
:
> + assert_type(df.clip(lower=None, axis="index", inplace=True), pd.DataFrame),
+ pd.DataFrame,
+ )
+ check(
+ assert_type(df.clip(lower=5, axis="index", inplace=True), None),
+ type(None),
+ )
+ check(
+ assert_type(df.clip(lower=pd.Series([1, 2]), axis="index", inplace=True), None),
+ type(None),
+ )
+ check(
+ assert_type(df.clip(lower=None, axis="index", inplace=True), pd.DataFrame),
lines 847 and 859 are the same test
------------------------------
In tests/test_frame.py
<#1206 (comment)>
:
> + assert_type(df.clip(lower=None, axis=0, inplace=True), pd.DataFrame),
+ pd.DataFrame,
+ )
+ check(assert_type(df.clip(lower=5, axis=0, inplace=True), None), type(None))
+ check(
+ assert_type(df.clip(lower=None, axis=0, inplace=True), pd.DataFrame),
lines 883 and 888 are the same test
------------------------------
In tests/test_frame.py
<#1206 (comment)>
:
> + assert_type(df.clip(lower=None, axis=None, inplace=True), pd.DataFrame),
+ pd.DataFrame,
+ )
+ check(
+ assert_type(df.clip(lower=5, axis=None, inplace=True), None),
+ type(None),
+ )
+ check(
+ assert_type(df.clip(lower=None, axis=None, inplace=True), pd.DataFrame),
+ pd.DataFrame,
lines 828 and 836 are the same test
—
Reply to this email directly, view it on GitHub
<#1206 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMQNSQB5TEGEXPERXRNI5T324DWOBAVCNFSM6AAAAAB4BXDTWOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMBXGUYTGNBUGM>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
|
Turns out the axis parameter does not need anything specific (to split the overloads) since it is not used (unless you pass something not allowed). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DataFrame.clip()#984assert_type()to assert the type of any return valueTurns out there is a nasty edge case when you pass everything as None for upper and lower and ask for
inplace=True, it will returnSelfand notNone.