CLN: remove unused parts of skiplist (most of it) #27465
Conversation
|
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 |
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. |
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 agree, as mentioned in the OP.
The obvious wrongness of this upsets me. You can disagree without being disparaging. |
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.
great, but then why are we removing this
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 |
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 Brock is replying that the stuff he's deleting isn't related to code that's in 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 ?" are probably better. The "AFAICT" qualifier in #27465 (comment) seems to be important. Jeff says otherwise "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 |
|
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 |
| @@ -5,144 +5,3 @@ | |||
| # Link: http://code.activestate.com/recipes/576930/ | |||
|
|
|||
| # Cython version: Wes McKinney | |||
There was a problem hiding this comment.
Is there a reason that we need this file (now without any code) ?
There was a problem hiding this comment.
No, we'd need to update setup.py
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