Skip to content

simplify skiplist inclusion/cimport to be more cythonize-friendly #18420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 27, 2017
Merged

simplify skiplist inclusion/cimport to be more cythonize-friendly #18420

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d0dbe93
simplify skiplist inclusion/cimport to be more cythonize-friendly
jbrockmendel Nov 22, 2017
1e80126
restore signbit as it broke on appveyor
jbrockmendel Nov 22, 2017
35c87b7
Merge branch 'master' of https://github.com/pandas-dev/pandas into pr…
jbrockmendel Nov 24, 2017
0fde045
make skiplist a standalone extension
jbrockmendel Nov 24, 2017
64d5482
revert sqrt to use nogil version
jbrockmendel Nov 26, 2017
81b1762
Merge branch 'master' of https://github.com/pandas-dev/pandas into pr…
jbrockmendel Nov 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions pandas/_libs/skiplist.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# -*- coding: utf-8 -*-
# cython: profile=False

from cython cimport Py_ssize_t

from numpy cimport double_t


cdef extern from "src/skiplist.h":
ctypedef struct node_t:
node_t **next
int *width
double value
int is_nil
int levels
int ref_count

ctypedef struct skiplist_t:
node_t *head
node_t **tmp_chain
int *tmp_steps
int size
int maxlevels

skiplist_t* skiplist_init(int) nogil
void skiplist_destroy(skiplist_t*) nogil
double skiplist_get(skiplist_t*, int, int*) nogil
int skiplist_insert(skiplist_t*, double) nogil
int skiplist_remove(skiplist_t*, double) nogil


# Note: Node is declared here so that IndexableSkiplist can be exposed;
# Node itself not intended to be exposed.
cdef class Node:
cdef public:
double_t value
list next
list width


cdef class IndexableSkiplist:
cdef:
Py_ssize_t size, maxlevels
Node head

cpdef get(self, Py_ssize_t i)
cpdef insert(self, double value)
cpdef remove(self, double value)
18 changes: 9 additions & 9 deletions pandas/_libs/src/skiplist.pyx → pandas/_libs/skiplist.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@

# Cython version: Wes McKinney

cdef extern from "math.h":
double log(double x)
from libc.math cimport log

# MSVC does not have log2!

Expand All @@ -16,6 +15,7 @@ cdef double Log2(double x):

cimport numpy as np
import numpy as np
from numpy cimport double_t

from random import random

Expand All @@ -25,10 +25,10 @@ np.import_array()
# TODO: optimize this, make less messy

cdef class Node:
cdef public:
double_t value
list next
list width
# cdef public:
# double_t value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you commenting these out? just to show what the structure is? I would remove this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren't you supposed to declare the member variables in the .pyx, and not the .pxd?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren't you supposed to declare the member variables in the .pyx, and not the .pxd?

AFAICT the rule is that if the class is declared in the .pxd, the member variables need to be declared there (and only there)

why are you commenting these out? just to show what the structure is? I would remove this

I leave these in place so we don't have to go back and check the pxd

# list next
# list width

def __init__(self, double_t value, list next, list width):
self.value = value
Expand All @@ -43,9 +43,9 @@ cdef class IndexableSkiplist:
Sorted collection supporting O(lg n) insertion, removal, and
lookup by rank.
"""
cdef:
Py_ssize_t size, maxlevels
Node head
# cdef:
# Py_ssize_t size, maxlevels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

# Node head

def __init__(self, expected_size=100):
self.size = 0
Expand Down
22 changes: 0 additions & 22 deletions pandas/_libs/src/skiplist.pxd
View file
Open in desktop

This file was deleted.

19 changes: 7 additions & 12 deletions pandas/_libs/window.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,15 @@ np.import_array()
cimport util

from libc.stdlib cimport malloc, free

from libc.math cimport sqrt

from numpy cimport ndarray, double_t, int64_t, float64_t

from skiplist cimport (IndexableSkiplist,
node_t, skiplist_t,
skiplist_init, skiplist_destroy,
skiplist_get, skiplist_insert, skiplist_remove)

cdef np.float32_t MINfloat32 = np.NINF
cdef np.float64_t MINfloat64 = np.NINF

Expand All @@ -30,19 +35,9 @@ cdef inline int int_min(int a, int b): return a if a <= b else b

from util cimport numeric

from skiplist cimport (
skiplist_t,
skiplist_init,
skiplist_destroy,
skiplist_get,
skiplist_insert,
skiplist_remove)

cdef extern from "../src/headers/math.h":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this makes any difference (the sqrt & log above)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be pretty weird (isn't the path there incorrect anyway?) but no harm in reverting it just to see...

Since skiplist is only used in window, and used to be "include"d anyway, I think pasting it at the bottom of window is a decent fallback option.

Copy link
Member Author

@jbrockmendel jbrockmendel Nov 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending double-checking... reverting to getting sqrt from src/headers/math.h seems to make up the difference and then some. It looks like the sqrt exposed in libc.math is not declared with nogil; that could plausibly make a difference. ... or I could accidentally be profiling one version via SSH and another locally. Never mind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, reverting the sqrt import does appear to have closed the gap.

taskset 2 asv continuous -f 1.1 -E virtualenv master HEAD -b rolling -b quantile -b median
[...]
BENCHMARKS NOT SIGNIFICANTLY CHANGED.

taskset 2 asv continuous -f 1.1 -E virtualenv master HEAD -b rolling -b quantile -b median
[...]
       before           after         ratio
     [2dbf2a6a]       [4432b71c]
-           2.61s            2.35s     0.90  rolling.DataframeRolling.time_rolling_cov
-           2.71s            2.23s     0.82  rolling.DataframeRolling.time_rolling_corr

taskset 2 asv continuous -f 1.1 -E virtualenv master HEAD -b rolling -b quantile -b median
[...]
BENCHMARKS NOT SIGNIFICANTLY CHANGED.

I'll push with this change and open an issue to check for other places where libc.sqrt is cimported instead of the nogil version.

double sqrt(double x) nogil
cdef extern from "src/headers/math.h":
int signbit(double) nogil

include "skiplist.pyx"

# Cython implementations of rolling sum, mean, variance, skewness,
# other statistical moment functions
Expand Down
8 changes: 5 additions & 3 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,7 @@ class CheckSDist(sdist_class):
'pandas/_libs/missing.pyx',
'pandas/_libs/testing.pyx',
'pandas/_libs/window.pyx',
'pandas/_libs/skiplist.pyx',
'pandas/_libs/sparse.pyx',
'pandas/_libs/parsers.pyx',
'pandas/_libs/tslibs/strptime.pyx',
Expand Down Expand Up @@ -544,6 +545,9 @@ def pxd(name):
'_libs.reshape': {
'pyxfile': '_libs/reshape',
'depends': _pxi_dep['reshape']},
'_libs.skiplist': {
'pyxfile': '_libs/skiplist',
'depends': ['pandas/_libs/src/skiplist.h']},
'_libs.sparse': {
'pyxfile': '_libs/sparse',
'depends': _pxi_dep['sparse']},
Expand Down Expand Up @@ -629,9 +633,7 @@ def pxd(name):
'pyxfile': '_libs/testing'},
'_libs.window': {
'pyxfile': '_libs/window',
'pxdfiles': ['_libs/src/skiplist', '_libs/src/util'],
'depends': ['pandas/_libs/src/skiplist.pyx',
'pandas/_libs/src/skiplist.h']},
'pxdfiles': ['_libs/skiplist', '_libs/src/util']},
'io.sas._sas': {
'pyxfile': 'io/sas/sas'}}

Expand Down