DOC: Updated convert_dtype of Series.apply #39941

sukriti1 · 2021-02-20T20:29:10Z

closes BUG: apply on Series of dtype category produces dtype category when float was expected #39580
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

This updates the docs for parameter convert_dtype in pandas.Series.apply and adds dtypes for which the original dtype is kept.

pep8speaks · 2021-02-20T20:29:12Z

Hello @sukriti1! 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-04-16 09:22:45 UTC

MarcoGorelli · 2021-02-21T08:16:44Z

pandas/core/series.py

+            False, leave as dtype=object. For the dtypes Categorical,
+            Sparse, Interval, Period, DatetimeArray and TimedeltaArray
+            the original dtype is kept.


Hi @sukriti1 - how did you find this list of types? Was it from the code, or are they documented elsewhere?

Hi @MarcoGorelli - I found this list of type from this comment :
#39580 (comment)

I see, thanks!

OK, so instead of listing the dtypes here (which could expand in the future) maybe it's better to just say something like

note that conversion doesn't happen for extension array dtypes which have a map method (e.g. Categorical)

cc @attack68

@MarcoGorelli Changes made!

Agreeing with @MarcoGorelli's second thought here, I do like the original wording better. Do we have tests for this?

Perhaps we can also add something to the effect that the list is not necessarily exhaustive.

Like, "For some dtypes, such as Categorical,
Sparse, Interval, Period, DatetimeArray, and TimedeltaArray,
the original dtype is kept."?

MarcoGorelli · 2021-03-01T09:58:21Z

Thanks for updating

Hey @rhshadrach - any thoughts on this? Re-reading this, I think my suggestion wasn't a very good one (besides, it's ._values that can have .map, not the dtype), I don't think internals need describing to users

jreback · 2021-03-05T01:08:00Z

pandas/core/series.py

