StringArray comparisions return BooleanArray #30231

TomAugspurger · 2019-12-12T15:14:37Z

TomAugspurger · 2019-12-12T15:16:19Z

pandas/core/ops/__init__.py

@@ -59,6 +59,37 @@
    rxor,
 )

+# -----------------------------------------------------------------------------
+# constants
+ARITHMETIC_BINOPS: Set[str] = {


@jbrockmendel am I reinventing any wheels here? This seems like something useful enough that it would have been done before.

i dont think so, no

doc/source/user_guide/text.rst

pandas/core/arrays/string_.py

pandas/tests/arrays/string_/test_string.py

…-boolean

TomAugspurger · 2019-12-12T16:00:01Z

Thanks! Fixed all those I think.

Tangentially related: we'll need a docs section discussing the behavior of pd.NA and arrays using pd.NA in comparison operations (propagate rather than unequal). I think it makes sense to combine user_guide/integer_na.rst and user_guide/boolean.rst into a single "nullable types" document which discusses all of this? What do you think?

jorisvandenbossche · 2019-12-12T16:03:56Z

I think it makes sense to combine user_guide/integer_na.rst and user_guide/boolean.rst into a single "nullable types" document which discusses all of this? What do you think?

I was floating the idea in my head (but didn't make an issue or so yet) to create a "user_guide/data_types" directory and move the integer and boolean files there (so an extra nesting in the left sidebar). And then the main "data types" page could have a short intro to extension dtypes, pointing to those pages.
But, if the content is relatively limited, it might also make sense to just combine both on a single page for now.

…-boolean

jreback

I don't think you are really testing this. I think you actually need to take some of our existing indexing tests, then paramterize over the existing object based strings and then StringArray, then test indexing. I suspect a fair amount of things will break.

pandas/core/ops/__init__.py

jorisvandenbossche · 2019-12-13T06:21:25Z

I don't think you are really testing this. I think you actually need to take some of our existing indexing tests,

This does not yet add indexing, this only changes the comparison operations. Indexing is one of the next steps on the list in #29556

jreback · 2019-12-13T13:51:03Z

I don't think you are really testing this. I think you actually need to take some of our existing indexing tests,

This does not yet add indexing, this only changes the comparison operations. Indexing is one of the next steps on the list in #29556

ok that's fair.

would at like to at least in this PR put into place an xfail test that assures that if we try to do indexing with a BooleanArray it fails

TomAugspurger · 2019-12-13T14:31:16Z

Indexing seems independent right? BooleanArray comparisons already return BooleanArray, and AFAIK those aren't tested in indexing yet.

I'm happy to write those tests / work on the implementation, but it feels like a separate piece of work.

jorisvandenbossche · 2019-12-13T14:51:41Z

Yes, let's keep it separate

jreback · 2019-12-13T15:09:55Z

Indexing seems independent right? BooleanArray comparisons already return BooleanArray, and AFAIK those aren't tested in indexing yet.

I'm happy to write those tests / work on the implementation, but it feels like a separate piece of work.

they are separate until sometries to assign a BooleanArray to a column and use it. We have nothing for this, certainly do in a separate PR. What I am suggesting is a single test to validate that this raises (for now)

TomAugspurger · 2019-12-13T15:27:56Z

Do we know what the defined behavior is yet? In #28778 it sounds like we're contemplating either raising or propagating.

jorisvandenbossche · 2019-12-14T07:56:59Z

doc/source/user_guide/text.rst

@@ -94,7 +94,12 @@ l. For ``StringDtype``, :ref:`string accessor methods<api.series.str>`
 2. Some string methods, like :meth:`Series.str.decode` are not available
   on ``StringArray`` because ``StringArray`` only holds strings, not
   bytes.
-
+3. In comparision operations, :class:`arrays.StringArray` and ``Series`` backed
+   by a ``StringArray`` will return an object with :class:`arrays.BooleanDtype`,


Suggested change

by a ``StringArray`` will return an object with :class:`arrays.BooleanDtype`,

by a ``StringArray`` will return an object with :class:`BooleanDtype`,

jreback · 2019-12-15T21:55:09Z

doc/source/user_guide/text.rst

+3. In comparision operations, :class:`arrays.StringArray` and ``Series`` backed
+   by a ``StringArray`` will return an object with :class:`arrays.BooleanDtype`,
+   rather than a ``bool`` dtype object, depending on whether
+   there are missing values. Missing values in a ``StringArray`` will propagate


is there a reason we are not always returning a BooleanArray, or am I mis-reading this? this would be very confusing if this is indeed the case.

I think it is stated confusingly. When the dtype is "string" (not object), comparisons always return BooleanDtype.
Tom, I think you can removed the ", depending on whether there are missing values"

…-boolean

WillAyd

generally lgtm

WillAyd · 2019-12-16T20:27:52Z

pandas/core/ops/__init__.py

@@ -59,6 +59,37 @@
    rxor,
 )

+# -----------------------------------------------------------------------------
+# constants
+ARITHMETIC_BINOPS: Set[str] = {


Just as FYI if you create a container with literal values mypy will infer that it is a Set[str] for you, so not required to specify. Have to specify if the container is empty on instantiation

Good to know!

TomAugspurger · 2019-12-17T17:26:20Z

This should be good to go.

jorisvandenbossche · 2019-12-18T08:30:33Z

Thanks!

xref pandas-dev#29556

TomAugspurger added 2 commits December 12, 2019 09:14

StringArray comparisions return BooleanArray

c548c67

xref pandas-dev#29556

update docs

f4ed269

TomAugspurger commented Dec 12, 2019

View reviewed changes

jorisvandenbossche reviewed Dec 12, 2019

View reviewed changes

doc/source/user_guide/text.rst Outdated Show resolved Hide resolved

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

pandas/tests/arrays/string_/test_string.py Outdated Show resolved Hide resolved

TomAugspurger added 2 commits December 12, 2019 09:57

fixups

240c6ee

Merge remote-tracking branch 'upstream/master' into string-comparison…

b9bd131

…-boolean

jorisvandenbossche mentioned this pull request Dec 12, 2019

Missing values proposal: concrete steps for 1.0 #29556

Closed

13 tasks

TomAugspurger added 2 commits December 12, 2019 11:40

Merge remote-tracking branch 'upstream/master' into string-comparison…

3528654

…-boolean

fixup

5a860e1

jreback added API Design Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Dec 12, 2019

jreback requested changes Dec 12, 2019

View reviewed changes

pandas/core/ops/__init__.py Show resolved Hide resolved

jorisvandenbossche approved these changes Dec 14, 2019

View reviewed changes

jreback requested changes Dec 15, 2019

View reviewed changes

TomAugspurger added 2 commits December 16, 2019 08:15

Merge remote-tracking branch 'upstream/master' into string-comparison…

cc22ff6

…-boolean

fix docs

e3b10ac

WillAyd reviewed Dec 16, 2019

View reviewed changes

TomAugspurger added the ExtensionArray Extending pandas with custom dtypes or arrays. label Dec 17, 2019

jorisvandenbossche merged commit 0badf06 into pandas-dev:master Dec 18, 2019

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

StringArray comparisions return BooleanArray (pandas-dev#30231)

ed1b5fe

xref pandas-dev#29556

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

StringArray comparisions return BooleanArray (pandas-dev#30231)

96b71d1

xref pandas-dev#29556

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StringArray comparisions return BooleanArray #30231

StringArray comparisions return BooleanArray #30231

TomAugspurger commented Dec 12, 2019

TomAugspurger Dec 12, 2019

jbrockmendel Dec 12, 2019

TomAugspurger commented Dec 12, 2019

jorisvandenbossche commented Dec 12, 2019 •

edited

Loading

jreback left a comment

jorisvandenbossche commented Dec 13, 2019

jreback commented Dec 13, 2019 •

edited

Loading

TomAugspurger commented Dec 13, 2019

jorisvandenbossche commented Dec 13, 2019

jreback commented Dec 13, 2019

TomAugspurger commented Dec 13, 2019

jorisvandenbossche Dec 14, 2019

jreback Dec 15, 2019

jorisvandenbossche Dec 16, 2019

TomAugspurger Dec 16, 2019

WillAyd left a comment

WillAyd Dec 16, 2019

TomAugspurger Dec 16, 2019

TomAugspurger commented Dec 17, 2019

jorisvandenbossche commented Dec 18, 2019

	by a ``StringArray`` will return an object with :class:`arrays.BooleanDtype`,
	by a ``StringArray`` will return an object with :class:`BooleanDtype`,

StringArray comparisions return BooleanArray #30231

StringArray comparisions return BooleanArray #30231

Conversation

TomAugspurger commented Dec 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Dec 12, 2019

jorisvandenbossche commented Dec 12, 2019 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

jorisvandenbossche commented Dec 13, 2019

jreback commented Dec 13, 2019 • edited Loading

TomAugspurger commented Dec 13, 2019

jorisvandenbossche commented Dec 13, 2019

jreback commented Dec 13, 2019

TomAugspurger commented Dec 13, 2019

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

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Dec 17, 2019

jorisvandenbossche commented Dec 18, 2019

jorisvandenbossche commented Dec 12, 2019 •

edited

Loading

jreback commented Dec 13, 2019 •

edited

Loading