FIX: interesction and union changed index names. fixes #9943 partly #9862

[hsperr]

had some time this morning and started working

closes #9943

for now I just uncommented the explicit setting to None of the index names.
let me know if I should remove it completely.

[sinhrks]

Related to #9926. From the issue, expected behavior is:

• In case of Index (ops/method) Index, name is preserved if left and right index have the same name. Otherwise, name is reset to None.
• In case of Index (ops/method) scalar or scalar (ops/method) Index, name of the index is preserved.

So can you confirm our understandings are the same, and describe what is being changed?

[hsperr]

Hey sinhrks,

thanks for you kind reply. I read the task you linked and had to find that given the other two issues my understanding of the correct behavior was wrong.

I will change the behavior to be:

idx = pd.Index([1,2,3], name='a')
idx2 = pd.Index([4,5,6], name='b')
idx3 = pd.Index([1,2,3], name='a')
L = [1]

idx.union(idx2).name
#None
idx.union(idx3).name
#a
idx.union(L)
#a

idx.intersection(idx2).name
#None
idx.intersection(idx3).name
#a
idx.intersection(L).name
#a

Right now index.union(array) is crashing, I fixed that while changing the naming issue. Please let me know if that is intended or not.

I am rerunning the tests, will add above mentioned test cases and reissue a pull request later.

[jreback]

@hsperr this should mirror .append (which IIRC you fixed?) (and put the tests next to it)

[hsperr]

I am sorry that my initial PR was so vague and unclear, I will improve on that.

I just pushed the new changes.
I started out writing an extensive naming test for .append to check if intersection and append have the same naming behavior

please review:
test_append_naming_behavior()

it tests several combination of index append index index append array index append datetimeindex with same names, different names or no name.
if you have an idea for adding a multiindex assert.

I added the same tests for intersection and union to make sure the have the same behavior.
I am sorry for the many pushes but I made a few mistakes on my way to this solution.

[jreback]

don't change something unrelated (and this is in a fast path for other things, so it causes a perf drag)

[hsperr]

The reason why i changed this is, that a union between an index and a numpy/python array right now crashes on AttributeErrors

index = pd.Index([1,2,3])
index.union([4,5,6])

-> 1238 if self.dtype != other.dtype:
1239 this = self.astype('O')
1240 other = other.astype('O')

AttributeError: 'list' object has no attribute 'dtype'

so I think the _ensure_index(other) has to be done regardless of the if len(self) == 0:

I also think it is implemented like that in intersection

[sinhrks]

Can you add tests for short pathes for each dtypes, maybe fail with current impl? I agree moving tests to Base class is better.

self.assertEqual(pd.Index([], name='a').union(pd.Index([], name='a')).name, 'a')
self.assertEqual(pd.Index(['x'], name='a').union(pd.Index([], name='a')).name, 'a')
self.assertEqual(pd.Index([], name='a').union(pd.Index(['x'], name='a')).name, 'a')
self.assertEqual(pd.Index([], name='a').union(pd.Index([], name='b')).name, None)
self.assertEqual(pd.Index(['x'], name='a').union(pd.Index([], name='b')).name, None)
self.assertEqual(pd.Index([], name='a').union(pd.Index(['x'], name='b')).name, None)

[hsperr]

ok I have been looking into the base test and I think I get the general architecture now.
I suppose the right way to do it then is to use the self.create_index() method since that gets overwritten by every subclass and returns the right index. I wonder if its okay to give that create index two optional parameters namely 'content' and 'name' or something or whether I should first create and index and then give it a name.

Just to make sure I understand could you explain more on what a short path for each dtype means? is it related to the code you posted?

[sinhrks]

@hsperr I meant these lines which return values without calling _wrap_union_result.

• https://github.com/pydata/pandas/blob/master/pandas/core/index.py#L1220
• https://github.com/pydata/pandas/blob/master/pandas/core/index.py#L1223

Because ``DatetimeIndex` has separate logic and similar short path, I think it is better to test each dtypes.

• https://github.com/pydata/pandas/blob/master/pandas/tseries/index.py#L970

[hsperr]

Ok sorry for the long break, I was out of town for a while but I am still active on this issue.

@sinhrks I understand the short paths now and build a testcase that helped me find quite a few of them. I am having a bit of trouble moving the testcase to Base class though since some indexes (Categorical) require special setup (same categories etc.) But I will figure it out!

Another question I have is in this case
https://github.com/pydata/pandas/blob/master/pandas/core/index.py#L1375
it seems to me if we union an empty index with a non empty one we just return the non empty one, in this case setting the correct name would be changeing the name of the other index I guess.

I was also wondering if it is too dirty to make something like a decorator function that determines what the name should be before we enter and makes sure the name is correct when it returns, no matter which of the returns we take, might make up for less/cleaner code

[sinhrks]

it seems to me if we union an empty index with a non empty one we just return the non empty one, in this case setting the correct name would be changeing the name of the other index I guess.

I think this should be shallow-copied like #10033, @jreback and @shoyer ?

I was also wondering if it is too dirty to make something like a decorator function

You may be able to use common._maybe_match_name. But the logic looks not to cover objects which doesn't have name attribute. Changing the func to cover them makes impl easier?

https://github.com/pydata/pandas/blob/master/pandas/core/common.py#L3324

[sinhrks]

Can you change None comparisons to self.assertTrue(... is None)? Sorry my test examples are incorrect.
Or you can use check_index_equal to check names (see #9972).

[hsperr]

I pushed the code that I have so far, to discuss a few items and whether the direction is okay or not.

the current test cases are here:
https://github.com/hsperr/pandas/blob/index_names/pandas/tests/test_index.py#L91

I wanted to make them as generic as possible os every index type just has to instantiate a few indexes and can use these methods.

Problems I found:

test_union
https://github.com/hsperr/pandas/blob/index_names/pandas/tests/test_index.py#L795
right now fails because I changed
https://github.com/hsperr/pandas/blob/index_names/pandas/core/index.py#L1372

return _ensure_index(other) 

to

result = _ensure_index(other, copy=True)
result.name = result_name
return result

If Index([]).union(someotherindex) is supposed to return the other index, I will add that behavior to the documentation and change the logic back

I moved ensure_index out of the if block because otherwise this line crashes with when we try to union with python arrays
https://github.com/hsperr/pandas/blob/index_names/pandas/core/index.py#L1379

https://github.com/hsperr/pandas/blob/index_names/pandas/tests/test_index.py#L4718
right now crashes because df.loc seems to call append on indexes somewhere inbetween and that resets the name (because it seems to build new indexes and the names don't match). I am still debugging this.

EDIT: I also just saw that rebasing with master mangled up the whats new file, I will change that

[jreback]

pls rebase on master

[max-sixty]

@hsperr - I was planning to have a look at this, as it intersects (no pun intended...) a fix I just worked on. Can you let us know if you're planning on pushing this over the line? Otherwise I'm happy to help out and take it there.

[hsperr]

@MaximilianR recently quite a few things came up in my private life and I didn't have too much time to get back to this task, recently I tried getting back into it but I also realize that it involves changes in many places and I do not know the pandas setup well enough to make good test cases and be sure that all the branches are covered properly. So if you want you can take it from here, and I am sorry for causing any unnecessary work. If you need some info on the task please let me know!

[jreback]

cc @MaximilianR

IIRC you have a PR to replace this, yes?

[max-sixty]

Nothing yet @jreback, but it's next on my list. TBH it's unlikely to be before next weekend...

[jreback]

thanks