Skip to content

API: referring to duplicate level name raises ValueError #21678

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 7 commits into from
Jul 8, 2018
Merged

API: referring to duplicate level name raises ValueError #21678

merged 7 commits into from
Jul 8, 2018

Conversation

jorisvandenbossche
Copy link
Member

follow-up on: #21423

But, this currently has a lot of failures, because of #21677 that we first need to resolve.

@pep8speaks
Copy link

pep8speaks commented Jun 29, 2018 •
edited
Loading

Hello @jorisvandenbossche! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 08, 2018 at 13:57 Hours UTC

@jorisvandenbossche jorisvandenbossche mentioned this pull request Jun 29, 2018
Merged
@jorisvandenbossche
Copy link
Member Author

Changed the implementation a bit to keep the fallback logic for integer names, to decouple it from the discussion in #21677

@codecov
Copy link

codecov bot commented Jun 29, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@1986dbe). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #21678   +/-   ##
=========================================
  Coverage          ?    91.9%           
=========================================
  Files             ?      160           
  Lines             ?    49894           
  Branches          ?        0           
=========================================
  Hits              ?    45854           
  Misses            ?     4040           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.28% <100%> (?)
#single 42.1% <75%> (?)
Impacted Files Coverage Δ
pandas/core/reshape/reshape.py 99.78% <ø> (ø)
pandas/core/indexes/multi.py 95.04% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1986dbe...450c6bd. Read the comment docs.

@jorisvandenbossche jorisvandenbossche added this to the 0.24.0 milestone Jun 29, 2018
jreback
jreback requested changes Jul 1, 2018
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.

wow much simpler

minor comment - pls add whatsnew note

any downsides to doing this?

pandas/core/indexes/multi.py Outdated
@@ -755,11 +755,11 @@ def _from_elements(values, labels=None, levels=None, names=None,
return MultiIndex(levels, labels, names, sortorder=sortorder)

def _get_level_number(self, level):
count = self.names.count(level)
if (count > 1) and not isinstance(level, int):
Copy link
Contributor

Choose a reason for hiding this comment

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

use is_integer here

@jorisvandenbossche
Copy link
Member Author

wow much simpler

I don't think the code is simpler, the diff is just smaller :-)

any downsides to doing this?

I don't think so.
It's an api change, but only a minor one I think. In principle we could also keep it as KeyError (I don't think there is necessarily a more correct one, although KeyError is typically used for non-existing key, while here the key is existing but duplicate), but then encountering this in eg stack would change from ValueError to KeyError (so a api change as well).

jreback
jreback requested changes Jul 8, 2018
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.

minor comment

doc/source/whatsnew/v0.24.0.txt Outdated
@@ -229,6 +229,9 @@ Other API Changes
^^^^^^^^^^^^^^^^^

- :class:`DatetimeIndex` now accepts :class:`Int64Index` arguments as epoch timestamps (:issue:`20997`)
- Accessing a level of a ``MultiIndex`` with a duplicate name (e.g. in
:meth:~MultiIndex.get_level_values) now raises a ValueError instead of
Copy link
Contributor

Choose a reason for hiding this comment

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

double backticks around the errors

@jorisvandenbossche jorisvandenbossche merged commit 13febab into pandas-dev:master Jul 8, 2018
@jorisvandenbossche jorisvandenbossche deleted the api-duplicate-level-erro branch July 8, 2018 14:16
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
@jorisvandenbossche
API: referring to duplicate level name raises ValueError (pandas-dev#…
1f271fc 
…21678)
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

Assignees
No one assigned
Labels
API Design MultiIndex
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jorisvandenbossche @pep8speaks @jreback