REGR: Fix assignment bug for unary operators #39971

dsaxton · 2021-02-22T16:11:16Z

closes BUG: unary operators on IntegerArray return shared mask creating inconsistencies when assigning null and non-null values #39943
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

If there was somehow a way to copy the mask only on write so we aren't always copying that may be better. @simonjayhawkins I put this in the IntegerArray tests even though we're technically checking a Series, let me know if you'd prefer I rewrite or move elsewhere.

simonjayhawkins · 2021-02-22T16:16:20Z

If there was somehow a way to copy the mask only on write so we aren't always copying that may be better.

for a backport, let's keep this simple.

simonjayhawkins · 2021-02-22T16:19:22Z

pandas/tests/arrays/integer/test_arithmetic.py

@@ -317,3 +317,13 @@ def test_abs_nullable_int(any_signed_nullable_int_dtype, source, target):
    result = abs(s)
    expected = pd.array(target, dtype=dtype)
    tm.assert_extension_array_equal(result, expected)
+
+
+@pytest.mark.parametrize("op", ["__neg__", "__abs__"])


can we also test __invert__, since I believe has the same issue.

Good catch, that required another change

jreback · 2021-02-22T17:48:43Z

If there was somehow a way to copy the mask only on write so we aren't always copying that may be better. @simonjayhawkins I put this in the IntegerArray tests even though we're technically checking a Series, let me know if you'd prefer I rewrite or move elsewhere.

this is not necessary, these copies are very cheap, semantics are way more important.

jreback · 2021-02-22T17:49:47Z

pandas/tests/arrays/integer/test_arithmetic.py

@@ -317,3 +317,13 @@ def test_abs_nullable_int(any_signed_nullable_int_dtype, source, target):
    result = abs(s)
    expected = pd.array(target, dtype=dtype)
    tm.assert_extension_array_equal(result, expected)
+
+
+@pytest.mark.parametrize("op", ["__neg__", "__abs__", "__invert__"])


can you test for boolean as well

Sure, added

jreback · 2021-02-22T23:53:00Z

lgtm. waiting on CI.

simonjayhawkins · 2021-02-23T09:56:16Z

TestAccessor.test_from_coo also failing on master https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=54759&view=logs&j=4b4d380c-92b5-58cf-5b80-2c6c9b1d5805&t=8eab1181-1335-58ed-7ed5-a271d9d86005&l=80

2021-02-23T01:07:24.4100333Z 
2021-02-23T01:07:24.4100753Z ================================== FAILURES ===================================
2021-02-23T01:07:24.4101532Z _________________________ TestAccessor.test_from_coo __________________________
2021-02-23T01:07:24.4102065Z [gw0] win32 -- Python 3.7.9 C:\Miniconda\envs\pandas-dev\python.exe
2021-02-23T01:07:24.4102361Z 
2021-02-23T01:07:24.4103135Z self = <pandas.tests.arrays.sparse.test_array.TestAccessor object at 0x000001317E6BC888>
2021-02-23T01:07:24.4103462Z 
2021-02-23T01:07:24.4103797Z     @td.skip_if_no_scipy
2021-02-23T01:07:24.4104207Z     def test_from_coo(self):
2021-02-23T01:07:24.4104597Z         import scipy.sparse
2021-02-23T01:07:24.4104947Z     
2021-02-23T01:07:24.4105283Z         row = [0, 3, 1, 0]
2021-02-23T01:07:24.4105674Z         col = [0, 3, 1, 2]
2021-02-23T01:07:24.4106045Z         data = [4, 5, 7, 9]
2021-02-23T01:07:24.4106494Z         sp_array = scipy.sparse.coo_matrix((data, (row, col)))
2021-02-23T01:07:24.4106960Z         result = pd.Series.sparse.from_coo(sp_array)
2021-02-23T01:07:24.4107356Z     
2021-02-23T01:07:24.4107834Z         index = pd.MultiIndex.from_arrays([[0, 0, 1, 3], [0, 2, 1, 3]])
2021-02-23T01:07:24.4108389Z         expected = pd.Series([4, 9, 7, 5], index=index, dtype="Sparse[int]")
2021-02-23T01:07:24.4109198Z >       tm.assert_series_equal(result, expected)
2021-02-23T01:07:24.4109817Z E       AssertionError: Attributes of Series are different
2021-02-23T01:07:24.4110213Z E       
2021-02-23T01:07:24.4110548Z E       Attribute "dtype" are different
2021-02-23T01:07:24.4110965Z E       [left]:  Sparse[float64, nan]
2021-02-23T01:07:24.4111366Z E       [right]: Sparse[int32, 0]
2021-02-23T01:07:24.4111579Z 
2021-02-23T01:07:24.4112141Z pandas\tests\arrays\sparse\test_array.py:1193: AssertionError
2021-02-23T01:07:24.4112871Z ============================== warnings summary ===============================

