Fix for #12723: Unexpected behavior with binary operators and fill… #12791

facaiy · 2016-04-04T05:28:47Z

closes #xxxx
tests added / passed
passes git diff upstream/master | flake8 --diff
whatsnew entry

jreback · 2016-04-04T17:21:32Z

pandas/core/frame.py

        if self.empty:
            return self

-        new_data = self._data.eval(func=func, other=other,
-                                   raise_on_error=raise_on_error)
+        if fill_value is not None:


you need to do the eval first, THEN, fill.

Do you mean that df2.add(2, fill_value=0) should act like df2.add(2).fillna(0), instead of df2.fillna(0).add(2)?

the steps are align, arith then fill (with mask).

align is moot in this case. yes you are right, fill then arith.

In [7]: Series([np.nan,1,np.nan]).add(Series([2,3],index=[2,3]),fill_value=10) Out[7]: 0 NaN 1 11.0 2 12.0 3 13.0 dtype: float64

I have modified the API, DataFrame._combine_const, is it OK?
if yes, for consistency, it seems that we need to modify _combine_const method of sparse and series and their caller .

jreback · 2016-04-06T16:27:27Z

pandas/core/frame.py

-        new_data = self._data.eval(func=func, other=other,
-                                   raise_on_error=raise_on_error)
+        if fill_value is not None:
+            self_copy = self.copy()


you can just do

if fill_value is not None: self = self.fillna(fill_value) data = self._data.eval(......) ....

we don't directly access .values like this as you can have multiple dtypes.

Thanks, @jreback.

Since the _combine_const of all classes are designed without fill_value parameter, I worry whether modifying the API is a good idea. At least, Series and some member of sparse need do something to keep consistent. Could you give me some advice?

facaiy · 2016-04-08T07:17:33Z

pandas/core/ops.py

@@ -1086,7 +1089,7 @@ def f(self, other, axis=default_axis, level=None, fill_value=None):
                raise ValueError("Incompatible argument shape: %s" %
                                 (other.shape, ))
        else:
-            return self._combine_const(other, na_op)
+            return self._combine_const(other, na_op, fill_value=fill_value)


How about use fillna here, instead of modifying the _combine_const method? @jreback

facaiy · 2016-04-08T12:31:03Z

If it is OK, I'd like to add a whatsnew and rebase.

jreback · 2016-04-08T13:03:50Z

pandas/tests/frame/test_missing.py

+
+    def test_fill_value_missing_with_binary_operators(self):
+        # GH12723
+        dat = np.arange(10)


specify this as np.arange(10, dtype='int64') or the comparison will fail on windows

jreback · 2016-04-08T13:04:50Z

pls test for the sparse cases as well

facaiy · 2016-04-16T06:54:42Z

pandas/sparse/tests/test_sparse.py

+
+        exp = df2.fillna(0).add(df1)
+        res = df2.add(df1, fill_value=0)
+        tm.assert_sp_frame_equal(res, exp)


I'm not familiar with sparse dataframe. And the sparse item and NaN are quite similar and confuse for me.

Traceback (most recent call last): File "/Users/facaiyan/Workshop/pandas/pandas/sparse/tests/test_sparse.py", line 966, in test_fill_value_missing_with_binary_operators tm.assert_sp_frame_equal(res, exp) File "/Users/facaiyan/Workshop/pandas/pandas/util/testing.py", line 1257, in assert_sp_frame_equal assert_sp_series_equal(series, right[col]) File "/Users/facaiyan/Workshop/pandas/pandas/util/testing.py", line 1229, in assert_sp_series_equal assert_sp_array_equal(left.block.values, right.block.values) File "/Users/facaiyan/Workshop/pandas/pandas/util/testing.py", line 1205, in assert_sp_array_equal assert_almost_equal(left.sp_values, right.sp_values) File "/Users/facaiyan/Workshop/pandas/pandas/util/testing.py", line 124, in assert_almost_equal return _testing.assert_almost_equal(left, right, **kwargs) File "pandas/src/testing.pyx", line 58, in pandas._testing.assert_almost_equal (pandas/src/testing.c:3809) File "pandas/src/testing.pyx", line 120, in pandas._testing.assert_almost_equal (pandas/src/testing.c:2101) File "/Users/facaiyan/Workshop/pandas/pandas/util/testing.py", line 913, in raise_assert_detail raise AssertionError(msg) AssertionError: numpy array are different numpy array shapes are different [left]: (10,) [right]: (9,)

@jreback if I misunderstand, please tell me.

for sparse dataframe:

df.fillna(x): convert NaN entries of df to x.

In df.add(a, fill_value=x): fill_value, convert sparse entries of df to x.

facaiy · 2016-04-16T12:36:51Z

The work is done if all tests are passed.
Thanks, everyone.

…_value (GH12723) fix series test for sparse dataframe remove wrong test: fillna() is not equivalent of fill_value

facaiy · 2016-04-17T12:13:18Z

If it is ok,
might we close #12723 ?

jreback · 2016-04-17T13:12:44Z

issues are closed when the PRs are merged - I'll have a look soon

jreback · 2016-04-17T14:13:11Z

thanks for the fix!

jreback added Bug Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Apr 4, 2016

jreback reviewed Apr 4, 2016
View reviewed changes

sinhrks changed the title ~~Fix for #12723: Unexpected behavior with behavior with binary operators and fill…~~ Fix for #12723: Unexpected behavior with binary operators and fill… Apr 4, 2016

jreback reviewed Apr 6, 2016
View reviewed changes

facaiy force-pushed the fill_value_miss branch from 2e0ed3d to b0e4803 Compare April 8, 2016 06:42

facaiy reviewed Apr 8, 2016
View reviewed changes

jreback reviewed Apr 8, 2016
View reviewed changes

facaiy reviewed Apr 16, 2016
View reviewed changes

facaiy force-pushed the fill_value_miss branch 3 times, most recently from d2bf6c7 to 9e0835b Compare April 16, 2016 12:33

facaiy force-pushed the fill_value_miss branch 2 times, most recently from 9a41b79 to 34b0922 Compare April 16, 2016 22:07

BUG: Unexpected behavior with behavior with binary operators and fill…

c190fb8

…_value (GH12723) fix series test for sparse dataframe remove wrong test: fillna() is not equivalent of fill_value

facaiy force-pushed the fill_value_miss branch from 34b0922 to c190fb8 Compare April 16, 2016 22:08

jreback added this to the 0.18.1 milestone Apr 17, 2016

jreback closed this in 0734df0 Apr 17, 2016

facaiy deleted the fill_value_miss branch April 17, 2016 14:17

Uh oh!

Fix for #12723: Unexpected behavior with binary operators and fill… #12791

Fix for #12723: Unexpected behavior with binary operators and fill… #12791

Uh oh!

Conversation

facaiy commented Apr 4, 2016 • edited by jreback Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facaiy commented Apr 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Apr 8, 2016

Uh oh!

facaiy Apr 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facaiy Apr 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facaiy commented Apr 16, 2016

Uh oh!

facaiy commented Apr 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jreback commented Apr 17, 2016

Uh oh!

jreback commented Apr 17, 2016

Uh oh!

Uh oh!

facaiy commented Apr 4, 2016 •

edited by jreback

Loading

facaiy Apr 16, 2016 •

edited

Loading

facaiy Apr 16, 2016 •

edited

Loading

facaiy commented Apr 17, 2016 •

edited

Loading