ENH: Implement rounding for floating dtype array #38844 #39751

benoit9126 · 2021-02-11T11:04:08Z

Code proposed in the staled PR #38866

closes BUG: pandas.Series.round not yet implemented for FloatingArray #38844
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

jreback

lgtm minor comment

pandas/core/arrays/numeric.py

jreback · 2021-02-11T13:50:12Z

pandas/core/arrays/numeric.py

@@ -188,3 +189,48 @@ def reconstruct(x):
            return tuple(reconstruct(x) for x in result)
        else:
            return reconstruct(result)
+
+    def _apply(self, func: Callable, **kwargs) -> NumericArray:


cc @jorisvandenbossche @jbrockmendel do we want specific EA tests?

Not for general EA, but for the numeric arrays specifically, yes. As mentioned on the other PR, I would create a new test_function.py in the /tests/arrays/masked/ dir for this.

@jorisvandenbossche I can add a file in the /tests/arrays/masked/folder in order to test the consistency between the numpy round and the pandas round for all the following FLOAT_EA_DTYPES types. Nevertheless, I now use the any_float_allowed_nullable_dtype fixture in the test test_round_numpy_with_nan of tests/series/methods/test_round.py.

can you add general EA tests

I can add a file in the /tests/arrays/masked/ folder in order to test the consistency between the numpy round and the pandas round for all the following FLOAT_EA_DTYPES types. Nevertheless, I now use the any_float_allowed_nullable_dtype fixture in the test test_round_numpy_with_nan of tests/series/methods/test_round.py.

I would still add a small test in /tests/arrays/masked/ as well. Since this is a public method on the integer and floating arrays, we should test that specifically as well (and not only through Series)

And we should also test calling numpy.round on the array, see #38866 (comment)

jorisvandenbossche

Thanks for picking this up!

See my review at the other PR at #38866 (review), there are still a few outstanding comments I think

simonjayhawkins · 2021-02-11T16:28:18Z

pandas/tests/series/methods/test_round.py

+            index=range(3),
+            dtype=any_float_allowed_nullable_dtype,
+        )
+        result = round(ser).astype(any_float_allowed_nullable_dtype)


why is the .astype call added

You are right, it is not required.

jreback · 2021-02-21T18:20:40Z

can you merge master

simonjayhawkins · 2021-02-22T13:25:16Z

pandas/core/arrays/numeric.py

+            The image of the NumericArray
+        """
+        values = func(self._data, **kwargs)
+        return type(self)(values, self._mask.copy())


if func returns a view or is a no-op, would we need to the the same for the mask?

It was related to a comment of @jorisvandenbossche in the original PR proposal #38866

The mask needs to be copied I think? (result should not share a mask with the original array, because otherwise editing one can modify the other. We should probably also test this)

Here, I think this method should only be used with functions which return a copy (which is the case of most numpy functions I know --- without the out argument). It may need to be précised in the docstring for future developpers.

Agreed, see comment above as well. Let's not generalize this prematurely, just knowing (and documenting) the func returns a new object is fine for now IMO.

simonjayhawkins · 2021-02-22T13:25:56Z

pandas/core/arrays/numeric.py

@@ -199,3 +201,48 @@ def reconstruct(x):
            return tuple(reconstruct(x) for x in result)
        else:
            return reconstruct(result)
+
+    def _apply(self, func: Callable, **kwargs) -> NumericArray:


can you add type parameters to Callable.

I can replace it with Callable[[np.ndarray,...],np.ndarray]. Is it precise enough for you?

Callable[[np.ndarray],np.ndarray] ?

I need to add the ... for the **kwargs given to the func call.

simonjayhawkins · 2021-02-22T13:27:07Z

pandas/core/arrays/numeric.py

@@ -199,3 +201,48 @@ def reconstruct(x):
            return tuple(reconstruct(x) for x in result)
        else:
            return reconstruct(result)
+
+    def _apply(self, func: Callable, **kwargs) -> NumericArray:


the return type is the same as self?

I return return type(self)(values, self._mask.copy()) so, yes, it is a NumericArray. Do you want me to be more precise using a TypeVar?

I guess it depends on the func signature and the subclass constructors. If a func say changes dtype and the subclass constructor accepts the changed dtype. Is that what we want from apply? or would we want say an FloatArray if we apply a func with a signature Callable[[np.ndarray[int],...],np.ndarray[float]] to say a IntegerArray?

we may want to be more precise and restrictive on the callable and say that the func must return the same type (for now) and as a follow-up implement a more generic _apply function

As I mentioned on the previous PR, I would keep it focused here on just getting it working for round. It's true that if we want such a generic _apply for several cases, all those questions come up (return type of the function, does the function return a view or new object, etc). But let's deal with those issues when they actually come up.

Personally I think we can also just put those 2 lines of code in round() itself for now, but documenting the current restrictions of _apply (func needs to return new object of same dtype) is also fine for me.

simonjayhawkins · 2021-02-22T13:29:39Z

pandas/core/arrays/numeric.py

+        values = func(self._data, **kwargs)
+        return type(self)(values, self._mask.copy())
+
+    def round(self, decimals: int = 0, *args, **kwargs) -> NumericArray:


return type is same as self?

simonjayhawkins · 2021-02-22T13:32:36Z

pandas/core/arrays/numeric.py

+        values = func(self._data, **kwargs)
+        return type(self)(values, self._mask.copy())
+
+    def round(self, decimals: int = 0, *args, **kwargs) -> NumericArray:


is *args needed? having *args after a keyword argument can be problematic

I did the same as already done in some other files of the library. For instance here

jorisvandenbossche

Thanks for the updates!

jorisvandenbossche · 2021-02-24T08:49:02Z

pandas/core/arrays/numeric.py

+
+    def round(self, decimals: int = 0, *args, **kwargs) -> NumericArray:
+        """
+        Round each value in NumericArray a to the given number of decimals.


