Fix bool float no coercion #18607

aschade · 2017-12-03T06:31:37Z

[ x] closes BUG: boolean frames multiplied by floats have dtypes=object #18549
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2017-12-03T06:31:41Z

Hello @aschade! Thanks for updating the PR.

In the file pandas/core/internals.py, following are the PEP8 issues :

Line 1070:80: E501 line too long (84 > 79 characters)

Comment last updated on February 03, 2018 at 23:04 Hours UTC

gfyoung · 2017-12-03T08:55:49Z

@aschade : Thanks for submitting this! Quick question: any reason why you submitted a new PR?

gfyoung · 2017-12-03T08:56:56Z

pandas/tests/frame/test_dtypes.py

@@ -669,6 +669,19 @@ def test_arg_for_errors_in_astype(self):

        df.astype(np.int8, errors='ignore')

+    from operator import (add, mul, floordiv, sub)


I see no reason why we shouldn't move this import to the top. You might even consider just doing "import operator as ops" so that you can namespace with (ops.[operation-name]).

aschade · 2017-12-03T14:40:21Z

@gfyoung the other branch got out of sync so I just made a clean one

jreback · 2017-12-03T14:52:21Z

@aschade you can simply merge master or rebase and force push to keep up to date
this branch is ok for now

codecov · 2017-12-03T15:02:35Z

Codecov Report

Merging #18607 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18607      +/-   ##
==========================================
- Coverage   91.46%   91.45%   -0.02%     
==========================================
  Files         157      157              
  Lines       51439    51441       +2     
==========================================
- Hits        47051    47044       -7     
- Misses       4388     4397       +9

Flag	Coverage Δ
#multiple	`89.32% <100%> (ø)`	⬆️
#single	`40.6% <20%> (-0.11%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/internals.py	`94.47% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️

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 0e16818...903ee6a. Read the comment docs.

codecov · 2017-12-03T15:02:49Z

Codecov Report

Merging #18607 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18607      +/-   ##
==========================================
+ Coverage   91.62%   91.62%   +<.01%     
==========================================
  Files         150      150              
  Lines       48681    48685       +4     
==========================================
+ Hits        44604    44608       +4     
  Misses       4077     4077

