Skip to content

API: Raise when setting name via level #30574

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

Merged
merged 4 commits into from
Jan 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions doc/source/user_guide/advanced.rst
Original file line number Diff line number Diff line change
Expand Up @@ -565,19 +565,15 @@ When working with an ``Index`` object directly, rather than via a ``DataFrame``,
mi2 = mi.rename("new name", level=0)
mi2

.. warning::

Prior to pandas 1.0.0, you could also set the names of a ``MultiIndex``
by updating the name of a level.
You cannot set the names of the MultiIndex via a level.

.. code-block:: none
.. ipython:: python
:okexcept:

>>> mi.levels[0].name = 'name via level'
>>> mi.names[0] # only works for older pandas
'name via level'
mi.levels[0].name = "name via level"

As of pandas 1.0, this will *silently* fail to update the names
of the MultiIndex. Use :meth:`Index.set_names` instead.
Use :meth:`Index.set_names` instead.

Sorting a ``MultiIndex``
------------------------
Expand Down
4 changes: 2 additions & 2 deletions doc/source/whatsnew/v1.0.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,10 @@ For backwards compatibility, you can still *access* the names via the levels.
mi.levels[0].name

However, it is no longer possible to *update* the names of the ``MultiIndex``
via the name of the level. The following will **silently** fail to update the
name of the ``MultiIndex``
via the level.

.. ipython:: python
:okexcept:

mi.levels[0].name = "new name"
mi.names
Expand Down
10 changes: 10 additions & 0 deletions pandas/core/indexes/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,10 @@ def _outer_indexer(self, left, right):
_data: Union[ExtensionArray, np.ndarray]
_id = None
_name: Optional[Hashable] = None
# MultiIndex.levels previously allowed setting the index name. We
# don't allow this anymore, and raise if it happens rather than
# failing silently.
_no_setting_name: bool = False
_comparables = ["name"]
_attributes = ["name"]
_is_numeric_dtype = False
Expand Down Expand Up @@ -1214,6 +1218,12 @@ def name(self):

@name.setter
def name(self, value):
if self._no_setting_name:
# Used in MultiIndex.levels to avoid silently ignoring name updates.
raise RuntimeError(
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeError instead?

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 think a RuntimeError is best, since we don't check the type or valuje of value checked here.

"Cannot set name on a level of a MultiIndex. Use "
"'MultiIndex.set_names' instead."
)
maybe_extract_name(value, None, type(self))
self._name = value

Expand Down
1 change: 1 addition & 0 deletions pandas/core/indexes/category.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ def _simple_new(cls, values, name=None, dtype=None, **kwargs):
setattr(result, k, v)

result._reset_identity()
result._no_setting_name = False
return result

# --------------------------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions pandas/core/indexes/datetimes.py
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ def _simple_new(cls, values, name=None, freq=None, tz=None, dtype=None):
result = object.__new__(cls)
result._data = dtarr
result.name = name
result._no_setting_name = False
# For groupby perf. See note in indexes/base about _index_data
result._index_data = dtarr._data
result._reset_identity()
Expand Down
1 change: 1 addition & 0 deletions pandas/core/indexes/interval.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ def _simple_new(cls, array, name, closed=None):
result = IntervalMixin.__new__(cls)
result._data = array
result.name = name
result._no_setting_name = False
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be set to False in each _simple_new, or can it just use the class-level default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it’s just on the class, won’t it be shared amongst all instances? I think I should change the type declaration to not have a default.

Copy link
Member

Choose a reason for hiding this comment

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

If it’s just on the class, won’t it be shared amongst all instances?

I think the relevant thing is: when you set level._no_setting_name = True in the one place where you do that, will that update the class attribute, and the answer should be no.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, level._no_setting_name = True adds an instance attribute, so there shouldn't happen anything on the class attribute.

result._reset_identity()
return result

Expand Down
3 changes: 3 additions & 0 deletions pandas/core/indexes/multi.py
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,9 @@ def levels(self):
result = [
x._shallow_copy(name=name) for x, name in zip(self._levels, self._names)
]
for level in result:
# disallow midx.levels[0].name = "foo"
level._no_setting_name = True
return FrozenList(result)

@property
Expand Down
17 changes: 17 additions & 0 deletions pandas/tests/indexes/multi/test_names.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,20 @@ def test_get_names_from_levels():

assert idx.levels[0].name == "a"
assert idx.levels[1].name == "b"


def test_setting_names_from_levels_raises():
idx = pd.MultiIndex.from_product([["a"], [1, 2]], names=["a", "b"])
with pytest.raises(RuntimeError, match="set_names"):
idx.levels[0].name = "foo"
Copy link
Member

Choose a reason for hiding this comment

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

i could do something weird like:

lvl = idx.levels[0]

ser = pd.Series(lvl, index=lvl)
ser.index.name="foo"

is there a less-contrived way in which idx.levels[0] could come un-attached from the multiindex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That raises (correctly, I think). I can add a test for it if you like.


with pytest.raises(RuntimeError, match="set_names"):
idx.levels[1].name = "foo"

new = pd.Series(1, index=idx.levels[0])
with pytest.raises(RuntimeError, match="set_names"):
new.index.name = "bar"

assert pd.Index._no_setting_name is False
assert pd.Int64Index._no_setting_name is False
assert pd.RangeIndex._no_setting_name is False