Series.rank() doesn't handle small floats correctly
State: closed

Okay, this fixes #6868 but does so with a bit of a performance penalty. The current pandas version performs just a tad slower than `scipy.stats.rankdata()`, but after these changes, it's about 2-3x slower. On the plus side, it does (what I think is) the right thing.

I'm no cython expert, so there may well be things that can be done to improve the speed.

Here's some quick benchmarking code (couldn't figure out how to work with the vbench suite):

```
import timeit
setup2 = """import pandas
import scipy.stats
import numpy
numpy.random.seed(154)
s = pandas.Series(numpy.random.normal(size=10000))"""

print "pandas:", timeit.repeat(stmt='s.rank()', setup=setup2, repeat=3, number=10000)
print "scipy:", timeit.repeat(stmt='scipy.stats.rankdata(s)', 
    setup=setup2, repeat=3, number=10000)
```


Labels:
name: Bug
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff

Comments:
okay, so I've been looking at this (cython -a is really useful here, but you have to copy all the pyx files that are under pandas/ to pandas/src). Adding the `bint` declaration + changing the inf comparisons to use c-level declarations seems to bring pandas closer to scipy:

```
In [14]: %timeit s.rank()
1000 loops, best of 3: 1.17 ms per loop

In [15]: %timeit scipy.stats.rankdata(s)
1000 loops, best of 3: 915 µs per loop
```

I can't figure out how to get the last line not to go up to Python level though.

``` cython
cdef inline bint float64_are_diff(float64_t left, float64_t right):
    cdef double abs_diff, allowed
    if right == MAXfloat64 or right == -MAXfloat64:
        if left == right:
            return False
        else:
            return True
    else:
        abs_diff = fabs(left - right)
        allowed = REL_TOL * fabs(right)
        return abs_diff > allowed
```

Here's cython -a output (complete white means entirely at C-level, bright yellow means entirely at python level):

![cython-a output](https://cloud.githubusercontent.com/assets/1598379/2715534/f35ecdd0-c50f-11e3-960c-1f3eddb87678.png)

Just for completeness, that's a reduction from 52cafd0, not just machine differences:

``` python
# 52cafd0
In [4]: %timeit s.rank()
%te100 loops, best of 3: 4.3 ms per loop

In [5]: %timeit scipy.stats.rankdata(s)
1000 loops, best of 3: 953 µs per loop
# small changes to use Cythonized inf comparison
```

so at the least we need to make sure that we are handling infs correctly

The changes you've made have brought the speed pretty much up to par with the original (master?).

Cool well it was just tweaking what you had. Would you be able to put together a bench mark + a test that includes infs (unless we have one already) ? Then we could get this merged. 

This is fabulous: no, there was no test for ranking with infs, and no, neither the new nor the original version passes the test that I put in. The problem is that all nan values get clobbered with either `inf` or `-inf`, and hence the loop doesn't distinguish between inputs that had an `(-)inf` and inputs that had a `nan`. Don't have time right now to think about how to reimplement things to fix this, but hopefully shouldn't be too difficult.

As for benchmarking the ranking, I've found several in stat_ops. 52172e9 which incorporates your changes shows performance comparable to the master.

tl;dr we've fixed the float precision issue but uncovered a pre-existing inf issue.

Awesome! Great to find hidden bugs!

@nspies can you add a vbench for this ? (in vb_suite) somewhere

need a release note referecing the bugfix issue

also, don't merge in master to your branch, pls rebase this away

see here: https://github.com/pydata/pandas/wiki/Using-Git

@nspies ping me when you have updated

I did the fetch upstream/rebase upstream/master but it won't let me push (says the tip of my current branch is behind its remote counterpart). A little more help? Sorry, still a git newbie :)

Also, there're a bunch of vbench benchmarks already which I think should suffice: http://pandas.pydata.org/pandas-docs/vbench/vb_stat_ops.html#stats-rank-average. Let me know if you think I should add something more.

What do you want to do about the inf ranking issue -- report as a separate bug? Right now, the unit test I added doesn't pass on travis CI.

On Apr 21, 2014, at 10:28 AM, jreback wrote:

> @nspies can you add a vbench for this ? (in vb_suite) somewhere
> 
> need a release note referecing the bugfix issue
> 
> also, don't merge in master to your branch, pls rebase this away
> 
> see here: https://github.com/pydata/pandas/wiki/Using-Git
> 
> —
> Reply to this email directly or view it on GitHub.

after rebasing you need to force push

eg

git push myfork thisbranch -f

Okay, got it pushed. Let me know if you want me to do something else here.

On Apr 22, 2014, at 10:48 AM, jreback wrote:

> after rebasing you need to force push
> 
> eg
> 
> git push myfork thisbranch -f
> 
> —
> Reply to this email directly or view it on GitHub.

@nspies - in regards to inf fix - either way is fine (but add a skip to the
test if it isn't fixed in this issue)

can you rebase and push again?

Okay, done.

gr8.

can you post the perf tests (that already exist)? (`test_perf.sh -b master -t HEAD -r rank`)

I don't believe their is a issue about the `inf` ranking issue? can you create one (ref this issue
and put a pointer to the tests (after this is merged) so that when someone fixes it the tests will be obvious)

squash if you can to a single commit 

I've opened #6945 regarding the `inf` issue.

Benchmarking

```
Invoked with :
--ncalls: 3
--repeats: 3

-------------------------------------------------------------------------------
Test name                                    | head[ms] | base[ms] |  ratio   |
-------------------------------------------------------------------------------
stats_rank_average                           |  36.8500 |  37.0790 |   0.9938 |
stats_rank_average_int                       |  25.7943 |  25.9283 |   0.9948 |
stats_rank2d_axis1_average                   |  14.7780 |  14.5180 |   1.0179 |
stats_rank2d_axis0_average                   |  26.8903 |  26.0603 |   1.0318 |
stats_rank_pct_average                       |  38.9826 |  34.4080 |   1.1330 |
-------------------------------------------------------------------------------
Test name                                    | head[ms] | base[ms] |  ratio   |
-------------------------------------------------------------------------------

Ratio < 1.0 means the target commit is faster then the baseline.
Seed used: 1234
```

I did the squash (but it still shows the individual commit messages -- is there some way to get rid of them?).

merged via https://github.com/pydata/pandas/commit/d96fd800a597d0f81c4906b7b11681178d0a4ea9

thanks for the contribution!