Skip to content

Conversation

skvrahul
Copy link
Contributor

@skvrahul skvrahul commented Dec 13, 2020

@simonjayhawkins simonjayhawkins added this to the 1.2.1 milestone Dec 13, 2020
@simonjayhawkins
Copy link
Member

Thanks @skvrahul for the PR. please add a test.

@simonjayhawkins simonjayhawkins added Bug Output-Formatting __repr__ of pandas objects, to_string labels Dec 13, 2020
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.

this issue needs to confirm / add any tests that we are missing

else:

if n > max_seq_items:
if max_seq_items == 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also cover in a test the max_seq_items == 0 case (I think we have this but maybe not). Also pls confirm that we have a test for n == max_seq_items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we don't have a test for n == max_seq_items. Will cover these in my tests.

Just wanted to confirm that this is the relevant location for adding these?

def test_repr(self, idx):
result = idx[:1].__repr__()
expected = """\
MultiIndex([('foo', 'one')],
names=['first', 'second'])"""
assert result == expected
result = idx.__repr__()
expected = """\
MultiIndex([('foo', 'one'),
('foo', 'two'),
('bar', 'one'),
('baz', 'two'),
('qux', 'one'),
('qux', 'two')],
names=['first', 'second'])"""
assert result == expected
with pd.option_context("display.max_seq_items", 5):
result = idx.__repr__()
expected = """\
MultiIndex([('foo', 'one'),
('foo', 'two'),
...
('qux', 'one'),
('qux', 'two')],
names=['first', 'second'], length=6)"""
assert result == expected

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah there plus some generic Index ones (look in pandas/tests/indexes/); you may have to grep around a bit

e.g.

~/pandas-dev$ grep -r max_seq_items pandas/tests/
pandas/tests/io/formats/test_format.py:        with option_context("display.max_seq_items", 2000):
pandas/tests/io/formats/test_format.py:        with option_context("display.max_seq_items", 5):
pandas/tests/groupby/test_groupby.py:    "max_seq_items, expected",
pandas/tests/groupby/test_groupby.py:def test_groups_repr_truncates(max_seq_items, expected):
pandas/tests/groupby/test_groupby.py:    with pd.option_context("display.max_seq_items", max_seq_items):
pandas/tests/indexes/multi/test_formats.py:    with pd.option_context("display.max_seq_items", None):
pandas/tests/indexes/multi/test_formats.py:        with pd.option_context("display.max_seq_items", 5):
pandas/tests/indexes/common.py:        with pd.option_context("display.max_seq_items", None):
pandas/tests/indexes/base_class/test_formats.py:        with cf.option_context("display.max_seq_items", 10):

i think pandas/tests/indexes/base_class/test_formats.py: with cf.option_context("display.max_seq_items", 10):
has a number of tests as well

@skvrahul
Copy link
Contributor Author

@jreback @simonjayhawkins
Could you confirm that this parameter is expected to affect even the list of names of columns in the index
I.e. repr evaluates to:

MultiIndex([...
            ('qux', 'two')],
           names=['first', ...], length=6)

and not

MultiIndex([...
            ('qux', 'two')],
           names=['first', 'second'], length=6)

@skvrahul
Copy link
Contributor Author

Could you confirm that this parameter is expected to affect even the list of names of columns in the index
I.e. repr evaluates to:

MultiIndex([...
            ('qux', 'two')],
           names=['first', ...], length=6)

and not

MultiIndex([...
            ('qux', 'two')],
           names=['first', 'second'], length=6)

Checked on master and can confirm this (parameter affects even the list of column names)
Added tests accordingly.

@skvrahul skvrahul requested a review from jreback December 14, 2020 19:05
else:

if n > max_seq_items:
if max_seq_items == 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah there plus some generic Index ones (look in pandas/tests/indexes/); you may have to grep around a bit

e.g.

~/pandas-dev$ grep -r max_seq_items pandas/tests/
pandas/tests/io/formats/test_format.py:        with option_context("display.max_seq_items", 2000):
pandas/tests/io/formats/test_format.py:        with option_context("display.max_seq_items", 5):
pandas/tests/groupby/test_groupby.py:    "max_seq_items, expected",
pandas/tests/groupby/test_groupby.py:def test_groups_repr_truncates(max_seq_items, expected):
pandas/tests/groupby/test_groupby.py:    with pd.option_context("display.max_seq_items", max_seq_items):
pandas/tests/indexes/multi/test_formats.py:    with pd.option_context("display.max_seq_items", None):
pandas/tests/indexes/multi/test_formats.py:        with pd.option_context("display.max_seq_items", 5):
pandas/tests/indexes/common.py:        with pd.option_context("display.max_seq_items", None):
pandas/tests/indexes/base_class/test_formats.py:        with cf.option_context("display.max_seq_items", 10):

i think pandas/tests/indexes/base_class/test_formats.py: with cf.option_context("display.max_seq_items", 10):
has a number of tests as well

names=['first', 'second'], length=6)"""
assert result == expected

# display.max_seq_items == n
Copy link
Contributor

Choose a reason for hiding this comment

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

prob better to make a new test as easier to see / run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Moved this to it's own test case

@skvrahul skvrahul requested a review from jreback December 17, 2020 13:51
@skvrahul
Copy link
Contributor Author

@jreback Added tests and moved one of the tests to its own TC.


-
-

Copy link
Member

Choose a reason for hiding this comment

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

can you undo this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Have reverted the changes.

@simonjayhawkins simonjayhawkins modified the milestones: 1.2.1, 1.3 Dec 26, 2020
Revert deleted bullet points
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.

pls also merge master

else:

if n > max_seq_items:
if max_seq_items == 1:
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 a case of max_seq_items==0 ? (I think this is a valid value), < 0 is restricted i am pretty sure?

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 we do. Max_seq_items ==0 implies unlimited if I'm not mistaken

@jreback jreback merged commit 760c6ff into pandas-dev:master Dec 28, 2020
@jreback
Copy link
Contributor

jreback commented Dec 28, 2020

thanks @skvrahul very nice

luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Output-Formatting __repr__ of pandas objects, to_string

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: MultiIndex.__repr__ is broken when display.max_seq_items = 1

3 participants