Skip to content

Infer names in MultiIndex.from_product #27895

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

Closed
wants to merge 6 commits into from
Closed

Infer names in MultiIndex.from_product #27895

wants to merge 6 commits into from

Conversation

christopherzimmerman
Copy link
Contributor

@christopherzimmerman christopherzimmerman commented Aug 13, 2019

Copy link
Member

@jschendel jschendel left a comment

Choose a reason for hiding this comment

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

Thanks! A few comments but looks good overall.

@jschendel jschendel added this to the 1.0 milestone Aug 13, 2019
Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

To disable this, I now have to do

from_product(iterables, names=[None] * len(iterables)

Are we OK with that? Or do we want to allow names=None to continue to mean no names? If so, we'll need a different sentinel value that None (no_default = object())

@@ -31,7 +31,7 @@ Other enhancements

.. _whatsnew_1000.api_breaking:

-
- :meth:`MultiIndex.from_product` infers level names from inputs if possible (:issue:`27292`)
Copy link
Contributor

Choose a reason for hiding this comment

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

But this above the .. _whatsnew_1000.api_breaking: line.

And maybe say "when not explicitly provided" rather than "if possible".

Copy link
Member

Choose a reason for hiding this comment

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

Put this above the .. _whatsnew_1000.api_breaking: line.

It looks like a few of the section reference names are slightly misaligned with ticks throughout the whatsnew:

Enhancements
~~~~~~~~~~~~

.. _whatsnew_1000.enhancements.other:

-
-

Other enhancements
^^^^^^^^^^^^^^^^^^

.. _whatsnew_1000.api_breaking:

-
-

Backwards incompatible API changes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. _whatsnew_1000.api.other:

- :class:`pandas.core.groupby.GroupBy.transform` now raises on invalid operation names (:issue:`27489`).

I'll open up a quick PR to fix it across the board. Might result in a merge conflict here but should be straightforward to resolve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change to the text in whatsnew, holding off on changing anything else if jschendel is going to be changing other parts of the file.

Copy link
Member

Choose a reason for hiding this comment

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

I just merged my changes to the whatsnew, so you should be good to make the additional changes now. Should just need to address the merge conflict by making sure your entry is above the .. _whatsnew_1000.api_breaking: line.

@christopherzimmerman
Copy link
Contributor Author

christopherzimmerman commented Aug 14, 2019

@TomAugspurger in regards to your first comment, the reason I opened an issue about this in the first place was due to the addition of from_frame, which does infer names from the Series inputs, so it seemed like an inconsistency. If that behavior usually isn't wanted/needed in from_product, then maybe this PR isn't necessary.

@jreback
Copy link
Contributor

jreback commented Aug 15, 2019

you would also have to change from_arrays for the same reason. I think if you did that would be ok with this (ex using @TomAugspurger suggestion, that an explicit names=None works)

@TomAugspurger
Copy link
Contributor

@christopherzimmerman do you have time to make names=None mean "no names for any levels"?

The default value will be something like

_no_default = object()

def from_product(self, ..., names=_no_default, ...):
    if names is None or names is _no_default:
        names = [None] * nlevels

@christopherzimmerman
Copy link
Contributor Author

christopherzimmerman commented Sep 6, 2019

@TomAugspurger sorry for the inactivity, I'll make those changes. I believe I messed up the commit history for this PR when I rebased from master (which is resulting in a check failing), and have a lot of junk commits included, I may close this PR and open another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infer names in MultiIndex.from_product if inputs have a name attribute
4 participants