CLN: remove unused parts of skiplist (most of it)
State: closed

As a follow-up I'll take a look at either removing the now-empty .pyx file or trying to port the c file to pyx

Labels:
name: Clean

Comments:
Yep, did a cython coverage run last night, found a bunch of unused code
I think we would actually prefer this in cython for ease of understanding, but I understand if there is a perf diff. Can you do a quick check and see how much differ the c-impl is vs the cython (don't spend too much time); but if its close then might be worth using the cython rather than the c impl.
@jreback not sure we're on the same page.  AFAICT the code this removes isn't a cython imlementation of skiplist.h, it's just not used.  Porting the c-impl to cython will be a separate task
> @jreback not sure we're on the same page. AFAICT the code this removes isn't a cython imlementation of skiplist.h, it's just not used. Porting the c-impl to cython will be a separate task

note sure you read my comment. this IS a cython impl that is not used of course; the point is I would rather use a cython impl than the c one if the perf impact is not bad. Sure that would be a separate task; I asked to benchmark whether a simple use of this is a performance bottleneck.
> I asked to benchmark whether a simple use of this is a performance bottleneck.

OK, what exactly would you want to benchmark?  The code being removed here isn't a drop-in replacement for any C code, so there is nothing to benchmark.

> I would rather use a cython impl than the c one if the perf impact is not bad

I agree, as mentioned in the OP.

> note sure you read my comment.

The obvious wrongness of this upsets me.  You can disagree without being disparaging.
> OK, what exactly would you want to benchmark? The code being removed here isn't a drop-in replacement for any C code, so there is nothing to benchmark.

Sure there is, this code looks like its working, how about say inserting and getting some items. just something basic to see if these are like 10x difference or 2x difference.


> I agree, as mentioned in the OP.
> 

great, but then why are we removing this

> The obvious wrongness of this upsets me. You can disagree without being disparaging.

well your response does not indicate that. I disagree, I certainly am not being disparaging in any way.

@TomAugspurger or @jorisvandenbossche can one of you take over here?  jeff and I are having trouble communicating
> @TomAugspurger or @jorisvandenbossche can one of you take over here? jeff and I are having trouble communicating

I think you should be more clear. 
> I think you should be more clear.

And I think that backs up my "trouble communicating" claim.

It appears to Jeff that I am not reading his comments.  It appears to me that he is not reading mine.  So I'm asking for a fresh pair of eyeballs, and in the interim there is no harm in putting this on the backburner.
I can try to summarize

Jeff is requesting that we take places currently using `skiplist.h` and make it use the Cython implementation.

[Brock is replying](https://github.com/pandas-dev/pandas/pull/27465#issuecomment-513493299) that the stuff he's deleting isn't related to code that's in `skiplist.h`. IOW, even if we wanted to remove skiplist.h  and use the Cython code, we wouldn't be using what's being deleted in this PR.

Things devolve from there. I'm not going to wade into the "did or didn't read comments" stuff, other than to observe that comments like that are rarely helpful. Phrasing like "Did you see <insert specific point>?"  are probably better.

---

The "AFAICT" qualifier in https://github.com/pandas-dev/pandas/pull/27465#issuecomment-513493299 seems to be important. [Jeff says otherwise](https://github.com/pandas-dev/pandas/pull/27465#issuecomment-513493587) "note sure you read my comment. this IS a cython impl that is not used of course"?

So, what's the relationship between the deleted code here and `skiplist.h`?
IIUC I don't have a problem merging this as is and leaving as a separate exercise to port the header file over to Cython. Of course we would probably be reimplementing these methods in that case anyway, but I could see it being less confusing as to how this works in the interim if this code isn't around (i.e. dead code can only be confusing)
ok, re-reading, this is what I would do.

ok on this PR to remove this dead code, but I would add a comment to a link to the cython impl (put in a link to a commit before this), so that if/when we need to port to cython we already have a reference impl.

sound good?
sure
lgtm. ping on green.
ping
@WillAyd since you're on a roll
Yep thanks @jbrockmendel