BUG: #714, changed equal to check for hash equality b/c nan != nan
State: closed


Labels:

Comments:
you're going to want to reorder those comparisons because `a == b` needs to short circuit first (if you explore the performance a bit you will see why). also fairly certain that's going to hurt performance everywhere, will need to do some explorations

That's why I didn't commit master :) yeah, should definitely look at perf implications

Benchmarks w/o change:

   #define kh_python_hash_equal(a, b) ((a == b) || PyObject_RichCompareBool(a, b, Py_EQ))

   khash, preallocate  khash  
   100    0.001453            0.002522
   1000   0.001632            0.002654
   5000   0.001664            0.003382
   10000  0.001709            0.003914
   25000  0.002176            0.005938
   50000  0.004211            0.009230

After change:

#define kh_python_hash_equal(a, b) ((a == b) || PyObject_RichCompareBool(a, b, Py_EQ) || (PyObject_Hash(a) == PyObject_Hash(b)))

   khash, preallocate  khash  
   100    0.001533            0.002443  
   1000   0.001596            0.002734  
   5000   0.001713            0.003516  
   10000  0.002130            0.004592  
   25000  0.002327            0.006968  
   50000  0.004822            0.009667 

Difference:

In [12]: df2/df1-1
Out[12]: 
   khash,  preallocate  khash  
0  0       0.05506     -0.03132
1  0      -0.02206      0.03014
2  0       0.02945      0.03962
3  0       0.24634      0.17322
4  0       0.06939      0.17346
5  0       0.14510      0.04735

This latter actually breaks one random test, still have to figure out what's going on.

```
======================================================================
ERROR: test_crosstab_margins (pandas.tools.tests.test_pivot.TestCrosstab)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/adam/code/pandas/pandas/tools/tests/test_pivot.py", line 213, in test_crosstab_margins
    margins=True)
  File "/home/adam/code/pandas/pandas/tools/pivot.py", line 258, in crosstab
    aggfunc=len, margins=margins)
  File "/home/adam/code/pandas/pandas/tools/pivot.py", line 100, in pivot_table
    cols=cols, aggfunc=aggfunc)
  File "/home/adam/code/pandas/pandas/tools/pivot.py", line 171, in _add_margins
    row_margin[k] = grand_margin[k[0]]
  File "/home/adam/code/pandas/pandas/core/series.py", line 381, in __setitem__
    raise KeyError('%s not in this series!' % str(key))
KeyError: "('__dummy__', 'All', '') not in this series!"

----------------------------------------------------------------------
Ran 1378 tests in 25.483s
```

Hm. it's not terrible. i need to think a bit-- I'm not sure that making `PyObjectHashTable` NaN-friendly is the right solution (especially if it requires calling `PyObject_Hash` twice with every lookup as in this case). MIght be worth adding an NA check inside the unique function (`if val == val`)-- part of me thinks that `Series.unique` should simply ignore NA values (or maybe have a `skipna` option)

For reference R's `unique` does include NAs in the output so I guess excluding altogether is out. Maybe tracking whether or not NA has been seen in a bint or something. hmm

I tried 

```
    for i in range(n):
        val = values[i]
        if val != val:
            if not seen_na:
                uniques.append(val)
                seen_na = 1
        else:
            k = kh_get_pymap(self.table, <PyObject*>val)
            if k == self.table.n_buckets:
                k = kh_put_pymap(self.table, <PyObject*>val, &ret)
                uniques.append(val)
```

Looks like:

```
In [3]: old
Out[3]: 
   khash,  preallocate  khash   
0  100     0.001466     0.001812
1  1000    0.001569     0.002154
2  5000    0.001637     0.002761
3  10000   0.001692     0.003246
4  25000   0.002096     0.005000
5  50000   0.004288     0.008018

In [4]: new
Out[4]: 
   khash,  preallocate  khash   
0  100     0.002091     0.002336
1  1000    0.002269     0.002703
2  5000    0.002314     0.003464
3  10000   0.002403     0.003969
4  25000   0.002861     0.005787
5  50000   0.005137     0.008969

In [5]: new/old-1
Out[5]: 
   khash,  preallocate  khash 
0  0       0.4263       0.2892
1  0       0.4461       0.2549
2  0       0.4136       0.2546
3  0       0.4202       0.2227
4  0       0.3650       0.1574
5  0       0.1980       0.1186
```