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

(Assuming we don't want to disallow 1-level MultiIndexes), stop identifying MultiIndex by nlevels == 1 #21149

Closed
toobaz opened this Issue May 21, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@toobaz
Member

toobaz commented May 21, 2018

Code Sample, a copy-pastable example if possible

In [2]: mi = pd.MultiIndex.from_product([[0, 1]])

In [3]: mi.set_names(('first',), level=0)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-3-1f4ad6c7f0bf> in <module>()
----> 1 mi.set_names(('first',), level=0)

/home/nobackup/repo/pandas/pandas/core/indexes/base.py in set_names(self, names, level, inplace)
   1386 
   1387         if level is not None and self.nlevels == 1:
-> 1388             raise ValueError('Level must be None for non-MultiIndex')
   1389 
   1390         if level is not None and not is_list_like(level) and is_list_like(

ValueError: Level must be None for non-MultiIndex

Problem description

The case of MultiIndex with self.nlevels == 1 is currently perfectly valid (despite e.g. MultiIndex.droplevel returning a normal Index when only one level is left). Unless we want to change this, we should always identify a MultiIndex with isinstance, not by looking at self.nlevels.

Expected Output

In [4]: mi.set_names(('first',))
Out[4]: 
MultiIndex(levels=[[0, 1]],
           labels=[[0, 1]],
           names=['first'])

Output of pd.show_versions()

INSTALLED VERSIONS

commit: 5cf4957
python: 3.5.3.final.0
python-bits: 64
OS: Linux
OS-release: 4.9.0-6-amd64
machine: x86_64
processor:
byteorder: little
LC_ALL: None
LANG: it_IT.UTF-8
LOCALE: it_IT.UTF-8

pandas: 0.24.0.dev0+14.g5cf4957a2.dirty
pytest: 3.5.0
pip: 9.0.1
setuptools: 39.0.1
Cython: 0.25.2
numpy: 1.14.3
scipy: 0.19.0
pyarrow: None
xarray: None
IPython: 6.2.1
sphinx: 1.5.6
patsy: 0.5.0
dateutil: 2.7.3
pytz: 2018.4
blosc: None
bottleneck: 1.2.0dev
tables: 3.3.0
numexpr: 2.6.1
feather: 0.3.1
matplotlib: 2.0.0
openpyxl: 2.3.0
xlrd: 1.0.0
xlwt: 1.3.0
xlsxwriter: 0.9.6
lxml: 4.1.1
bs4: 4.5.3
html5lib: 0.999999999
sqlalchemy: 1.0.15
pymysql: None
psycopg2: None
jinja2: 2.10
s3fs: None
fastparquet: None
pandas_gbq: None
pandas_datareader: 0.2.1

@toobaz

This comment has been minimized.

Member

toobaz commented May 21, 2018

Another way to see it is: a 1-level MultiIndex and a flat index should behave the same way 99% of time (there can be exceptions), and hence the test which is raising a ValueError above has no reason to be there.

@jreback

This comment has been minimized.

Contributor

jreback commented May 21, 2018

yes this would be nice to fix

@KalyanGokhale

This comment has been minimized.

Contributor

KalyanGokhale commented May 22, 2018

@toobaz can I submit the suggested fix? unless you were planning to incorporate in another PR...

@toobaz

This comment has been minimized.

Member

toobaz commented May 22, 2018

@toobaz can I submit the suggested fix? unless you were planning to incorporate in another PR...

You're welcome to go on... but there are in total three checks of self.nlevels == 1 or self.nlevels > 1 in the same file, it could be great if you could take care of all of them together (or verify they don't need any change).

@KalyanGokhale

This comment has been minimized.

Contributor

KalyanGokhale commented May 23, 2018

Sure - of two self.nlevels statement in def set_names, at one place change is needed and at the other its not needed - and I have made the fix.

For one self.nlevels statement in def cmp_method in the same file pandas/core/indexes/base.py, I am evaluating a few test cases - and will submit a single PR once done.

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