simplify skiplist inclusion/cimport to be more cythonize-friendly
State: closed

Next up in the moving-towards-`cythonize` parade...

`_libs.window` has both `include "skiplist.pyx"` and `from skiplist cimport`.  The cimport refers to `src/skiplist.pxd`, which is just passing through declarations from `src/skiplist.h`.  This PR removes skiplist.pxd and moves its contents into src/skiplist.pyx.

Background on motivation:  when `cythonize` is used in setup.py it chokes on cimports from the src/ directory.  After some troubleshooting to avoid this choking, I decided to take the alternate route of just avoiding cimporting from there.  This is the first of four cimports to remove.

- [ ] closes #xxxx
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Clean

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18420?src=pr&el=h1) Report
> Merging [#18420](https://codecov.io/gh/pandas-dev/pandas/pull/18420?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/d1010643fea058ba43c2c7124af75cc462ccf242?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/18420/graphs/tree.svg?height=150&src=pr&token=eZ4WkYLtcO&width=650)](https://codecov.io/gh/pandas-dev/pandas/pull/18420?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #18420   +/-   ##
=======================================
  Coverage   91.33%   91.33%           
=======================================
  Files         163      163           
  Lines       49801    49801           
=======================================
  Hits        45487    45487           
  Misses       4314     4314
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.13% <ø> (ø)` | :arrow_up: |
| #single | `40.77% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18420?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18420?src=pr&el=footer). Last update [d101064...81b1762](https://codecov.io/gh/pandas-dev/pandas/pull/18420?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

so rather than making this even more obscure. why don't you just make skiplist.pyx a separate cython module? and leave the pxd (move out of src is ok).
> so rather than making this even more obscure. why don't you just make skiplist.pyx a separate cython module? and leave the pxd (move out of src is ok).

Works for me.  We'll need to do the same for khash anyway.  Any objection to lumping that into this PR?
> Works for me. We'll need to do the same for khash anyway. Any objection to lumping that into this PR?

no.


As I look at it more closely, this is going to take non-trivial refactoring to make make _libs.skiplist and make the appropriate cimports available in the appropriate places.  Given that it currently `include`d into window.pyx, why not just paste it into the bottom of that file?  It's only 146 lines.
skiplist should be a separate 
khash is header only so this is different 
separate module
ok this looks fine. can you run some of the rolling benchmarks (quantile, median) which use this. should be no change.
```
taskset 2 asv continuous -f 1.1 -E virtualenv master HEAD -b rolling -b quantile -b median
[...]
     [2dbf2a6a]       [0fde0459]
+       124±0.9ms        140±0.8ms     1.13  rolling.SeriesRolling.time_rolling_quantile_median
```

Ran a few times; this looks consistent.  I guess when things are exposed via pxd, the compiler cant be quite as aggressive as it could be if all uses are internal?
concur

master 
```
In [1]: np.random.seed(1234); df = pd.DataFrame({'a': np.random.random(100000)})

In [2]: %timeit df.rolling(10).median()
59.6 ms ± 3.71 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [3]: %timeit df.rolling(10).median()
58.4 ms ± 748 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [4]: %timeit df.rolling(10).quantile(.1)
140 ms ± 4.31 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [5]: %timeit df.rolling(10).quantile(.1)
139 ms ± 4.74 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)
```

PR
```
In [1]: np.random.seed(1234); df = pd.DataFrame({'a': np.random.random(100000)})

In [2]: %timeit df.rolling(10).median()
60.4 ms ± 3.98 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [3]: %timeit df.rolling(10).median()
63.7 ms ± 4.25 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [4]: %timeit df.rolling(10).quantile(.1)
162 ms ± 4.49 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [5]: %timeit df.rolling(10).quantile(.1)
159 ms ± 5.95 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)
```
so the sqrt nogil was the culprit?
verified locally this looks good.
> so the sqrt nogil was the culprit?

AFAICT yes.  I added a note in #18125 to track down other places where we might be using the wrong one.
> so the sqrt nogil was the culprit?

Can't be. From the point of view of Cython, both use "nogil", thus are entirely the same, and should result in the same C code. The only potential difference is the order in which C header files are included, but your code would better not depend on that...