Fixed IntegerArray division by 0 by TomAugspurger · Pull Request #30183 · pandas-dev/pandas

TomAugspurger · 2019-12-10T19:30:53Z

When dividing by 0, the result should be inf, not NaN.

When dividing by 0, the result should be `inf`, not `NaN`. Closes pandas-dev#27398

pandas/core/arrays/integer.py

jbrockmendel · 2019-12-10T19:51:50Z

pandas/core/arrays/integer.py

-        # may need to fill infs
-        # and mask wraparound
-        if is_float_dtype(result):
-            mask |= (result == np.inf) | (result == -np.inf)


i think this line was at the root of #27829, so this might fix that

I think you're correct.

jbrockmendel · 2019-12-10T19:52:35Z

pandas/tests/arrays/test_integer.py


+    def test_no_shared_mask(self, data):
+        result = data + 1
+        assert np.shares_memory(result._mask, data._mask) is False


i didnt know about shares_memory, neat!

pandas/tests/arrays/test_integer.py

jschendel · 2019-12-10T20:59:04Z

As an aside, floor division by zero looks a little suspicious to me. I'm not 100% sure what the expected behavior for it should be, but off the top of my head I think your test case should return the same values? Certainly doesn't need to be part of this PR though.

In [7]: a // 0
Out[7]: 
<IntegerArray>
[0, 0, 0, NaN]
Length: 4, dtype: Int64

In [8]: a // -0
Out[8]: 
<IntegerArray>
[0, 0, 0, NaN]
Length: 4, dtype: Int64

In [9]: a // 0.0
Out[9]: array([nan, nan, nan, nan])

In [10]: a // -0.0
Out[10]: array([nan, nan, nan, nan])

jschendel · 2019-12-10T21:26:16Z

pandas/tests/arrays/test_integer.py

+            values = [np.nan, -np.inf, np.inf, np.nan]
+        else:
+            values = [np.nan, np.inf, -np.inf, np.nan]
+        expected = np.array(values)


small nitpick but maybe a little more concise to do:

expected = np.array([np.nan, np.inf, -np.inf, np.nan]) if negative: expected *= -1

Given that CI is passing, I think I'm OK with how things are now :)

see tests.arithmetic.test_numeric.adjust_negative_zero

jorisvandenbossche

Looks good to me.

This can go in first before the pd.NA IntegerArray PR?

jorisvandenbossche · 2019-12-11T09:19:25Z

doc/source/whatsnew/v1.0.0.rst

 - Bug in :meth:`DataFrame.append` that raised ``IndexError`` when appending with empty list (:issue:`28769`)
 - Fix :class:`AbstractHolidayCalendar` to return correct results for
  years after 2030 (now goes up to 2200) (:issue:`27790`)
+- Fixed :class:`IntegerArray` returning ``NA`` rather than ``inf`` for operations dividing by 0 (:issue:`27398`)


This is a bit confusingly worded I think, as I would interpret this that it now returns NaN and no longer inf, while it is the other way around?

Also NA -> NaN, as the result is about floats with NaNs

for non-trivially to understand changes we usually have a sub-section, but up to you.

jreback

lgtm. if you want to expand whatsnew note cool.

jreback · 2019-12-11T12:56:25Z

doc/source/whatsnew/v1.0.0.rst

 - Bug in :meth:`DataFrame.append` that raised ``IndexError`` when appending with empty list (:issue:`28769`)
 - Fix :class:`AbstractHolidayCalendar` to return correct results for
  years after 2030 (now goes up to 2200) (:issue:`27790`)
+- Fixed :class:`IntegerArray` returning ``NA`` rather than ``inf`` for operations dividing by 0 (:issue:`27398`)


for non-trivially to understand changes we usually have a sub-section, but up to you.

jreback · 2019-12-11T12:57:41Z

pandas/tests/arrays/test_integer.py

        with pytest.raises(NotImplementedError):
            opa(np.arange(len(s)).reshape(-1, len(s)))

+    @pytest.mark.parametrize("zero, negative", [(0, False), (0.0, False), (-0.0, True)])


might be good to make a fixture for zeros as we do a lot of these types of tests for series ops, cc @jbrockmendel

TomAugspurger · 2019-12-11T14:14:29Z

I'd prefer to just merge this as is. I have a followup PR fixing IntergerArray.__pow__ which depends on this and I can clarify the whatsnew there.

TomAugspurger · 2019-12-11T14:15:22Z

This can go in first before the pd.NA IntegerArray PR?

Yes, this and my next PR fixing IntegerArray.__pow__ are splitting off tangentially-related components of that large PR.

jorisvandenbossche · 2019-12-11T17:09:19Z

I'd prefer to just merge this as is. I have a followup PR fixing IntergerArray.pow which depends on this and I can clarify the whatsnew there.

Sounds good

When dividing by 0, the result should be `inf`, not `NaN`. Closes pandas-dev#27398

Fixed IntegerArray division by 0

81b18ca

When dividing by 0, the result should be `inf`, not `NaN`. Closes pandas-dev#27398

TomAugspurger force-pushed the integer-array-pow branch from b0693be to 81b18ca Compare December 10, 2019 19:33

TomAugspurger commented Dec 10, 2019

View reviewed changes

pandas/core/arrays/integer.py Show resolved Hide resolved

jbrockmendel reviewed Dec 10, 2019

View reviewed changes

fixed merge conflict

5374d8b

jschendel reviewed Dec 10, 2019

View reviewed changes

pandas/tests/arrays/test_integer.py Outdated Show resolved Hide resolved

negative

f0264f9

TomAugspurger mentioned this pull request Dec 10, 2019

Maybe buggy nullable integer floor division by 0 #30188

Open

jschendel reviewed Dec 10, 2019

View reviewed changes

jorisvandenbossche approved these changes Dec 11, 2019

View reviewed changes

simonjayhawkins added ExtensionArray Extending pandas with custom dtypes or arrays. Numeric Operations Arithmetic, Comparison, and Logical operations labels Dec 11, 2019

simonjayhawkins added this to the 1.0 milestone Dec 11, 2019

jreback approved these changes Dec 11, 2019

View reviewed changes

TomAugspurger mentioned this pull request Dec 11, 2019

Fix IntegerArray pow for special cases #30210

Merged

jorisvandenbossche merged commit 8841969 into pandas-dev:master Dec 11, 2019

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

Fixed IntegerArray division by 0 (pandas-dev#30183)

fbe2dd8

When dividing by 0, the result should be `inf`, not `NaN`. Closes pandas-dev#27398

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

Fixed IntegerArray division by 0 (pandas-dev#30183)

a6ec84d

When dividing by 0, the result should be `inf`, not `NaN`. Closes pandas-dev#27398

jbrockmendel mentioned this pull request Jan 2, 2022

divide by 0 for Int64 is inconsistent with int64 #22793

Closed

Uh oh!

Conversation

TomAugspurger commented Dec 10, 2019 • edited by jreback Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

jschendel commented Dec 10, 2019

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!

jorisvandenbossche left a comment

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!

jreback left a comment

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!

TomAugspurger commented Dec 11, 2019

Uh oh!

TomAugspurger commented Dec 11, 2019

Uh oh!

jorisvandenbossche commented Dec 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TomAugspurger commented Dec 10, 2019 •

edited by jreback

Loading