Skip to content

BUG: fix astype conversion string -> float #37974

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

Jump to bottom
Merged
merged 11 commits into from
Nov 29, 2020
Merged

BUG: fix astype conversion string -> float #37974

merged 11 commits into from
Nov 29, 2020

Conversation

mlondschien
Copy link
Contributor

@mlondschien mlondschien commented Nov 20, 2020 •
edited
Loading

Fixes the string -> float conversion as discussed in #37626 in presence of pd.NA. I used the same approach as used in string -> Int64. Note that differently to the Int64 case, we return a numpy.ndarray, not a pd.array.

I added a test that compares pd.Series. Replacing this with pd.array the tm.assert_numpy_array_equal assertion will raise, as expected is of type FloatingArray. IIUC this is a new feature of pandas 1.2.0. Should we return this in .astype("float") by default?

@jreback jreback added the NA - MaskedArrays Related to pd.NA and nullable extension arrays label Nov 20, 2020
jreback
jreback requested changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

pls add a whatsnew note as well (I don't think this actually closes the original as you mentioned many cases, but that's ok)

pandas/tests/arrays/string_/test_string.py Outdated
@@ -226,6 +226,14 @@ def test_astype_int():
tm.assert_extension_array_equal(result, expected)


def test_astype_float():
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use the nulls_fixture (though may have to skip for pd.NaT)

Copy link
Contributor Author

@mlondschien mlondschien Nov 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can you expand what for? IIUC pd.Series(["1.1", null], dtype="string") should be equal for null in pd.NA, np.nan, pd.NaT (works), None. This behaviour should be tested somewhere else and adding the fixture would just result in repeating the same test four times.

ivanovmg
ivanovmg reviewed Nov 22, 2020
View reviewed changes
Copy link
Member

@ivanovmg ivanovmg left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!
Please see the comments below.

Would you consider fixing the similar issue with astype("int") conversion as well in a separate PR?

@mlondschien
Copy link
Contributor Author

Sure.

Would you consider fixing the similar issue with astype("int") conversion as well in a separate PR?

Can you expand on the issue and, if not obvious, with the expected behaviour?

@ivanovmg
Copy link
Member

Sure.

Would you consider fixing the similar issue with astype("int") conversion as well in a separate PR?

Can you expand on the issue and, if not obvious, with the expected behaviour?

Similar to float.

In [1]: import pandas as pd
In [2]: ser = pd.Series([1, pd.NA, 2], dtype="string")
In [3]: ser.astype("int")
...
TypeError: int() argument must be a string, a bytes-like object or a number, not 'NAType'

@mlondschien
Copy link
Contributor Author

mlondschien commented Nov 22, 2020 •
edited
Loading

In [1]: import pandas as pd
In [2]: ser = pd.Series([1, pd.NA, 2], dtype="string")
In [3]: ser.astype("int")
...
TypeError: int() argument must be a string, a bytes-like object or a number, not 'NAType'

Hmm. As far as I understood, int (the non-extension array dtype) does not support missings. See:

In [1]: import numpy as np
In [2]: import pandas as pd
In [3]: pd.Series([1.0, np.nan], dtype="float").astype("int")
ValueError: Cannot convert non-finite values (NA or inf) to integer

However, using the nullable extension array dtype Int64, the conversion works:

In [4]: pd.Series(["1", pd.NA], dtype="string").astype("Int64")
Out[4]: 
0       1
1    <NA>
dtype: Int64

Would you prefer if above (string with pd.NA to int) raised a ValueError instead?

Edit: Were you possibly referring toobject -> Int64 as in pd.Series(["1", "2"]).astype("Int64")?

Unrelated: Any idea what's up with the failing tests?

jreback
jreback requested changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

ping on green

@mlondschien
Copy link
Contributor Author

@jreback Could you have another look?

@mlondschien mlondschien requested a review from jreback November 25, 2020 07:46
@jreback jreback added this to the 1.2 milestone Nov 26, 2020
jreback
jreback approved these changes Nov 26, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm. @jorisvandenbossche if any comments.

@jreback jreback merged commit 8eca4b7 into pandas-dev:master Nov 29, 2020
@jreback
Copy link
Contributor

jreback commented Nov 29, 2020

thanks @mlondschien

@mlondschien mlondschien deleted the issue-37626 branch November 29, 2020 19:04
@mlondschien mlondschien mentioned this pull request Jan 4, 2021
Open
3 tasks
@simonjayhawkins simonjayhawkins mentioned this pull request Mar 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

@ivanovmg ivanovmg Awaiting requested review from ivanovmg

Assignees
No one assigned
Labels
NA - MaskedArrays Related to pd.NA and nullable extension arrays
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.
3 participants
@mlondschien @ivanovmg @jreback