Flag	Coverage Δ
#multiple	`89.99% <100%> (ø)`	⬆️
#single	`41.81% <50%> (+0.07%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/ops.py	`95.74% <ø> (ø)`	⬆️
pandas/core/frame.py	`97.43% <100%> (ø)`	⬆️
pandas/core/reshape/merge.py	`94.21% <0%> (-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 3f3b4e0...0f72afd. Read the comment docs.

aschade · 2017-12-03T15:05:29Z

@jreback Ok will do next time

jreback

this also needs some tests for series

jreback · 2017-12-03T16:41:24Z

pandas/core/internals.py

@@ -1026,7 +1026,7 @@ def f(m, v, i):

        return [self.make_block(new_values, fastpath=True)]

-    def coerce_to_target_dtype(self, other):
+    def coerce_to_target_dtype(self, other, force_coericion=False):


huh? I don't think you should be touching this at all, this is much more related to some logic in ops.py

Looks like the problem is that before the operation is done, coerce_to_target_dtype coerces the np.array to object in this block:

if self.is_bool or is_object_dtype(dtype) or is_bool_dtype(dtype): # we don't upcast to bool return self.astype(object)

Example here:

>>> import numpy as np >>> (np.array([True]) * 1.0).dtype dtype('float64') >>> (np.array([True], dtype='object') * 1.0).dtype dtype('O')

you are missing the point; the fix is much higher up the stack

jreback · 2017-12-03T16:42:20Z

pandas/tests/frame/test_dtypes.py

@@ -669,6 +670,17 @@ def test_arg_for_errors_in_astype(self):

        df.astype(np.int8, errors='ignore')

+    @pytest.mark.parametrize("num", [1.0, 1])


should be in pandas/tests/frame/test_operators

jreback · 2017-12-03T16:42:54Z

pandas/tests/frame/test_dtypes.py

+    def test_assert_list_and_bool_coerce(self, num, struct, op):
+        # issue 18549
+        target_type = np.array([op(num, num)]).dtype
+        res = op(struct([True]), num).dtypes


actually compare the result and directly construct the expected value; use assert_frame_equal

jreback · 2017-12-07T11:39:19Z

pandas/core/ops.py

@@ -1264,6 +1272,9 @@ def f(self, other, axis=default_axis, level=None, fill_value=None):
            if fill_value is not None:
                self = self.fillna(fill_value)

+            if is_number(other):


I think this is better handled in combine_const

what do you think would be the best approach in that method? cast bool columns to float/int before eval is used?

jreback

looks good. small change.

jreback · 2017-12-15T02:00:16Z

pandas/core/frame.py

-        return self._constructor(new_data)
+        result = self._constructor(new_data)
+        if is_number(other):
+            coerce_bool_cols = {col: type(other) for col in self.select_dtypes('bool')}


maybe something more along the lines of

In [18]: df = pd.DataFrame({'A': [True], 'B':[1], 'C':[False]}) In [19]: df Out[19]: A B C 0 True 1 False # this is the internal routine, but appropriate here In [20]: bt = df.dtypes.apply(pandas.core.dtypes.common.is_bool_dtype) In [22]: i = bt[bt].index In [23]: i Out[23]: Index(['A', 'C'], dtype='object')

then
result[i] = result[i].astype('bool')

as select_dtypes copies things.

aschade · 2017-12-15T16:37:12Z

The Series tests pass for me locally but on AppVeyer they're failing since the operations are casting the Series' dtype to int32 and on my local machine they're int64. Not really sure how to debug this since I can't reproduce locally.

aschade · 2017-12-16T15:51:45Z

@jreback

jreback · 2017-12-18T14:20:59Z

pandas/tests/series/test_operators.py

+        # issue 18549
+        ser1 = pd.Series([op(num, num)])
+        ser2 = pd.Series([True])
+        assert_series_equal(ser1, op(ser2, num))


use
result = Series([op(num, num)])
and
assert_series_equal(result, expected)
and write the expected out (and same in DataFrame tests).

actually its hard to tell which is result and expected here.

jreback · 2017-12-18T14:31:46Z

pandas/core/frame.py

+        if is_number(other):
+            bool_cols = self.dtypes.apply(is_bool_dtype)
+            index = bool_cols[bool_cols].index
+            result[index] = result[index].astype(type(other))


so on windows type(other) if otheris an int would yield np.int32, while on all other systems this would be np.int64.

is there a reason we don't just cast back to bool for these columns?

Hmmm shouldn't these cols be coerced to the same dtype as other? Ie copy the behavior from python:

>>> type(1.0 * True) <class 'float'> >>> 1.0 * True 1.0

jreback

can you rebase and i will take a look

jreback · 2018-02-01T13:17:24Z

doc/source/whatsnew/v0.22.0.txt

@@ -262,6 +262,7 @@ Conversion
 - Fixed a bug where creating a Series from an array that contains both tz-naive and tz-aware values will result in a Series whose dtype is tz-aware instead of object (:issue:`16406`)
 - Adding a ``Period`` object to a ``datetime`` or ``Timestamp`` object will now correctly raise a ``TypeError`` (:issue:`17983`)
 - Fixed a bug where ``FY5253`` date offsets could incorrectly raise an ``AssertionError`` in arithmetic operatons (:issue:`14774`)
+- Bug in :class:`frame` where math operations on a `DataFrame` containing `bool` are coerced to `object` and not the target `dtype` (:issue: `18607`)


can you move to 0.23.0

jreback · 2018-03-16T22:08:29Z

closing as stale

gfyoung added Bug Dtype Conversions Unexpected or buggy dtype conversions Numeric Operations Arithmetic, Comparison, and Logical operations Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Dec 3, 2017

gfyoung reviewed Dec 3, 2017

View reviewed changes

jreback requested changes Dec 3, 2017

View reviewed changes

jreback requested changes Dec 7, 2017

View reviewed changes

jreback requested changes Dec 15, 2017

View reviewed changes

jreback requested changes Dec 18, 2017

View reviewed changes

jreback requested changes Feb 1, 2018

View reviewed changes

aschade force-pushed the fix-bool-float-no-coercion branch 9 times, most recently from 7b32dc0 to 59cabff Compare February 3, 2018 04:26

aschade added 2 commits February 3, 2018 18:02

BUG: Returning other dtype when first is bool and other is int or float

27673ff

TST: Fixed test failure

0f72afd

aschade force-pushed the fix-bool-float-no-coercion branch from 59cabff to 0f72afd Compare February 3, 2018 23:04

jreback closed this Mar 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bool float no coercion #18607

Fix bool float no coercion #18607

aschade commented Dec 3, 2017 •

edited by gfyoung

Loading

pep8speaks commented Dec 3, 2017 •

edited

Loading

gfyoung commented Dec 3, 2017 •

edited

Loading

gfyoung Dec 3, 2017

aschade commented Dec 3, 2017

jreback commented Dec 3, 2017

codecov bot commented Dec 3, 2017

codecov bot commented Dec 3, 2017 •

edited

Loading

aschade commented Dec 3, 2017

jreback left a comment

jreback Dec 3, 2017

aschade Dec 3, 2017

jreback Dec 3, 2017

jreback Dec 3, 2017

jreback Dec 3, 2017

jreback Dec 7, 2017

aschade Dec 9, 2017

jreback Dec 9, 2017

jreback left a comment

jreback Dec 15, 2017

aschade commented Dec 15, 2017

aschade commented Dec 16, 2017

jreback Dec 18, 2017

jreback Dec 18, 2017

jreback Dec 18, 2017

aschade Dec 20, 2017

jreback left a comment

jreback Feb 1, 2018

jreback commented Mar 16, 2018

		@@ -669,6 +669,19 @@ def test_arg_for_errors_in_astype(self):

		df.astype(np.int8, errors='ignore')

		from operator import (add, mul, floordiv, sub)

		@@ -669,6 +670,17 @@ def test_arg_for_errors_in_astype(self):

		df.astype(np.int8, errors='ignore')

		@pytest.mark.parametrize("num", [1.0, 1])

Fix bool float no coercion #18607

Fix bool float no coercion #18607

Conversation

aschade commented Dec 3, 2017 • edited by gfyoung Loading

pep8speaks commented Dec 3, 2017 • edited Loading

Comment last updated on February 03, 2018 at 23:04 Hours UTC

gfyoung commented Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

aschade commented Dec 3, 2017

jreback commented Dec 3, 2017

codecov bot commented Dec 3, 2017

Codecov Report

codecov bot commented Dec 3, 2017 • edited Loading

Codecov Report

aschade commented Dec 3, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschade commented Dec 15, 2017

aschade commented Dec 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Mar 16, 2018

aschade commented Dec 3, 2017 •

edited by gfyoung

Loading

pep8speaks commented Dec 3, 2017 •

edited

Loading

gfyoung commented Dec 3, 2017 •

edited

Loading

codecov bot commented Dec 3, 2017 •

edited

Loading