API: Raise when setting name via level #30574

TomAugspurger · 2019-12-30T22:52:50Z

This is extremely ugly, but gets the job done. It's probably worth doing to avoid silently failing, but not sure.

jbrockmendel · 2019-12-30T23:12:12Z

pandas/core/indexes/interval.py

@@ -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


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

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.

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.

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

jbrockmendel · 2019-12-30T23:16:31Z

pandas/tests/indexes/multi/test_names.py

+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"


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?

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

topper-123

If you can avoid setting this in subclasses (other than MultiIndex), it would simplify quite a bit.

This will be removed later, when we feel confident that people have switched idiom, right? It's in general not possible to prevent people setting attributes on child objects (e.g. in CategoricalIndex, people can already do cidx.categories.name = "name") and it just adds complexity trying to prevent it, IMO.

topper-123 · 2019-12-31T00:58:20Z

pandas/core/indexes/base.py

@@ -520,6 +524,7 @@ def _simple_new(cls, values, name=None, dtype=None):
        # we actually set this value too.
        result._index_data = values
        result._name = name
+        result._no_setting_name = False


If _no_setting_name = False in the class declaration, isn't it unnecessary here? In both cases the attribute is set to False.

topper-123 · 2019-12-31T01:01:19Z

pandas/core/indexes/base.py

@@ -1214,6 +1219,12 @@ def name(self):

    @name.setter
    def name(self, value):
+        if self._no_setting_name:
+            # Used in MultiIndex.levels.
+            raise RuntimeError(


TypeError instead?

I think a RuntimeError is best, since we don't check the type or valuje of value checked here.

TomAugspurger · 2019-12-31T01:21:12Z

I think it has to be on all index subclasses since they make up the levels of a MultiIndex.

topper-123 · 2019-12-31T01:32:45Z

@TomAugspurger , but if it's set on Index all index subclasses inherit the attribute?

TomAugspurger · 2019-12-31T02:05:08Z

Right, since it’s the level of the MI that can’t have its name set, not the MI itself.

TomAugspurger · 2019-12-31T12:15:04Z

My apologies, I've simplified things a bit. I misunderstood what happened to class attributes when the attribute is set on an instance.

TomAugspurger · 2019-12-31T19:52:16Z

This will be removed later, when we feel confident that people have switched idiom, right?

I'm not sure. We can weigh the maintenance cost down the road and remove it if it's burdensome.

jbrockmendel · 2019-12-31T20:07:05Z

I'm fine with this as-is.

Spitballing another solution: we could have Index._names be an ndarray for all Index subclasses, with

@property
def name(self):
    return self._names[0]     

@name.setter
def name(self, value):
    if np.is_view_or_whatever(self._names):
        raise RuntimeError(...)
    self._names[0] = value

jreback · 2020-01-01T02:34:57Z

this looks fine, butI would consider something slightly different. We could have an attribute defined on Index._parent = None

then set a weakref when it is contained in a MultiIndex. This gives us provenance on where things are coming from. might be useful.

TomAugspurger · 2020-01-02T12:15:51Z

Mmm, that's also reasonable. My initial concern is that indexes are potentially shared, so they could have multiple parents? For this purpose it's not a problem since we're creating the Index from scratch, just something to keep in mind going forward.

jreback · 2020-01-02T12:20:20Z

Mmm, that's also reasonable. My initial concern is that indexes are potentially shared, so they could have multiple parents? For this purpose it's not a problem since we're creating the Index from scratch, just something to keep in mind going forward.

I don’t think we share for a contained Index inside a MultiIndex but not really sure

TomAugspurger · 2020-01-02T14:55:53Z

You're right, these levels of a MI are created on the fly.

In theory, a user-action could end up with an Index that's created from the level of a MI

s = pd.Series(index=idx.levels[0])

but that probably doesn't affect your proposed "parent" reference.

That implementation is going to be more complex though, since subclasses don't call Index._simple_new. Thoughts on waiting till we need it?

jreback · 2020-01-02T15:13:33Z

yeah was just an idea i think what u did here is ok
let me have another glance

jreback · 2020-01-03T02:16:43Z

thanks @TomAugspurger

API: Raise when setting via name

2dd305c

Closes pandas-dev#29032

TomAugspurger added this to the 1.0 milestone Dec 30, 2019

TomAugspurger added the MultiIndex label Dec 30, 2019

jbrockmendel reviewed Dec 30, 2019

View reviewed changes

topper-123 reviewed Dec 31, 2019

View reviewed changes

TomAugspurger added 3 commits December 31, 2019 05:59

Merge remote-tracking branch 'upstream/master' into 29032-raises

1d8ef93

remove default

a411fe9

simplify

df4762a

jreback approved these changes Jan 3, 2020

View reviewed changes

jreback merged commit 7b35099 into pandas-dev:master Jan 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Raise when setting name via level #30574

API: Raise when setting name via level #30574

TomAugspurger commented Dec 30, 2019

jbrockmendel Dec 30, 2019

TomAugspurger Dec 31, 2019

jbrockmendel Dec 31, 2019

topper-123 Dec 31, 2019

jbrockmendel Dec 30, 2019

TomAugspurger Dec 31, 2019

topper-123 left a comment

topper-123 Dec 31, 2019

topper-123 Dec 31, 2019

TomAugspurger Dec 31, 2019

TomAugspurger commented Dec 31, 2019

topper-123 commented Dec 31, 2019

TomAugspurger commented Dec 31, 2019

TomAugspurger commented Dec 31, 2019

TomAugspurger commented Dec 31, 2019

jbrockmendel commented Dec 31, 2019

jreback commented Jan 1, 2020

TomAugspurger commented Jan 2, 2020

jreback commented Jan 2, 2020

TomAugspurger commented Jan 2, 2020

jreback commented Jan 2, 2020

jreback commented Jan 3, 2020

API: Raise when setting name via level #30574

API: Raise when setting name via level #30574

Conversation

TomAugspurger commented Dec 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topper-123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Dec 31, 2019

topper-123 commented Dec 31, 2019

TomAugspurger commented Dec 31, 2019

TomAugspurger commented Dec 31, 2019

TomAugspurger commented Dec 31, 2019

jbrockmendel commented Dec 31, 2019

jreback commented Jan 1, 2020

TomAugspurger commented Jan 2, 2020

jreback commented Jan 2, 2020

TomAugspurger commented Jan 2, 2020

jreback commented Jan 2, 2020

jreback commented Jan 3, 2020