Skip to content

BUG: Fix series clipping NA issue #41141

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
Merged
merged 4 commits into from
Apr 30, 2021
Merged

BUG: Fix series clipping NA issue #41141

merged 4 commits into from
Apr 30, 2021

Conversation

Rasori
Copy link
Contributor

@Rasori Rasori commented Apr 24, 2021 •
edited
Loading

Series clipping method could not handle series with NA values.
Issue was fixed by first comparing values and only after then converted
to numpy array. Now the test code provided with the issue ticket provides an expected result.

@Rasori Rasori changed the title Fix series clipping NA issue BUG: Fix series clipping NA issue Apr 24, 2021
phofl
phofl requested changes Apr 24, 2021
View reviewed changes
Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

Please add tests

@pep8speaks
Copy link

pep8speaks commented Apr 25, 2021 •
edited
Loading

Hello @Rasori! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-28 18:17:50 UTC

@Rasori Rasori requested a review from phofl April 25, 2021 10:44
jreback
jreback requested changes Apr 26, 2021
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

pls add a whatsnew note, bug fixes numeric section

pandas/tests/generic/test_series.py Outdated
@@ -134,3 +134,17 @@ def finalize(self, other, method=None, **kwargs):
# reset
Series._metadata = _metadata
Series.__finalize__ = _finalize # FIXME: use monkeypatch

def test_series_clipping_with_na_values(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

move to pandas/tests/series/methods/test_clip.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pandas/tests/generic/test_series.py Outdated
@@ -134,3 +134,17 @@ def finalize(self, other, method=None, **kwargs):
# reset
Series._metadata = _metadata
Series.__finalize__ = _finalize # FIXME: use monkeypatch

def test_series_clipping_with_na_values(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you parameterize by nullable int & float types (I think we have a fixture for this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I parameterized the test but I could not find any clear listing of all nullable int and float types.

@jreback jreback added Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Bug ExtensionArray Extending pandas with custom dtypes or arrays. NA - MaskedArrays Related to pd.NA and nullable extension arrays and removed ExtensionArray Extending pandas with custom dtypes or arrays. labels Apr 26, 2021
@Rasori Rasori requested a review from jreback April 27, 2021 18:54
jreback
jreback reviewed Apr 27, 2021
View reviewed changes
pandas/tests/series/methods/test_clip.py Outdated
@@ -40,6 +40,21 @@ def test_clip_types_and_nulls(self):
assert list(isna(s)) == list(isna(lower))
assert list(isna(s)) == list(isna(upper))

@pytest.mark.parametrize("dtypes", ["Float64", "Int64", "Float32", "Int32"])
Copy link
Contributor

Choose a reason for hiding this comment

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

we have fixtures for all of these look in pandas/conftest.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not familiar with testing but I think I got it this time. I also expanded the test to cover all nulls as there was a fixture for it already. Thanks for your tips!

Rasori added 3 commits April 28, 2021 18:29
@Rasori
Fix series clipping NA issue
89bfe1d 
Series clipping method could not handle series with NA values.
Issue was fixed by first comparing values and only after then converted
to numpy array.
@Rasori
Add test to ensure series clipping method can handle NA values
b621d79
@Rasori
Add whatsnew entry
f49cfd0
@Rasori Rasori requested a review from jreback April 28, 2021 16:59
@Rasori
Copy link
Contributor Author

Rasori commented Apr 28, 2021 •
edited
Loading

The test fail is not related to my changes.

E: I re-pushed the changes to see if the check passes this time. At least checks of other PRs are passing ...

phofl
phofl approved these changes Apr 28, 2021
View reviewed changes
Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

lgtm

@jreback jreback added this to the 1.3 milestone Apr 30, 2021
@jreback
Copy link
Contributor

jreback commented Apr 30, 2021

thanks @Rasori

@jreback jreback merged commit 29512dc into pandas-dev:master Apr 30, 2021
yeshsurya pushed a commit to yeshsurya/pandas that referenced this pull request May 6, 2021
@Rasori @yeshsurya
BUG: Fix series clipping NA issue (pandas-dev#41141)
ccaa262
@TomAugspurger
Copy link
Contributor

This seemed to cause a performance regression. https://pandas.pydata.org/speed/pandas/index.html#series_methods.Clip.time_clip?python=3.8&Cython=0.29.21&p-n=50&commits=29512dc9ea2b066ab5f4a7cb80c1f8f5becef1c2

@jreback
Copy link
Contributor

jreback commented May 12, 2021

@TomAugspurger if u wouldn't mind creating a new issue about this perf regression

@TomAugspurger TomAugspurger mentioned this pull request May 12, 2021
Closed
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
@Rasori @JulianWgs
BUG: Fix series clipping NA issue (pandas-dev#41141)
98015b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

@phofl phofl phofl approved these changes

Assignees
No one assigned
Labels
Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Bug NA - MaskedArrays Related to pd.NA and nullable extension arrays
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

BUG: Pandas 1.2 the clip method does not support Float64Dtype with pd.NA
5 participants
@Rasori @pep8speaks @jreback @TomAugspurger @phofl