Skip to content
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.

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

CLN: remove internal usage of integer_array() #38289

Merged
merged 3 commits into from
Dec 11, 2020

Conversation

jorisvandenbossche
Copy link
Member

The integer_array() function stems from a time that pd.array(..) didn't exist yet. But since now pd.array can do this, IMO we shouldn't be using that outside of the integer module (eg I didn't add a floating_array(..)).

Only in tests/arrays/integer/tst_construction.py I still left the usage of integer_array() as is (but probably could also change it there). Could in principle remove the function altogether, since it is just a small wrapper around the private arrays.integer.coerce_to_array

@jorisvandenbossche jorisvandenbossche added Clean NA - MaskedArrays Related to pd.NA and nullable extension arrays labels Dec 4, 2020
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.

comment + need a whatsnew to mention that we are removing (as experimental totally fine just to do)

from pandas.core.arrays import IntegerArray, integer_array
from pandas.core.arrays.integer import Int8Dtype, Int32Dtype, Int64Dtype
from pandas.core.arrays import IntegerArray
from pandas.core.arrays.integer import Int8Dtype, Int32Dtype, Int64Dtype, integer_array
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

shouldn't this be removed?

see the top post, this is the only file were I for now kept integer_array (it's used a lot, and it is specifically testing the behaviour of the construction implemented by it). But just changed the import, because I removed it from pandas.core.arrays
Now, as said above, I can further remove it in this file as well (can do that here, or later in another PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

i would completely remove it in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I removed any remaining usage of integer_array in this file, so could also remove the actual function.

I replaced it with a mixture of pd.array and IntegerArray._from_sequence (and sometimes a parametrized fixture to test both), since they are not exactly equal. For example, with _from_sequence you can also pass it a float array without specifying a dtype, something you can't replicate by only testing pd.array(..)

@jreback jreback added this to the 1.2 milestone Dec 4, 2020
@jorisvandenbossche
Copy link
Member Author

need a whatsnew to mention that we are removing

This is not public, so no need for a whatnew I think? (I am only removing internal usage, there was no public exposure of this)

@jreback
Copy link
Contributor

jreback commented Dec 4, 2020

need a whatsnew to mention that we are removing

This is not public, so no need for a whatnew I think? (I am only removing internal usage, there was no public exposure of this)

i diagree this has been exposed for a while (in pandas.core.arrays), which is public-ish. i am ok with just removing it, but need to have a note.

@jorisvandenbossche
Copy link
Member Author

pandas.core.arrays is not public. The array classes are exposed, but in pandas.arrays

@jorisvandenbossche
Copy link
Member Author

(anyway, I won't further argue against adding a note if you insist (it's less work to write that sentence than writing this comment ;)), but just want to be clear this is something completely internal)

@jreback
Copy link
Contributor

jreback commented Dec 5, 2020

(anyway, I won't further argue against adding a note if you insist (it's less work to write that sentence than writing this comment ;)), but just want to be clear this is something completely internal)

thanks, i would also completely remove the usage of this as noted above.

@jorisvandenbossche
Copy link
Member Author

Yes, will update test_construction.py to stop using it as well

@jorisvandenbossche jorisvandenbossche removed this from the 1.2 milestone Dec 8, 2020
@jreback
Copy link
Contributor

jreback commented Dec 8, 2020

@jorisvandenbossche i think this is worth putting on 1.2. ping when green.

@jreback jreback added this to the 1.2 milestone Dec 8, 2020
@jbrockmendel
Copy link
Member

can you merge master

@jorisvandenbossche
Copy link
Member Author

i think this is worth putting on 1.2.

Is there a reason to include it for 1.2? (since it's only an internal clean-up)

@jreback
Copy link
Contributor

jreback commented Dec 11, 2020

i think this is worth putting on 1.2.

Is there a reason to include it for 1.2? (since it's only an internal clean-up)

no this is fine for 1.3. thanks

@jreback jreback modified the milestones: 1.2, 1.3 Dec 11, 2020
@jreback jreback merged commit 4b022e6 into pandas-dev:master Dec 11, 2020
@jorisvandenbossche jorisvandenbossche deleted the remove-integer-array branch December 12, 2020 08:50
luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean NA - MaskedArrays Related to pd.NA and nullable extension arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants