MAINT Fix GCC warnings from Cython by thomasjpfan · Pull Request #14077 · scikit-learn/scikit-learn

thomasjpfan · 2019-06-12T17:44:10Z

A 9 hour train ride is the best time to fix Cython warnings:

Updates the signature of PyArray_NewFromDescr.
Initializes original_median = 0.0, they are not used update_median_parameters_post_remove or update_median_parameters_post_push when the size is 0.

jnothman

Otherwise lgtm

jnothman · 2019-06-13T07:30:20Z

sklearn/neighbors/dist_metrics.pyx


+    cdef inline DTYPE_t rdist(self, DTYPE_t* x1, DTYPE_t* x2,
+                              ITYPE_t size) nogil except -1:
+        return self._rdist(x1, x2, size)


But this is definitely uglier and makes the code harder to understand. I think it at least deserves a comment referencing this pull request

The other way to make this work is to not have inline. The generated C code for any rdist and dist are not marked with CYTHON_INLINE. I suspect because the base class DistanceMetric does not mark rdist and dist as inline. DistanceMetric can not mark them as inline because it would make them "final".

TLDR: Remove inline everywhere since Cython does not generated code that inlines rdist and dist anyways.

I am fine with the last options but please check with a short micro-benchmark.

For future reference, the Cython docs state:

Note that class-level cdef functions are handled via a virtual function table, so the compiler won’t be able to inline them in almost all cases.

https://cython.readthedocs.io/en/latest/src/userguide/pyrex_differences.html#cdef-inline

thomasjpfan · 2019-06-15T18:58:24Z

This PR was updated:

Remove some warnings with gcc regarding unsigned/signed ints.
Reverted the dist_metrics.pyx changes. This should be in its own PR. Currently the inline for dist may not be respected and it would be nice to have it inline (somehow).

thomasjpfan · 2019-06-15T19:44:20Z

Error is unrelated to this PR.

rth · 2019-06-15T21:34:35Z

sklearn/linear_model/cd_fast.pyx

    cdef floating dual_norm_XtA
    cdef unsigned int ii
-    cdef unsigned int n_iter = 0
+    cdef int n_iter = 0


Do you remember what was the warning? uint seems like the right type for n_iter..

n_iter is used to iterate later:

for n_iter in range(max_iter): ...

where max_iter is an int. When n_iter is a unsigned int we get warning similar to this:

warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for (__pyx_t_10 = 0; __pyx_t_10 < __pyx_t_29; __pyx_t_10+=1) {

Alternatively we can cast max_iter to an unsigned int.

Casting seems like a bad idea, there isn't any checking of max_iter when enet_path calls enet_coordinate_descent.

rth · 2019-06-15T21:38:16Z

sklearn/utils/sparsefuncs_fast.pyx

    # and their corresponding values are stored in:
    #    data[indptr[i]:indptr[i+1]]
-    cdef np.npy_intp i, j
+    cdef unsigned long long i


intp seems like the right dtype here.

Integer used for indexing (same as C ssize_t; normally either int32 or int64)

What was the warning?

Later we use i as an index for n_samples which is an unsigned long long. If i does not match this type, we get a comparison warning like above.

j will iterate over X_indptr[i] to X_indptr[i + 1] which is an integral type. If i does not match this type, we get a comparison warning like above.

Traditionally np.npy_intp is used as an index for numpy arrays, which allows for negative indexing, i.e. x[-3]. In this case we set wraparound=False, thus only accepting positive integers as indices. In this case, I think this allows for large sparse matrices with a size of MAX(unsigned long long)

Alternatively, we can fully embrace np.intp and have:

cdef np.npy_intp n_samples = shape[0]

rth · 2019-06-15T21:38:32Z

sklearn/utils/sparsefuncs_fast.pyx

    # cannot be declared directly and can only be passed as function arguments
    cdef:
-        np.npy_intp i
+        unsigned long long i


Same as above in the PR (below in the code)

thomasjpfan · 2019-06-30T23:01:44Z

@rth I reverted the cython change regarding indexing to make this PR smaller. (I will open another PR for the index changes)

NicolasHug · 2019-07-01T13:11:07Z

this is fixing gcc warnings rather than cython warnings, right?

adrinjalali

Thanks @thomasjpfan

thomasjpfan · 2019-07-01T14:33:38Z

this is fixing gcc warnings rather than cython warnings, right?

Yup - Change title to reflect this.

rth

Thanks @thomasjpfan !

thomasjpfan added 5 commits June 12, 2019 08:29

BLD No warnings on quad_tree

f87a6ca

BLD No warnings in dist_metrics

daae017

BLD No warnings in _tree

3b51493

BLD No warnings in tree._utils.pyx

6ba0c4a

REV Less diffs

c31cf24

jnothman approved these changes Jun 13, 2019

View reviewed changes

thomasjpfan added 7 commits June 15, 2019 12:44

Merge remote-tracking branch 'upstream/master' into cython-compile-fixes

e613f06

REV Distance metrics

e75563e

MAINT Uses signed int as index

c92835a

MAINT Adjust types of temp index variables

dd139df

MAINT Uses correct types of indexing

bc3863b

MAINT Updates indexes in cd_fast

a88c900

REV Remove splitting changes

a709aca

rth reviewed Jun 15, 2019

View reviewed changes

thomasjpfan added 5 commits June 25, 2019 14:33

Merge remote-tracking branch 'upstream/master' into cython-compile-fixes

a367c12

CLN Removes unused k

2e806d3

REV Less diffs

cd01837

Merge remote-tracking branch 'upstream/master' into cython-compile-fixes

fb9adb9

REV Index changes

0c677af

adrinjalali approved these changes Jul 1, 2019

View reviewed changes

thomasjpfan changed the title ~~[MRG] MAINT Fixes Cython warnings~~ [MRG] MAINT Fixes GCC warnings from Cython Jul 1, 2019

rth approved these changes Jul 1, 2019

View reviewed changes

rth changed the title ~~[MRG] MAINT Fixes GCC warnings from Cython~~ MAINT Fix GCC warnings from Cython Jul 1, 2019

rth merged commit 581b0e1 into scikit-learn:master Jul 1, 2019

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

MAINT Fix GCC warnings from Cython (scikit-learn#14077)

d9e4f7b

Uh oh!

Conversation

thomasjpfan commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented Jun 15, 2019

Uh oh!

thomasjpfan commented Jun 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth Jun 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasHug commented Jul 1, 2019

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan commented Jul 1, 2019

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thomasjpfan commented Jun 12, 2019 •

edited

Loading

rth Jun 15, 2019 •

edited

Loading

thomasjpfan commented Jun 30, 2019 •

edited

Loading