ENH: Implement unary operators for FloatingArray class #39916

zitorelova · 2021-02-19T20:02:42Z

closes BUG: TypeError: bad operand type for unary -: 'FloatingArray' #38749
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

pandas/tests/arrays/floating/test_arithmetic.py

pandas/tests/series/test_unary.py

arw2019 · 2021-02-19T22:13:57Z

doc/source/whatsnew/v1.3.0.rst

@@ -425,6 +425,7 @@ ExtensionArray

 - Bug in :meth:`DataFrame.where` when ``other`` is a :class:`Series` with :class:`ExtensionArray` dtype (:issue:`38729`)
 - Fixed bug where :meth:`Series.idxmax`, :meth:`Series.idxmin` and ``argmax/min`` fail when the underlying data is :class:`ExtensionArray` (:issue:`32749`, :issue:`33719`, :issue:`36566`)
+- Bug in :class:`FloatingArray` raises ``TypeError`` when trying to apply a negative operator (:issue:`38749`)


this could maybe go under enhancements since you're basically implementing the negation op

moved the whatsnew entry to enhancements

jreback

testing comment, otherwise lgtm. ping on green.

jreback · 2021-02-21T17:57:59Z

pandas/conftest.py

+    and float ea dtypes.
+
+    * 'Int8'
+    * 'Int16'


i would rather have any_nullable_real_dtype (e..g. include unsigned ints), this seems an oddball.

i don't think unsigned ints are supposed to be used here. It wouldn't make sense to use unary operators like + and - on unsigned dtypes right?

and that's exatly the reason to test this

pos and abs should for sure work and give consistent results. uints work as well (though the results may surprise you, they are consistent with how uints work generally). so pls test these.

if that's the case, should we separate the tests for uints? I think a single test handling all real dtypes would be too big.I would split the tests into signed ints, unsigned ints, and floats.

i don't see why, the way you have your tests now it should just work

I'm not sure how the tests for uints should be implemented? For test cases where there are negative numbers in the input, you can't create a series. Should I just use pytest.raises in these cases?

numpy gives a result so this will as well (it wraps around but that's irrelevant)

jreback · 2021-02-22T13:16:06Z

pandas/conftest.py

+    and float ea dtypes.
+
+    * 'Int8'
+    * 'Int16'


and that's exatly the reason to test this

jreback · 2021-02-22T13:17:33Z

pandas/conftest.py

+    and float ea dtypes.
+
+    * 'Int8'
+    * 'Int16'


pos and abs should for sure work and give consistent results. uints work as well (though the results may surprise you, they are consistent with how uints work generally). so pls test these.

simonjayhawkins · 2021-02-22T13:51:11Z

pandas/core/arrays/floating.py

@@ -257,6 +257,15 @@ 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)


should return a copy of the mask, xref #39943

