Skip to content

ENH: Implement _from_sequence_of_strings for BooleanArray #31159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
Jan 23, 2020
Merged

ENH: Implement _from_sequence_of_strings for BooleanArray #31159

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f7fdf45
Add test
Jan 20, 2020
9e57350
Implement _from_sequence_of_strings
Jan 20, 2020
eb591cd
Add read_csv test
Jan 20, 2020
87ac09b
Add to release note
Jan 20, 2020
51b6ac9
Check for pd.NA
Jan 20, 2020
6ffb018
Update tests
Jan 20, 2020
9ddc5b1
Blacken
Jan 20, 2020
4dd64b8
Type arguments
Jan 20, 2020
d28a6de
Use optional type
Jan 20, 2020
978f22d
Don't import io
Jan 20, 2020
19f9c18
Don't type dtype
Jan 20, 2020
f851b83
Nit
Jan 21, 2020
29dbfa1
Don't import Optional
Jan 21, 2020
0481153
Change Boolean strings
Jan 21, 2020
be0731b
Parametrize test over true / false strings
Jan 21, 2020
fdec55d
Remove test parameterization
Jan 21, 2020
e2656cb
Fix linting
Jan 22, 2020
6d06b84
Take out assertions
Jan 23, 2020
604b862
Take out parameterization
Jan 23, 2020
f2db6ed
Blacken
Jan 23, 2020
9e35b63
Update pandas/tests/arrays/test_boolean.py
dsaxton Jan 23, 2020
71bafbf
Update tests and raise invalid string error
Jan 23, 2020
184a9be
Fix test
Jan 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.0.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ type dedicated to boolean data that can hold missing values. The default
``bool`` data type based on a bool-dtype NumPy array, the column can only hold
``True`` or ``False``, and not missing values. This new :class:`~arrays.BooleanArray`
can store missing values as well by keeping track of this in a separate mask.
(:issue:`29555`, :issue:`30095`)
(:issue:`29555`, :issue:`30095`, :issue:`31131`)

.. ipython:: python

Expand Down
19 changes: 18 additions & 1 deletion pandas/core/arrays/boolean.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import numbers
from typing import TYPE_CHECKING, Any, Tuple, Type
from typing import TYPE_CHECKING, Any, List, Tuple, Type
import warnings

import numpy as np
Expand Down Expand Up @@ -286,6 +286,23 @@ def _from_sequence(cls, scalars, dtype=None, copy: bool = False):
values, mask = coerce_to_array(scalars, copy=copy)
return BooleanArray(values, mask)

@classmethod
def _from_sequence_of_strings(
cls, strings: List[str], dtype=None, copy: bool = False
):
def map_string(s):
if isna(s):
return s
elif s in ["True", "TRUE", "true"]:
return True
elif s in ["False", "FALSE", "false"]:
return False
else:
return s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point, we should probably make a more performant implementation for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, any thoughts on what such an implementation would look like?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, perf isn't worth worrying about too much until we can push the actual parsing down to the C reader. As is, we've already built up a list of Python strings in memory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually not too hard to do in parsers.pyx; this is generically handled in python code rather than in cython.


scalars = [map_string(x) for x in strings]
return cls._from_sequence(scalars, dtype, copy)

def _values_for_factorize(self) -> Tuple[np.ndarray, Any]:
data = self._data.astype("int8")
data[self._mask] = -1
Expand Down
10 changes: 10 additions & 0 deletions pandas/tests/arrays/test_boolean.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,16 @@ def test_coerce_to_numpy_array():
np.array(arr, dtype="bool")


@pytest.mark.parametrize("na_value", [None, np.nan, pd.NA])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a need to parametrize over de na values. The parser always gives data with NaNs

def test_to_boolean_array_from_strings(na_value):
result = BooleanArray._from_sequence_of_strings(["True", "False", na_value])
expected = BooleanArray(
np.array([True, False, False]), np.array([False, False, True])
)

tm.assert_extension_array_equal(result, expected)


def test_repr():
df = pd.DataFrame({"A": pd.array([True, False, None], dtype="boolean")})
expected = " A\n0 True\n1 False\n2 <NA>"
Expand Down
18 changes: 18 additions & 0 deletions pandas/tests/io/parser/test_dtypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,3 +550,21 @@ def test_numeric_dtype(all_parsers, dtype):

result = parser.read_csv(StringIO(data), header=None, dtype=dtype)
tm.assert_frame_equal(expected, result)


@pytest.mark.parametrize("na_string", ["NaN", "nan", "NA", "null", "NULL", ""])
@pytest.mark.parametrize("true_string", ["True", "TRUE", "true"])
@pytest.mark.parametrize("false_string", ["False", "FALSE", "false"])
def test_boolean_dtype(all_parsers, na_string, true_string, false_string):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parametrization generates a ton of test cases which increases pytest collection / run time. Can you inline all of them in a single test?

data = "\n".join(["a", "true", "TRUE", "True", "false", "FALSE", "False", "NaN", "nan", ...])

and then make an assert?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also test with a "wrong" string value to ensure a proper error message is raised?

Copy link
Member Author

@dsaxton dsaxton Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes a lot more sense than what I was doing. Regarding the assert, something like

assert all([s in data for s in ["True", "TRUE", "true"]])
assert all([s in data for s in ["False", "FALSE", "false"]])
assert all([s in data for s in ["NaN", "nan", "NA", "null", "NULL"]])

(separating them out to make a failure more meaningful / make it a little easier to read)? I dropped the empty string for now because it seems to get ignored in a single column CSV.

parser = all_parsers
data = f"a,b\n{true_string},{false_string}\nTrue,{na_string}\n"

result = parser.read_csv(StringIO(data), dtype="boolean")
expected = pd.DataFrame(
{
"a": pd.array([True, True], dtype="boolean"),
"b": pd.array([False, None], dtype="boolean"),
}
)

tm.assert_frame_equal(result, expected)