@@ -4056,7 +4056,9 @@ def apply(
            Python function or NumPy ufunc to apply.
        convert_dtype : bool, default True
            Try to find better dtype for elementwise function results. If
-            False, leave as dtype=object.
+            False, leave as dtype=object. Note that conversion does not
+            happen for extension array dtypes which have a map method


Suggested change

happen for extension array dtypes which have a map method

happen for extension array dtypes, such as Categorical.

There are different behaviors for different extension array dtypes - using convert_dtype=False with Series([1], dtype=pd.Int64Dtype()) and Series([1], dtype="category") results in object and category dtypes respectively. Agreed on not mentioning the map method here, but it seems better to me to spell out which pandas dtypes will not come out as object when convert_dtype is False.

jreback · 2021-03-05T01:08:07Z

pandas/core/series.py

-            False, leave as dtype=object.
+            False, leave as dtype=object. Note that conversion does not
+            happen for extension array dtypes which have a map method
+            (e.g. Categorical).


delete this line

jreback

ping on green

mkp-gebensleben · 2021-03-05T08:45:17Z

Do you really expect users to know what an extension array is? Maybe it's my fault, but I have been using pandas on and off for years and never heard of extension arrays.

If a user wants to know what dtypes are excluded from conversion they will land on this page next. Which quite frankly is gibberish if you are not educated in pandas internals.

But even if the user gets past that, they now will have to research which extension arrays have a map method and which ones do not.

MarcoGorelli · 2021-03-05T09:32:06Z

Hi @mkp-gebensleben - if you read through the discussion, you'll see that I went back on my initial suggestion to mention extension arrays and the map method

rhshadrach · 2021-03-14T10:57:23Z

pandas/core/series.py

@@ -4067,7 +4067,8 @@ def apply(
            Python function or NumPy ufunc to apply.
        convert_dtype : bool, default True
            Try to find better dtype for elementwise function results. If
-            False, leave as dtype=object.
+            False, leave as dtype=object. Note that conversion does not
+            happen for extension array dtypes, such as Categorical.


The phrase "Note that conversion does not happen" seems ambiguous to me. Is it that the conversion to object doesn't happen, or the conversion to find a better dtype? Maybe something like "Note that the dtype is always preserved for extension array dtypes, such as Categorical."

Hi @sukriti1 - do you want to update as per @rhshadrach 's suggestion? Sorry for having led you down the wrong route initially

rhshadrach · 2021-04-15T04:20:11Z

@sukriti1 - Are you interested in finishing this up? If not, would you mind if I pushed it over the finish line?

sukriti1 · 2021-04-16T09:24:19Z

@rhshadrach Sorry for the delay just pushed the changes! Please let me know if there's anymore feedback

MarcoGorelli · 2021-04-25T17:53:59Z

Thanks @sukriti1

rhshadrach · 2021-04-27T02:44:39Z

@sukriti1 @MarcoGorelli

#39941 (comment) - As commented there, I think the changes made in the PR are not quite correct. While it seems to me undesirable to list out the behavior for various extension arrays, I don't see another way to have correct documentation.

An alternative would be to change the behavior of the method itself to match the current docstring. I don't know how palatable this would be.

MarcoGorelli · 2021-04-27T07:55:15Z

Hey @rhshadrach - is it necessary to list out behaviour for various extension arrays, or would

Note that for some extension array dtypes, such as Categorical, the dtype is always preserved.

be enough to make it correct without being too long?

rhshadrach · 2021-04-29T21:21:21Z

I didn't consider that @MarcoGorelli and do think it's better than incorrect documentation (although it really doesn't tell the user what will happen). I am still wondering if it's not possible to make the current documentation (that is now in master) correct and will take a look at this in a few days.

* DOC: Updated convert_dtype of Series.apply * DOC: Updated convert_dtype for pandas.Series.apply * DOC: Updated convert_dtype for pandas.Series.apply * Editing the doc * only mention categorical as example * updates Co-authored-by: Marco Gorelli <[email protected]>

MarcoGorelli · 2021-06-12T10:22:48Z

I didn't consider that @MarcoGorelli and do think it's better than incorrect documentation (although it really doesn't tell the user what will happen). I am still wondering if it's not possible to make the current documentation (that is now in master) correct and will take a look at this in a few days.

Hey @rhshadrach - just checking if you've had further thoughts on this, and what your preference would be

* DOC: Updated convert_dtype of Series.apply * DOC: Updated convert_dtype for pandas.Series.apply * DOC: Updated convert_dtype for pandas.Series.apply * Editing the doc * only mention categorical as example * updates Co-authored-by: Marco Gorelli <[email protected]>

DOC: Updated convert_dtype of Series.apply

28a5b32

sukriti1 closed this Feb 20, 2021

DOC: Updated convert_dtype for pandas.Series.apply

9bef842

sukriti1 reopened this Feb 20, 2021

DOC: Updated convert_dtype for pandas.Series.apply

1555836

MarcoGorelli reviewed Feb 21, 2021

View reviewed changes

Editing the doc

3dfad1c

jreback added the Docs label Mar 5, 2021

jreback reviewed Mar 5, 2021

View reviewed changes

jreback requested changes Mar 5, 2021

View reviewed changes

jreback added the Apply Apply, Aggregate, Transform, Map label Mar 5, 2021

MarcoGorelli self-requested a review March 10, 2021 09:23

MarcoGorelli added 2 commits March 10, 2021 09:25

only mention categorical as example

8cb2910

Merge remote-tracking branch 'upstream/master' into pr/sukriti1/39941

ba8b87b

rhshadrach requested changes Mar 14, 2021

View reviewed changes

sukriti1 added 2 commits April 16, 2021 05:18

updates

8b9c378

changes

b532c5c

MarcoGorelli added this to the 1.3 milestone Apr 25, 2021

MarcoGorelli merged commit accf345 into pandas-dev:master Apr 25, 2021

rhshadrach mentioned this pull request May 4, 2021

ExtensionArray.map #23179

Closed

rhshadrach mentioned this pull request Jun 23, 2021

DOC: Correct docstring for Series.apply #42193

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Updated convert_dtype of Series.apply #39941

DOC: Updated convert_dtype of Series.apply #39941

sukriti1 commented Feb 20, 2021 •

edited

Loading

pep8speaks commented Feb 20, 2021 •

edited

Loading

MarcoGorelli Feb 21, 2021

sukriti1 Feb 21, 2021

MarcoGorelli Feb 23, 2021 •

edited

Loading

sukriti1 Mar 1, 2021

rhshadrach Mar 2, 2021

rhshadrach Mar 2, 2021

MarcoGorelli Mar 4, 2021

MarcoGorelli commented Mar 1, 2021

jreback Mar 5, 2021

rhshadrach Mar 14, 2021

jreback Mar 5, 2021

jreback left a comment

mkp-gebensleben commented Mar 5, 2021 •

edited

Loading

MarcoGorelli commented Mar 5, 2021

rhshadrach Mar 14, 2021

jbrockmendel Mar 29, 2021

MarcoGorelli Apr 4, 2021

rhshadrach commented Apr 15, 2021

sukriti1 commented Apr 16, 2021

MarcoGorelli commented Apr 25, 2021

rhshadrach commented Apr 27, 2021

MarcoGorelli commented Apr 27, 2021

rhshadrach commented Apr 29, 2021

MarcoGorelli commented Jun 12, 2021

	happen for extension array dtypes which have a map method
	happen for extension array dtypes, such as Categorical.

DOC: Updated convert_dtype of Series.apply #39941

DOC: Updated convert_dtype of Series.apply #39941

Conversation

sukriti1 commented Feb 20, 2021 • edited Loading

pep8speaks commented Feb 20, 2021 • edited Loading

Comment last updated at 2021-04-16 09:22:45 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli Feb 23, 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

MarcoGorelli commented Mar 1, 2021

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

mkp-gebensleben commented Mar 5, 2021 • edited Loading

MarcoGorelli commented Mar 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhshadrach commented Apr 15, 2021

sukriti1 commented Apr 16, 2021

MarcoGorelli commented Apr 25, 2021

rhshadrach commented Apr 27, 2021

MarcoGorelli commented Apr 27, 2021

rhshadrach commented Apr 29, 2021

MarcoGorelli commented Jun 12, 2021

sukriti1 commented Feb 20, 2021 •

edited

Loading

pep8speaks commented Feb 20, 2021 •

edited

Loading

MarcoGorelli Feb 23, 2021 •

edited

Loading

mkp-gebensleben commented Mar 5, 2021 •

edited

Loading