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

Add defaults to swaplevel() parameters i and j #12943

Closed
wants to merge 1 commit into from

Conversation

brandon-rhodes
Copy link
Contributor

Closes #12934 feature request.

@jreback
Copy link
Contributor

jreback commented Apr 21, 2016

@brandon-rhodes I just updated master, so you will need to rebase as unrelated issue with python-dateutil will cause a test for fail :< (we are now skipping)

@jreback jreback added Reshaping Concat, Merge/Join, Stack/Unstack, Explode API Design labels Apr 21, 2016
@@ -131,6 +131,8 @@ These changes conform sparse handling to return the correct types and work to ma

API changes
~~~~~~~~~~~
- ``.swaplevel()`` for ``Series``, ``DataFrame``, ``Panel``, and ``MultiIndex`` now features defaults for its first two parameters ``i`` and ``j`` that swap the two innermost levels of the index.
Copy link
Contributor

Choose a reason for hiding this comment

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

add the issue number at the end, don't add a blank line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make sure I understand: are you saying that (a) even though the subsequent bullet points are separated by blank lines, the blank line beneath this bullet point should disappear? or (b) that when I go in to the code to add the issue number, I should keep it on the same line and not let it land by accident on a subsequent line?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I just meant no need to add the line in between. its doesn't matter much as sphinx will ignore it, but just trying to be consistent (though I think there are some extra lines there anyhow....)

@jreback
Copy link
Contributor

jreback commented Apr 21, 2016

did u mean to close this?

@brandon-rhodes
Copy link
Contributor Author

No. I am used to the Phabricator work flow, and this pull request is teaching me the GitHub workflow :) Stand by!

@brandon-rhodes
Copy link
Contributor Author

@jreback Okay, I pulled and pushed some stuff, and all the code reappeared. :) I added the issue number, and rebased against your latest master — let me know how things look!

@jreback
Copy link
Contributor

jreback commented Apr 22, 2016

can u update tests for MultiIndex as well

@jreback
Copy link
Contributor

jreback commented Apr 22, 2016

pls squash as well

@@ -3384,7 +3384,7 @@ def nsmallest(self, n, columns, keep='first'):
"""
return self._nsorted(columns, n, 'nsmallest', keep)

def swaplevel(self, i, j, axis=0):
def swaplevel(self, i=-2, j=-1, axis=0):
"""
Swap levels i and j in a MultiIndex on a particular axis
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth noting in the doc strings (of alll of these) that the defaults were added in 0.18.1

you can use a versionadded directive

@brandon-rhodes
Copy link
Contributor Author

Oh — GitHub doesn't let you squash as you land the pull request? Yet another difference with the workflow I'm used to. Yes, I'll do the squashing myself, and also try addressing your two comments — thanks!

@jreback
Copy link
Contributor

jreback commented Apr 22, 2016

yes I will squash on merge - since we rebase its usually a bit easier to use logical commits and squash

@brandon-rhodes
Copy link
Contributor Author

Just saw your comment — had already squashed on my end :) Per the Sphinx docs, I went with versionchanged instead, since it seemed closer to the intent of a change like this. I also found the only test I could find where the multiindex tests used swaplevel and supplemented it. Let me know what you think! (Also: wow, your integration tests take a while!)

@jreback
Copy link
Contributor

jreback commented Apr 22, 2016

@shoyer
Copy link
Member

shoyer commented Apr 22, 2016

This seems reasonable enough to me. 👍

@TomAugspurger
Copy link
Contributor

👍

@jreback jreback closed this in ed324e8 Apr 22, 2016
@jreback
Copy link
Contributor

jreback commented Apr 22, 2016

thanks @brandon-rhodes

@brandon-rhodes
Copy link
Contributor Author

Thanks for considering this contribution from me! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Design Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants