Validate that 'name' attribute is set only if hashable 
State: closed

addresses part of issue #8263.


Labels:
name: Compat
description: pandas objects compatability with Numpy or Python functions

Comments:
a few points:
1. this needs a test to verify that it works.
2. the cleaner way to do this rather than checking this for _all_ attribute setting on every data structure would be to make `name` on a property on `Series` objects (the only type that currently has names in pandas). Something like:

``` python
    @property
    def name(self):
        return self._name

    @name.setter
    def name(self, value):
        try:
            hash(value)
        except TypeError:
            raise TypeError('name must be hashable')
        self._name = name
```

Thanks. Makes sense to me.

What next? Shall I try and make another commit on this branch while the
PR remains open, or do you want to reject it in which case (i) I could
try on a new branch and a new PR, or (ii) you could do it yourself?

Am 04.01.2015 um 20:51 schrieb Stephan Hoyer:

> a few points:
> 1. this needs a test to verify that it works.
> 2. the cleaner way to do this rather than checking this for /all/
>    attribute setting on every data structure would be to make |name|
>    on a property on |Series| objects (the only type that currently
>    has names in pandas). Something like:
>    
>    @property
>    def name(self):
>        return self._name
>    
>    @name.setter
>    def name(self, value):
>        try:
>            hash(value)
>        except TypeError:
>            raise TypeError('name must be hashable')
>        self._name = name
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/pydata/pandas/pull/9193#issuecomment-68646213.

@dr-leo just make a new commit on this PR

There was recently introduced a `pd.core.common.is_hashable()` in this PR: #8929. Maybe that can be used here to do the hash check.

@Joris: sorry, don't know how to use code from another PR in my own.

@all: I've just committed what will hopefully do part of the trick. I've
added a name property in core.series.py as Stephan suggested. I've also
added a test for this in test_series.py. I couldn't find a good place
for it so I put it behind the test_constructor_map case.

The test suite produces some failures: in test_series, line 1999, e.g.,
Series.name is set to a list type. That's mean, so I've made it a
string. This does not break that test, it is about repr.

Still there are 13 failures and one error out of 8027 tests. At least
one failure relates to Series.name. Unfortunately I am fairly unfamiliar
with nose and unittest. Before attempting to fix these failures I
thought I'd show you what I've done so far. 

 Please let me know your views on the test results and what it would
take to accept the PR.

As this is my very first PR for a serious project, I am somewhat baffled
about how much work it takes to put together good software :-O. 

Anyway, so far it has been fun to work on this and I've learnt quite a bit.

Thanks.

Leo

Am 05.01.2015 um 23:44 schrieb Joris Van den Bossche:

> There was recently introduced a |pd.core.common.is_hashable()| in this
> PR: #8929 https://github.com/pydata/pandas/pull/8929. Maybe that can
> be used here to do the hash check.
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/pydata/pandas/pull/9193#issuecomment-68795828.

@dr-leo The other PR is already merged, so you can just use the function it introduced directly.

Trust me, this gets easier with practice :)

I'm pretty sure the test failures you're getting is because `None` (the default value for name) is not hashable. We definitely want `None` to remain a valid name, so you'll need to add a special case for that.

Actually, that theory is wrong. None actually is hashable.

OK, it looks like the trouble here is related to some strange business pandas does with overwriting `__setattr__`. It looks like replacing `self._name = value` with `object.__setattr__(self, '_name', value)` should do the trick.

needs a test to see if this survives pickling 

Got you. Should all be doable.

On 13/01/2015, jreback notifications@github.com wrote:

> needs a test to see if this survives pickling
> 
> ---
> 
> Reply to this email directly or view it on GitHub:
> https://github.com/pydata/pandas/pull/9193#issuecomment-69671050

On picling: There is already a testcase in test_series @411:
    def test_pickle_preserve_name(self):
        unpickled = self._pickle_roundtrip_name(self.ts)
        self.assertEqual(unpickled.name, self.ts.name)

So I don't see any need for another.

On common.is_hashable: It returns False for NP.float64 (see below). This
breaks a couple of tests. I suppose this is a bug in is_hashable. If
not, we cannot use it to check Series.name for hashability. We allow
float64 for index labels after all.

In [3]: import numpy as NP

In [5]: f=NP.float64(3.14)

In [8]: hash(f)
Out[8]: 1846836513

In [9]: import pandas

In [11]: from pandas.core.common import is_hashable

In [12]: is_hashable(f)
Out[12]: False

Am 13.01.2015 um 00:55 schrieb jreback:

> needs a test to see if this survives pickling
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/pydata/pandas/pull/9193#issuecomment-69671050.

@dr-leo what version of numpy are you running? I'm seeing a different result on numpy 1.9.1

np1.9.1, py34,32bit, win7 64bit.

Am 15.01.2015 um 23:35 schrieb Stephan Hoyer:

> @dr-leo https://github.com/dr-leo what version of numpy are you
> running? I'm seeing a different result on numpy 1.9.1
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/pydata/pandas/pull/9193#issuecomment-70175647.

@dr-leo It's only a problem with Python 3. Just made a new issue: #9276

@dr-leo can you rebase on master and give this another try? We just fixed the is_hashable bug in #9473.

Great!

However, I am unfamiliar with rebase. To make things worse, I work with
Mercurial using hg-git. It does have a rebase extension but fiddling
with history is not one of my passions.

My hope was that you could simply merge my little PR branch into
master... That said, if you give me a hint I could try to help.

Leo

Am 17.02.2015 um 03:23 schrieb Stephan Hoyer:

> @dr-leo https://github.com/dr-leo can you rebase on master and give
> this another try? We just fixed the is_hashable bug in #9473
> https://github.com/pydata/pandas/pull/9473.
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/pydata/pandas/pull/9193#issuecomment-74607770.

needs a perf check 

closing pls reopen if/when updated