BUG: clip doesn't preserve dtype by column #24458

minggli · 2018-12-28T00:09:06Z

closes DataFrame.clip_upper does not preserve dtype per column #24162
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

>>> import pandas as pd
>>> data = pd.DataFrame([[1, 2], [3, 4]], columns=['int1', 'int2'])
>>> data.clip_upper(1)
__main__:1: FutureWarning: clip_upper(threshold) is deprecated, use clip(upper=threshold) instead
   int1  int2
0     1     1
1     1     1

Before

>>> data['float'] = data['int1'].astype(float)
>>> data.clip_upper(1)
   int1  int2  float
0   1.0   1.0    1.0
1   1.0   1.0    1.0

After

>>> data['float'] = data['int1'].astype(float)
>>> data.clip_upper(1)
   int1  int2  float
0     1     1    1.0
1     1     1    1.0

jreback · 2018-12-28T00:23:32Z

looks pretty good - pls add. whatsnew note
can u ensure that the OP test is covered?

i suspect we have a couple of open issues for clip that might be closed by this as well

if u can have a look

jreback · 2018-12-28T00:31:32Z

#20911 and #21476 maybe

minggli · 2018-12-28T00:37:42Z

#20911 and #21476 maybe
Thanks @jreback sure let me check them out!

mroeschke · 2018-12-28T00:49:49Z

pandas/core/generic.py

+            subset = self.le(upper, axis=None) | isna(result)
+            result = result.where(subset, upper, axis=None, inplace=inplace)
+        if lower is not None:
+            if inplace:


Why is this inplace check only when lower is not None? It looks like you also assign result = self before the upper and lower check.

Hi @mroeschke, thanks for reviewing!

result = self is to ensure there is a return value in case both lower and upper are None, which they can. when inplace=True, result is None

Got it. Thanks!

I think its just better to use the existing idiom

