MNT Centralize common cython compiler directives #21512

jeremiedbb · 2021-10-30T12:51:10Z

There are a bunch of compiler directives that we almost always use and a few that we don't always use but could. This PR avoids repeating these directives in all cython files.

It's still possible to set or override a specific compiler directive at an extension level with the # cython: <directive> at the top of the file or at a function level with the @cython.<directive> decorator.

jeremiedbb · 2021-10-30T12:52:30Z

sklearn/cluster/_hierarchical_fast.pyx

@@ -358,8 +341,7 @@ cdef class UnionFind(object):

        return

-    @cython.boundscheck(False)
-    @cython.nonecheck(False)
+    @cython.wraparound(True)


this method does negative indexing

jeremiedbb · 2021-10-30T12:52:48Z

sklearn/cluster/_hierarchical_fast.pyx

@@ -423,6 +404,7 @@ cpdef np.ndarray[DTYPE_t, ndim=2] _single_linkage_label(
    return result_arr


+@cython.wraparound(True)


This function does negative indexing

jeremiedbb · 2021-10-30T12:54:39Z

sklearn/feature_extraction/_hashing_fast.pyx

@@ -92,7 +90,7 @@ def transform(raw_X, Py_ssize_t n_features, dtype,
    indices_a = np.frombuffer(indices, dtype=np.int32)
    indptr_a = np.frombuffer(indptr, dtype=indices_np_dtype)

-    if indptr[-1] > np.iinfo(np.int32).max:  # = 2**31 - 1
+    if indptr[len(indptr) - 1] > np.iinfo(np.int32).max:  # = 2**31 - 1


this function used to do negative indexing but since there's only one and it's for indexing the last element I find it better to replace by a usual indexing instead of adding wraparound for the whole function

jeremiedbb · 2021-10-30T12:58:50Z

sklearn/utils/_random.pyx

@@ -278,7 +275,7 @@ cpdef sample_without_replacement(np.int_t n_population,

    all_methods = ("auto", "tracking_selection", "reservoir_sampling", "pool")

-    ratio = n_samples / n_population if n_population != 0.0 else 1.0
+    ratio = <double> n_samples / n_population if n_population != 0.0 else 1.0


this function does a division between 2 integers. Setting cdivision=True would make it an integer division which is not what we want, so we use explicit casting.

I checked each division in each file where we didn't have the cdivision=True directive to make sure that there's no other case like this one.

glemaitre · 2021-10-30T14:17:49Z

sklearn/_build_utils/__init__.py

@@ -76,7 +76,14 @@ def cythonize_extensions(top_path, config):
        compile_time_env={
            "SKLEARN_OPENMP_PARALLELISM_ENABLED": sklearn._OPENMP_SUPPORTED
        },
-        compiler_directives={"language_level": 3},
+        compiler_directives={


Super cool thing :)

glemaitre

I am trusting the tests. LGTM

thomasjpfan

When looking at Cython code, I often like to run:

cython -a sklearn/ensemble/_hist_gradient_boosting/_loss.pyx

to get an annotated HTML with Python interactions. With the directives moved to a python file, I see the following warnings:

FutureWarning: Cython directive 'language_level'
 not set, using 2 for now (Py2). This will change in a later release! File: /Users/thomasfan/Repos/scikit-learn-3/sklearn/ensemble/_hist_gradi
ent_boosting/_loss.pyx                                                                                                                        
  tree = Parsing.p_module(s, pxd, full_module_name)                                                                                           
warning: sklearn/ensemble/_hist_gradient_boosting/_loss.pyx:32:38: Use boundscheck(False) for faster access
...

Do you see a good way to run cython -a without getting these warnings?

jjerphan · 2021-11-02T14:19:35Z

@thomasjpfan: you can specify directives for Cython using the --directive flag or its shorthand: -X.

In scikit-learn case (and using the file you mentioned), one can use:

cython -X language_level=3 \
       -X boundscheck=False \
       -X wraparound=False \
       -X initializedcheck=False \
       -X nonecheck=False \
       -X cdivision=True \
       -a sklearn/ensemble/_hist_gradient_boosting/_loss.pyx

jjerphan

This is a clear improvement for maintenance. 👍

There are still a few directives that are set locally, namely:

$ ag "cython:"
# ... Results stripped to only show relevant source files
sklearn/metrics/_pairwise_fast.pyx:1:#cython: boundscheck=False
sklearn/metrics/_pairwise_fast.pyx:2:#cython: cdivision=True
sklearn/metrics/_pairwise_fast.pyx:3:#cython: wraparound=False
sklearn/neighbors/_ball_tree.pyx:2:#cython: boundscheck=False
sklearn/neighbors/_ball_tree.pyx:3:#cython: wraparound=False
sklearn/neighbors/_ball_tree.pyx:4:#cython: cdivision=True
sklearn/neighbors/_ball_tree.pyx:5:#cython: initializedcheck=False
sklearn/neighbors/_kd_tree.pyx:2:#cython: boundscheck=False
sklearn/neighbors/_kd_tree.pyx:3:#cython: wraparound=False
sklearn/neighbors/_kd_tree.pyx:4:#cython: cdivision=True
sklearn/neighbors/_kd_tree.pyx:5:#cython: initializedcheck=False
sklearn/utils/_logistic_sigmoid.pyx:1:#cython: boundscheck=False
sklearn/utils/_logistic_sigmoid.pyx:2:#cython: cdivision=True
sklearn/utils/_logistic_sigmoid.pyx:3:#cython: wraparound=False
sklearn/utils/_weight_vector.pyx.tp:21:# cython: binding=False

sklearn/cluster/_hierarchical_fast.pyx

jeremiedbb · 2021-11-03T08:52:37Z

There are still a few directives that are set locally, namely:

good catch. I looked for # cython but not for #cython :)

jjerphan · 2021-11-03T09:01:01Z

There are still a few directives that are set locally, namely:

good catch. I looked for # cython but not for #cython :)

I naively looked at some file and saw this. :)

ba20b7c is treating all but sklearn/utils/_weight_vector.pyx.tp. Apart from that, this looks good to me.

jeremiedbb · 2021-11-03T09:08:01Z

ba20b7c is treating all but sklearn/utils/_weight_vector.pyx.tp. Apart from that, this looks good to me.

This is on purpose. In this file I only left the binding=False directive which was introduced by you. It's the only place we have it so I did not set it globally.

ogrisel

LGTM, assuming the CI is green.

jjerphan

LGTM. Thank you, @jeremiedbb.

jeremiedbb added 3 commits October 18, 2021 10:16

wip

db5bbc3

Merge branch 'master' into cython-compiler-directives

eaf36cb

global cython compiler directives

ea6ef6e

github-actions bot added the cython label Oct 30, 2021

jeremiedbb commented Oct 30, 2021

View reviewed changes

glemaitre reviewed Oct 30, 2021

View reviewed changes

glemaitre approved these changes Oct 30, 2021

View reviewed changes

thomasjpfan reviewed Oct 31, 2021

View reviewed changes

jjerphan self-requested a review November 2, 2021 14:24

jjerphan requested changes Nov 2, 2021

View reviewed changes

sklearn/cluster/_hierarchical_fast.pyx Show resolved Hide resolved

missing files

ba20b7c

ogrisel approved these changes Nov 3, 2021

View reviewed changes

jjerphan approved these changes Nov 3, 2021

View reviewed changes

jeremiedbb merged commit 1f8825c into scikit-learn:main Nov 3, 2021

lorentzenchr mentioned this pull request Nov 14, 2021

CI with boundscheck=False #21668

Closed

lorentzenchr added a commit to lorentzenchr/scikit-learn that referenced this pull request Nov 21, 2021

MNT remove Cython compiler directives due to scikit-learn#21512

7e0f812

lorentzenchr added a commit to lorentzenchr/scikit-learn that referenced this pull request Nov 21, 2021

MNT remove Cython compiler directives due to scikit-learn#21512

d967740

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 29, 2021

MNT Centralize common cython compiler directives (scikit-learn#21512)

0096787

lorentzenchr mentioned this pull request Nov 29, 2021

ENH Pairwise Distances ArgKmin #21462

Closed

samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021

MNT Centralize common cython compiler directives (scikit-learn#21512)

0e25732

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021

MNT Centralize common cython compiler directives (scikit-learn#21512)

c7238c8

glemaitre pushed a commit that referenced this pull request Dec 25, 2021

MNT Centralize common cython compiler directives (#21512)

caef81f

jjerphan mentioned this pull request Jun 30, 2022

(WIP) ENH: Cythonize hot path in RegularGridInterpolator scipy/scipy#16483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT Centralize common cython compiler directives #21512

MNT Centralize common cython compiler directives #21512

jeremiedbb commented Oct 30, 2021

jeremiedbb Oct 30, 2021

jeremiedbb Oct 30, 2021

jeremiedbb Oct 30, 2021

jeremiedbb Oct 30, 2021

glemaitre Oct 30, 2021

glemaitre left a comment

thomasjpfan left a comment

jjerphan commented Nov 2, 2021 •

edited

Loading

jjerphan left a comment •

edited

Loading

jeremiedbb commented Nov 3, 2021 •

edited

Loading

jjerphan commented Nov 3, 2021

jeremiedbb commented Nov 3, 2021

ogrisel left a comment

jjerphan left a comment

		@@ -423,6 +404,7 @@ cpdef np.ndarray[DTYPE_t, ndim=2] _single_linkage_label(
		return result_arr


		@cython.wraparound(True)

MNT Centralize common cython compiler directives #21512

MNT Centralize common cython compiler directives #21512

Conversation

jeremiedbb commented Oct 30, 2021

jeremiedbb Oct 30, 2021

Choose a reason for hiding this comment

jeremiedbb Oct 30, 2021

Choose a reason for hiding this comment

jeremiedbb Oct 30, 2021

Choose a reason for hiding this comment

jeremiedbb Oct 30, 2021

Choose a reason for hiding this comment

glemaitre Oct 30, 2021

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

jjerphan commented Nov 2, 2021 • edited Loading

jjerphan left a comment • edited Loading

Choose a reason for hiding this comment

jeremiedbb commented Nov 3, 2021 • edited Loading

jjerphan commented Nov 3, 2021

jeremiedbb commented Nov 3, 2021

ogrisel left a comment

Choose a reason for hiding this comment

jjerphan left a comment

Choose a reason for hiding this comment

jjerphan commented Nov 2, 2021 •

edited

Loading

jjerphan left a comment •

edited

Loading

jeremiedbb commented Nov 3, 2021 •

edited

Loading