Skip to content

BUG: Set dtypes of new columns when stacking (#36991) #40127

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 17 commits into from
Mar 10, 2021
Merged

BUG: Set dtypes of new columns when stacking (#36991) #40127

merged 17 commits into from
Mar 10, 2021

Conversation

maroth96
Copy link
Contributor

@maroth96 maroth96 commented Feb 28, 2021 •
edited
Loading

@pep8speaks
Copy link

pep8speaks commented Feb 28, 2021 •
edited
Loading

Hello @maroth96! 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-03-10 01:29:17 UTC

jreback
jreback requested changes Feb 28, 2021
View reviewed changes
@maroth96 maroth96 requested a review from jreback February 28, 2021 21:47
jreback
jreback requested changes Mar 1, 2021
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.

can you add a whatsnew note. 1.3 bug fixes under reshaping is good.

pandas/core/reshape/reshape.py Outdated
new_columns = MultiIndex.from_arrays(
[
Index(new_level, dtype=level.dtype)
if None not in new_level
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have any tests that hit the None case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, specifically test_stack_nan_in_multiindex_columns.

None is a tricky case---it is allowed for some Index types (like CategoricalIndex) but not others (like Int64Index). I am just avoiding levels with None entirely.

I've also looked at the way None is handled elsewhere, and it's not totally consistent:

>>> MultiIndex.from_arrays([[1, 2, None]]).levels[0]
Int64Index([1, 2], dtype='int64')

But:

>>> Index([1, 2, None])
Index([1, 2, None], dtype='object')

Perhaps in the future it would be better if Index accepted None values, even when a dtype is specified. Then Index([1, 2, None], dtype='int64') would return Int64Index([1, 2], dtype='int64'). Then we wouldn't need such a condition here. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm interesting, ok can you open an issue specifically showing the MI vs Index cases. I agree we should do something about this. ok for here on this PR.

@jreback jreback added Bug Dtype Conversions Unexpected or buggy dtype conversions Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Mar 1, 2021
@jreback jreback added this to the 1.3 milestone Mar 1, 2021
@maroth96 maroth96 requested a review from jreback March 2, 2021 01:31
jreback
jreback requested changes Mar 3, 2021
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.

@jbrockmendel if you can have a look here

pandas/core/reshape/reshape.py Outdated
new_columns = MultiIndex.from_arrays(
[
Index(new_level, dtype=level.dtype)
if None not in new_level
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm interesting, ok can you open an issue specifically showing the MI vs Index cases. I agree we should do something about this. ok for here on this PR.

@maroth96 maroth96 requested a review from jreback March 3, 2021 07:10
@maroth96
Copy link
Contributor Author

maroth96 commented Mar 3, 2021

I extracted a function for stacking the column index. Let me know if you prefer it inlined. Otherwise, the same is possible for the index (lines 714--732).

jbrockmendel
jbrockmendel reviewed Mar 3, 2021
View reviewed changes
jbrockmendel
jbrockmendel reviewed Mar 3, 2021
View reviewed changes
@maroth96 maroth96 requested a review from jbrockmendel March 3, 2021 20:42
jbrockmendel
jbrockmendel reviewed Mar 6, 2021
View reviewed changes
jbrockmendel
jbrockmendel reviewed Mar 6, 2021
View reviewed changes
jreback
jreback requested changes Mar 8, 2021
View reviewed changes
@maroth96 maroth96 requested a review from jreback March 9, 2021 04:35
jreback
jreback requested changes Mar 9, 2021
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.

looks good some comments

@maroth96 maroth96 requested a review from jreback March 10, 2021 01:29
@jreback jreback merged commit 7b5957f into pandas-dev:master Mar 10, 2021
@jreback
Copy link
Contributor

jreback commented Mar 10, 2021

thanks @maroth96 very nice! keep em coming!

jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Mar 11, 2021
@maroth96 @jbrockmendel
BUG: Set dtypes of new columns when stacking (pandas-dev#36991) (pand…
a9935f1 
…as-dev#40127)
@maroth96 maroth96 mentioned this pull request Mar 11, 2021
Open
3 tasks
@maroth96 maroth96 deleted the b-36991 branch March 13, 2021 07:15
This was referenced Feb 1, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Bug Dtype Conversions Unexpected or buggy dtype conversions Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

BUG: MultiIndex loses category after .stack()
4 participants
@maroth96 @pep8speaks @jreback @jbrockmendel