if inplace: self._update_inplace(result0 else: return self

actioned. thanks.

codecov · 2018-12-28T01:16:37Z

Codecov Report

Merging #24458 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #24458      +/-   ##
==========================================
- Coverage   92.29%   92.29%   -0.01%     
==========================================
  Files         163      163              
  Lines       51969    51964       -5     
==========================================
- Hits        47967    47962       -5     
  Misses       4002     4002

Flag	Coverage Δ
#multiple	`90.7% <100%> (-0.01%)`	⬇️
#single	`43.02% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/generic.py	`96.62% <100%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24a50fc...7235940. Read the comment docs.

codecov · 2018-12-28T01:16:37Z

Codecov Report

Merging #24458 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #24458      +/-   ##
==========================================
- Coverage   92.29%   92.29%   -0.01%     
==========================================
  Files         163      163              
  Lines       51969    51943      -26     
==========================================
- Hits        47967    47940      -27     
- Misses       4002     4003       +1

Flag	Coverage Δ
#multiple	`90.7% <100%> (-0.01%)`	⬇️
#single	`43% <0%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/generic.py	`96.62% <100%> (-0.01%)`	⬇️
pandas/core/indexes/datetimelike.py	`97.56% <0%> (-0.15%)`	⬇️
pandas/core/indexes/datetimes.py	`96.2% <0%> (-0.11%)`	⬇️
pandas/core/indexes/period.py	`92.71% <0%> (-0.05%)`	⬇️
pandas/util/testing.py	`87.75% <0%> (ø)`	⬆️
pandas/core/indexes/accessors.py	`91% <0%> (ø)`	⬆️
pandas/core/indexes/timedeltas.py	`90.28% <0%> (+0.16%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24a50fc...fab5c99. Read the comment docs.

minggli · 2018-12-28T03:37:18Z

@jreback,

had look at #20911, I think that's not a supported use case when passing a DataFrame for either lower or upper, which is essentially used as a numpy array for creating a mask, having ignored the column orders.

#21476 is to do with different precisions between s and threshold. It works fine downcasting from int to float but appears not from float32 to float64. I suspect it's to do with numpy and need more investigation.

minggli · 2018-12-28T03:39:59Z

>>> df.clip(lower=2, upper=7)
   floats  int
0     2.0    2
1     2.0    2
2     2.2    2
3     3.3    3
4     4.4    4
5     5.5    5
6     6.6    6
7     7.0    7
8     7.0    7
9     7.0    7

>>> df.clip(lower=2, upper=7.5)
   floats  int
0     2.0  2.0
1     2.0  2.0
2     2.2  2.0
3     3.3  3.0
4     4.4  4.0
5     5.5  5.0
6     6.6  6.0
7     7.5  7.0
8     7.5  7.5
9     7.5  7.5

jreback · 2018-12-28T13:34:03Z

pandas/core/generic.py

+            subset = self.le(upper, axis=None) | isna(result)
+            result = result.where(subset, upper, axis=None, inplace=inplace)
+        if lower is not None:
+            if inplace:


I think its just better to use the existing idiom

if inplace: self._update_inplace(result0 else: return self

minggli · 2018-12-28T17:56:26Z

@jreback

review actioned and added test and documentation.

jreback · 2018-12-28T17:58:45Z

thanks. ping on green.

minggli · 2018-12-28T18:52:08Z

@jreback thanks, it's green.

jreback · 2018-12-28T19:03:33Z

thanks @minggli

minggli · 2018-12-28T21:49:58Z

@jreback @mroeschke thanks for reviewing and happy new year!

minggli added 3 commits December 27, 2018 16:16

use self.where method instead of numpy where to reuse casting logics

31f8567

modify clip scalar

61bb0c8

modify expected case

7235940

jreback added Bug Dtype Conversions Unexpected or buggy dtype conversions labels Dec 28, 2018

jreback added this to the 0.24.0 milestone Dec 28, 2018

mroeschke reviewed Dec 28, 2018

View reviewed changes

jreback requested changes Dec 28, 2018

View reviewed changes

minggli added 3 commits December 28, 2018 12:20

use existing handling of inplace

b5a6121

add test case

45bd478

update whatsnew

fab5c99

jreback approved these changes Dec 28, 2018

View reviewed changes

jreback merged commit d1b2a52 into pandas-dev:master Dec 28, 2018

minggli deleted the bugfix/clip_type branch December 28, 2018 21:50

qwhelan mentioned this pull request Jan 11, 2019

PERF: fix some of .clip() performance regression by using numpy arrays where possible #24735

Merged

4 tasks

jorisvandenbossche mentioned this pull request Feb 1, 2019

regression in 0.24: np.clip does not work for some values of a_min #25066

Open

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: clip doesn't preserve dtype by column (pandas-dev#24458)

673fc30

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: clip doesn't preserve dtype by column (pandas-dev#24458)

6a78014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: clip doesn't preserve dtype by column #24458

BUG: clip doesn't preserve dtype by column #24458

minggli commented Dec 28, 2018 •

edited

Loading

jreback commented Dec 28, 2018

jreback commented Dec 28, 2018

minggli commented Dec 28, 2018

mroeschke Dec 28, 2018

minggli Dec 28, 2018

mroeschke Dec 28, 2018

jreback Dec 28, 2018

minggli Dec 28, 2018

codecov bot commented Dec 28, 2018

codecov bot commented Dec 28, 2018 •

edited

Loading

minggli commented Dec 28, 2018

minggli commented Dec 28, 2018

jreback Dec 28, 2018

minggli commented Dec 28, 2018

jreback commented Dec 28, 2018

minggli commented Dec 28, 2018

jreback commented Dec 28, 2018

minggli commented Dec 28, 2018

BUG: clip doesn't preserve dtype by column #24458

BUG: clip doesn't preserve dtype by column #24458

Conversation

minggli commented Dec 28, 2018 • edited Loading

jreback commented Dec 28, 2018

jreback commented Dec 28, 2018

minggli commented Dec 28, 2018

mroeschke Dec 28, 2018

Choose a reason for hiding this comment

minggli Dec 28, 2018

Choose a reason for hiding this comment

mroeschke Dec 28, 2018

Choose a reason for hiding this comment

jreback Dec 28, 2018

Choose a reason for hiding this comment

minggli Dec 28, 2018

Choose a reason for hiding this comment

codecov bot commented Dec 28, 2018

Codecov Report

codecov bot commented Dec 28, 2018 • edited Loading

Codecov Report

minggli commented Dec 28, 2018

minggli commented Dec 28, 2018

jreback Dec 28, 2018

Choose a reason for hiding this comment

minggli commented Dec 28, 2018

jreback commented Dec 28, 2018

minggli commented Dec 28, 2018

jreback commented Dec 28, 2018

minggli commented Dec 28, 2018

minggli commented Dec 28, 2018 •

edited

Loading

codecov bot commented Dec 28, 2018 •

edited

Loading