simonjayhawkins · 2021-02-23T10:07:28Z

will merge after the #39979 is merged. (otherwise may not be able to auto backport)

simonjayhawkins · 2021-02-23T11:08:13Z

@meeseeksdev backport 1.2.x

…ators

) Co-authored-by: Daniel Saxton <[email protected]>

jorisvandenbossche · 2021-02-24T08:27:52Z

doc/source/whatsnew/v1.2.3.rst

@@ -16,6 +16,7 @@ Fixed regressions
 ~~~~~~~~~~~~~~~~~

 - Fixed regression in :meth:`~DataFrame.to_excel` raising ``KeyError`` when giving duplicate columns with ``columns`` attribute (:issue:`39695`)
+- Fixed regression in :class:`IntegerArray` unary ops propagating mask on assignment (:issue:`39943`)


For future reference, I think we should generally say something like "nullable integer dtype" instead of "IntegerArray", as most users should never deal / know about IntegerArray

Valid point, I can update this

done. #40019. thanks @dsaxton

jorisvandenbossche · 2021-02-24T08:28:14Z

pandas/core/arrays/integer.py

@@ -316,13 +316,13 @@ def __init__(self, values: np.ndarray, mask: np.ndarray, copy: bool = False):
        super().__init__(values, mask, copy=copy)

    def __neg__(self):
-        return type(self)(-self._data, self._mask)
+        return type(self)(-self._data, self._mask.copy())

    def __pos__(self):
        return self


We should do the same for __pos__?

probably not for a backport. but indeed this needs some discussion to ensure consistency across the codebase. see TimedeltaArray

def __pos__(self) -> TimedeltaArray: return type(self)(self._data, freq=self.freq)

>>> import pandas as pd >>> pd.__version__ '1.3.0.dev0+804.g3289f82975' >>> >>> arr = pd.timedelta_range(0, periods=10)._values >>> arr <TimedeltaArray> ['0 days', '1 days', '2 days', '3 days', '4 days', '5 days', '6 days', '7 days', '8 days', '9 days'] Length: 10, dtype: timedelta64[ns] >>> >>> arr2 = +arr >>> >>> arr2 is arr False >>> >>> arr2._data is arr._data True >>> >>> arr2[5] = None >>> >>> arr <TimedeltaArray> ['0 days', '1 days', '2 days', '3 days', '4 days', NaT, '6 days', '7 days', '8 days', '9 days'] Length: 10, dtype: timedelta64[ns] >>> >>>

OK, I see, so that's indeed something we should fix more generally. Is there already an issue about it?

In the context here I had been thinking that something like s1 = +s should behave the same way as s1 = s, but actually I think you are right. Likely +s should return something new instead of simply a reference to the thing.

Is there already an issue about it?

no. will need to create one, since the issue that this PR addresses is slightly different. #39943 is about different arrays sharing a mask, whereas returning self for __pos__, although maybe incorrect, does not create the inconsistencies when assigning null and non-null vales

jorisvandenbossche · 2021-02-24T08:28:53Z

pandas/tests/arrays/masked/test_arithmetic.py

+
+@pytest.mark.parametrize("op", ["__neg__", "__abs__", "__invert__"])
+@pytest.mark.parametrize(
+    "values, dtype", [([1, 2, 3], "Int64"), ([True, False, True], "boolean")]


you can use the data fixture here as used in the other tests above

this is being addressed in #39916, so no action required here

dsaxton added 2 commits February 22, 2021 10:00

REGR: Fix assignment bug for unary operators

3968846

Nit

7fa3768

dsaxton requested a review from simonjayhawkins February 22, 2021 16:11

simonjayhawkins added Bug NA - MaskedArrays Related to pd.NA and nullable extension arrays labels Feb 22, 2021

simonjayhawkins added this to the 1.2.3 milestone Feb 22, 2021

simonjayhawkins reviewed Feb 22, 2021

View reviewed changes

Fix

70a8bd8

jreback requested changes Feb 22, 2021

View reviewed changes

dsaxton and others added 3 commits February 22, 2021 12:18

Include boolean dtype

57b2b27

Move to masked

52bc411

Merge branch 'master' into copy-mask-in-unary-ops

b28c417

jreback approved these changes Feb 22, 2021

View reviewed changes

jreback mentioned this pull request Feb 22, 2021

ENH: Implement unary operators for FloatingArray class #39916

Merged

4 tasks

simonjayhawkins merged commit d25cb7f into pandas-dev:master Feb 23, 2021

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Feb 23, 2021

Backport PR pandas-dev#39971: REGR: Fix assignment bug for unary oper…

9c5805a

…ators

meeseeksmachine mentioned this pull request Feb 23, 2021

Backport PR #39971 on branch 1.2.x (REGR: Fix assignment bug for unary operators) #39990

Merged

simonjayhawkins mentioned this pull request Feb 23, 2021

BUG: unary operators on IntegerArray return shared mask creating inconsistencies when assigning null and non-null values #39943

Closed

3 tasks

simonjayhawkins pushed a commit that referenced this pull request Feb 23, 2021

Backport PR #39971: REGR: Fix assignment bug for unary operators (#39990

3b671c8

) Co-authored-by: Daniel Saxton <[email protected]>

dsaxton deleted the copy-mask-in-unary-ops branch February 23, 2021 13:11

jorisvandenbossche reviewed Feb 24, 2021

View reviewed changes

simonjayhawkins mentioned this pull request Feb 27, 2021

DOC: Edit regression fix release note #40019

Merged

This was referenced Mar 8, 2021

Bump pandas from 1.2.2 to 1.2.3 turururu-zz/technical#82

Closed

Bump pandas from 1.2.1 to 1.2.3 varunrai/freqtrade#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REGR: Fix assignment bug for unary operators #39971

REGR: Fix assignment bug for unary operators #39971

dsaxton commented Feb 22, 2021

simonjayhawkins commented Feb 22, 2021

simonjayhawkins Feb 22, 2021

dsaxton Feb 22, 2021

jreback commented Feb 22, 2021 •

edited

Loading

jreback Feb 22, 2021

dsaxton Feb 22, 2021

jreback commented Feb 22, 2021

simonjayhawkins commented Feb 23, 2021 •

edited

Loading

simonjayhawkins commented Feb 23, 2021

simonjayhawkins commented Feb 23, 2021

jorisvandenbossche Feb 24, 2021

dsaxton Feb 24, 2021

simonjayhawkins Feb 27, 2021

jorisvandenbossche Feb 24, 2021

simonjayhawkins Feb 24, 2021

jorisvandenbossche Feb 24, 2021

dsaxton Feb 24, 2021

simonjayhawkins Feb 27, 2021

jorisvandenbossche Feb 24, 2021

simonjayhawkins Feb 27, 2021

REGR: Fix assignment bug for unary operators #39971

REGR: Fix assignment bug for unary operators #39971

Conversation

dsaxton commented Feb 22, 2021

simonjayhawkins commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 22, 2021

simonjayhawkins commented Feb 23, 2021 • edited Loading

simonjayhawkins commented Feb 23, 2021

simonjayhawkins commented Feb 23, 2021

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

Choose a reason for hiding this comment

jreback commented Feb 22, 2021 •

edited

Loading

simonjayhawkins commented Feb 23, 2021 •

edited

Loading