Suggested change

Round each value in NumericArray a to the given number of decimals.

Round each value in the array to the given number of decimals.

(NumericArray is not really public, so would try to avoid it in the docstring where it's not really needed)

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

pandas/core/arrays/numeric.py

@@ -188,3 +189,48 @@ def reconstruct(x):
            return tuple(reconstruct(x) for x in result)
        else:
            return reconstruct(result)
+
+    def _apply(self, func: Callable, **kwargs) -> NumericArray:


I can add a file in the /tests/arrays/masked/ folder in order to test the consistency between the numpy round and the pandas round for all the following FLOAT_EA_DTYPES types. Nevertheless, I now use the any_float_allowed_nullable_dtype fixture in the test test_round_numpy_with_nan of tests/series/methods/test_round.py.

I would still add a small test in /tests/arrays/masked/ as well. Since this is a public method on the integer and floating arrays, we should test that specifically as well (and not only through Series)

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

pandas/core/arrays/numeric.py

@@ -199,3 +201,48 @@ def reconstruct(x):
            return tuple(reconstruct(x) for x in result)
        else:
            return reconstruct(result)
+
+    def _apply(self, func: Callable, **kwargs) -> NumericArray:


As I mentioned on the previous PR, I would keep it focused here on just getting it working for round. It's true that if we want such a generic _apply for several cases, all those questions come up (return type of the function, does the function return a view or new object, etc). But let's deal with those issues when they actually come up.

Personally I think we can also just put those 2 lines of code in round() itself for now, but documenting the current restrictions of _apply (func needs to return new object of same dtype) is also fine for me.

jorisvandenbossche · 2021-02-24T08:55:44Z

pandas/core/arrays/numeric.py

@@ -188,3 +189,48 @@ def reconstruct(x):
            return tuple(reconstruct(x) for x in result)
        else:
            return reconstruct(result)
+
+    def _apply(self, func: Callable, **kwargs) -> NumericArray:


And we should also test calling numpy.round on the array, see #38866 (comment)

jorisvandenbossche · 2021-02-24T08:57:57Z

pandas/core/arrays/numeric.py

+            The image of the NumericArray
+        """
+        values = func(self._data, **kwargs)
+        return type(self)(values, self._mask.copy())


Agreed, see comment above as well. Let's not generalize this prematurely, just knowing (and documenting) the func returns a new object is fine for now IMO.

benoit9126 · 2021-02-24T16:53:05Z

Thanks for all your reviews! I will take time to implement them, but I can't before Saturday.

benoit9126 · 2021-03-01T11:27:43Z

I pushed some modifications:

I added a small test_function.py in tests/arrays/masked/
I merged the _apply and the round methods to have only the round method
I merged master

jreback · 2021-03-01T14:28:28Z

some precommit issues

Performing static analysis using mypy
pandas/core/arrays/numeric.py:241: error: "T" has no attribute "_data"  [attr-defined]
pandas/core/arrays/numeric.py:242: error: Too many arguments for "object"  [call-arg]
pandas/core/arrays/numeric.py:242: error: "T" has no attribute "_mask"  [attr-defined]
Found 3 errors in 1 file (checked 1242 source files)

benoit9126 · 2021-03-01T16:24:21Z

@jreback two tests failed in code that I did not write/modify? Is this normal?

jreback

lgtm. @jorisvandenbossche if any comments.

@benoit9126 the failures are know on that build currently.

jreback · 2021-03-05T21:36:42Z

thanks for merging master @benoit9126 this lgtm.

jreback · 2021-03-08T14:32:35Z

thanks @benoit9126

jorisvandenbossche · 2021-03-08T19:54:24Z

Thanks a lot @benoit9126 !

…ndas-dev#39751)

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

jreback added this to the 1.3 milestone Feb 11, 2021

jreback approved these changes Feb 11, 2021

View reviewed changes

jorisvandenbossche reviewed Feb 11, 2021

View reviewed changes

simonjayhawkins reviewed Feb 11, 2021

View reviewed changes

simonjayhawkins reviewed Feb 22, 2021

View reviewed changes

jorisvandenbossche reviewed Feb 24, 2021

View reviewed changes

jreback approved these changes Mar 1, 2021

View reviewed changes

ENH: Implement rounding for floating dtype array #38844

01e9a9f

jreback merged commit ee2b8b5 into pandas-dev:master Mar 8, 2021

benoit9126 deleted the floating_round_gh38844 branch March 8, 2021 15:34

jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Mar 11, 2021

ENH: Implement rounding for floating dtype array pandas-dev#38844 (pa…

2e8c134

…ndas-dev#39751)

	Round each value in NumericArray a to the given number of decimals.
	Round each value in the array to the given number of decimals.

ENH: Implement rounding for floating dtype array #38844 #39751

ENH: Implement rounding for floating dtype array #38844 #39751

Conversation

benoit9126 commented Feb 11, 2021

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche Feb 11, 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

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

jreback commented Feb 21, 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

benoit9126 Feb 22, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoit9126 commented Feb 24, 2021

benoit9126 commented Mar 1, 2021 • edited Loading

jreback commented Mar 1, 2021

benoit9126 commented Mar 1, 2021

jreback left a comment

Choose a reason for hiding this comment

jreback commented Mar 5, 2021 • edited Loading

jreback commented Mar 8, 2021

jorisvandenbossche commented Mar 8, 2021

jorisvandenbossche Feb 11, 2021 •

edited

Loading

benoit9126 Feb 22, 2021 •

edited

Loading

benoit9126 commented Mar 1, 2021 •

edited

Loading

jreback commented Mar 5, 2021 •

edited

Loading