should probably move the integer implementations (after fixed #39943 (comment)) to the base clase.

simonjayhawkins · 2021-02-22T18:10:47Z

pandas/core/arrays/floating.py


    def __pos__(self):
-        return self
+        return self.copy()


This is inconsistent with IntegerArray, although it's not clear to me what the correct return type should be. I think we return a copy for a Series, and a new object with the same backing data for TimedeltaArray. numpy returns a new object. python returns the same object with scalars.

maybe just match IntegerArray for now so that we can move these methods to the base class

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

@zitorelova will want to rebase this after #39971 (merging soon). just to ensure copied masks.

jreback · 2021-02-24T00:54:22Z

pandas/core/arrays/floating.py

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


shoulnt' this be the same? (e.g. self.copy()) to preserve semantics

@simonjayhawkins noted that we should be consistent with the IntegerArray class

This is inconsistent with IntegerArray, although it's not clear to me what the correct return type should be. I think we return a copy for a Series, and a new object with the same backing data for TimedeltaArray. numpy returns a new object. python returns the same object with scalars.

maybe just match IntegerArray for now so that we can move these methods to the base class

jorisvandenbossche

@zitorelova thanks for working on this!

#39971 added a test to tests/arrays/masked/test_arithmetic.py for checking the mask is no longer shared. Can you update that test to also add float dtype? (you can use the data fixture defined in that file for this, which already includes that)

jorisvandenbossche · 2021-02-24T08:32:05Z

pandas/core/arrays/floating.py

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

+    def __neg__(self):


I think @simonjayhawkins already mentioned it as well, but can you move those definitions you added here to the base class NumericArray in arrays/numeric.py instead?

I've moved the operator definitions to arrays/numeric.py

pep8speaks · 2021-02-26T20:53:32Z

Hello @zitorelova! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-27 18:31:34 UTC

pandas/core/arrays/numeric.py

zitorelova · 2021-02-26T23:58:05Z

throwing errors with stata files

simonjayhawkins · 2021-02-27T12:06:15Z

throwing errors with stata files

fixed in #40094. restarted ci. if not passing with restart, will need to merge master.

simonjayhawkins · 2021-02-27T12:11:43Z

pandas/tests/arrays/masked/test_arithmetic.py

-    s = pd.Series(values, dtype=dtype)
+    data, _ = data
+    if data.dtype in ["Float32", "Float64"] and op == "__invert__":
+        pytest.skip("invert is not implemented for float ea dtypes")


maybe change this to add an xfail marker instead of skipping

Set to xfail

jreback · 2021-02-27T18:43:35Z

lgtm ping on green.

zitorelova · 2021-02-27T22:19:09Z

checks complete @jreback

simonjayhawkins · 2021-02-28T08:38:29Z

Thanks @zitorelova

arw2019 reviewed Feb 19, 2021

View reviewed changes

zitorelova changed the title ~~BUG TypeError when using negative operator on FloatingArray~~ ENH Implement unary operators for FloatingArray class Feb 19, 2021

zitorelova changed the title ~~ENH Implement unary operators for FloatingArray class~~ ENH: Implement unary operators for FloatingArray class Feb 19, 2021

jreback requested changes Feb 21, 2021

View reviewed changes

jreback added Enhancement ExtensionArray Extending pandas with custom dtypes or arrays. labels Feb 21, 2021

jreback added this to the 1.3 milestone Feb 21, 2021

jreback requested changes Feb 22, 2021

View reviewed changes

simonjayhawkins reviewed Feb 22, 2021

View reviewed changes

zitorelova added 17 commits February 23, 2021 21:47

Add neg, pos, abs methods to FloatingArray

e40c3c6

Add tests for arrays

9929bc4

Add tests for Series objects

1952467

Add whatsnew entry for v1.3.0

5fdf667

Fix pre-commit errors

de24336

Simplify testing for FloatingArray unary operators

853bc5d

Move whatsnew entry to enhancements

6a3e07d

Add fixture for signed nullable numeric dtypes

03801fc

Consolidate all numeric unary operator tests

1b9759d

Consolidate integer unary operator tests

bb77076

Fix shared mask bug

42e8cdf

Add test for float op mask

957edc3

Do not return copy when using pos op

38c610b

Don't test pos

eba6e04

Remove fixture

be5eb84

Edit test to include all numeric dtypes

9367b1b

Fix pre-commit

3e67655

zitorelova force-pushed the float-unary-bug branch from 36ed0bf to 3e67655 Compare February 23, 2021 22:04

jreback requested changes Feb 24, 2021

View reviewed changes

jorisvandenbossche reviewed Feb 24, 2021

View reviewed changes

zitorelova added 2 commits February 26, 2021 02:53

Move unary operator definitions to NumericArray

495ddb7

Test mask on all EA dtypes

0162e07

Add newline to numeric.py

7dda62d

jorisvandenbossche reviewed Feb 26, 2021

View reviewed changes

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

Remove definitions cause they've been moved to NumericArray

41e42fc

simonjayhawkins reviewed Feb 27, 2021

View reviewed changes

simonjayhawkins mentioned this pull request Feb 27, 2021

REGR: Fix assignment bug for unary operators #39971

Merged

4 tasks

zitorelova added 2 commits February 27, 2021 17:42

Xfail instead of skip for invert ops on float ea dtypes

cf2732a

Fix pre-commit error

3cc5212

jreback approved these changes Feb 27, 2021

View reviewed changes

simonjayhawkins merged commit 4c5e6fa into pandas-dev:master Feb 28, 2021

zitorelova deleted the float-unary-bug branch February 28, 2021 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Implement unary operators for FloatingArray class #39916

ENH: Implement unary operators for FloatingArray class #39916

zitorelova commented Feb 19, 2021

arw2019 Feb 19, 2021

zitorelova Feb 19, 2021

jreback left a comment

jreback Feb 21, 2021

zitorelova Feb 21, 2021 •

edited

Loading

jreback Feb 22, 2021

jreback Feb 22, 2021

zitorelova Feb 22, 2021

jreback Feb 22, 2021

zitorelova Feb 23, 2021

jreback Feb 23, 2021

jreback Feb 22, 2021

jreback Feb 22, 2021

simonjayhawkins Feb 22, 2021

simonjayhawkins Feb 22, 2021

jreback commented Feb 22, 2021

jreback Feb 24, 2021

zitorelova Feb 26, 2021

jorisvandenbossche left a comment

jorisvandenbossche Feb 24, 2021

zitorelova Feb 26, 2021

pep8speaks commented Feb 26, 2021 •

edited

Loading

zitorelova commented Feb 26, 2021

simonjayhawkins commented Feb 27, 2021

simonjayhawkins Feb 27, 2021

zitorelova Feb 27, 2021

jreback commented Feb 27, 2021

zitorelova commented Feb 27, 2021

simonjayhawkins commented Feb 28, 2021

ENH: Implement unary operators for FloatingArray class #39916

ENH: Implement unary operators for FloatingArray class #39916

Conversation

zitorelova commented Feb 19, 2021

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

zitorelova Feb 21, 2021 • edited Loading

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

Choose a reason for hiding this comment

jreback commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Feb 26, 2021 • edited Loading

Comment last updated at 2021-02-27 18:31:34 UTC

zitorelova commented Feb 26, 2021

simonjayhawkins commented Feb 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 27, 2021

zitorelova commented Feb 27, 2021

simonjayhawkins commented Feb 28, 2021

zitorelova Feb 21, 2021 •

edited

Loading

pep8speaks commented Feb 26, 2021 •

edited

Loading