Skip to content

[MRG] MAINT: Fix cython warnings#13461

Merged
jnothman merged 4 commits intoscikit-learn:masterfrom
thomasjpfan:fix_cython_warnings
Mar 25, 2019
Merged

[MRG] MAINT: Fix cython warnings#13461
jnothman merged 4 commits intoscikit-learn:masterfrom
thomasjpfan:fix_cython_warnings

Conversation

@thomasjpfan
Copy link
Copy Markdown
Member

What does this implement/fix? Explain your changes.

  1. Adds cython_language=3 to seq_dataset.*.tp
  2. Directly cimports _QuadTree. Previously, there was an warning:
sklearn/manifold/_barnes_hut_tsne.pyx: cannot find cimported module 'sklearn.neighbors'

This meant that the Cython code was going through Python to use _QuadTree.
3. Removes from . cimport libsvm, because it is not cimportable:

sklearn/svm/libsvm.pyx: cannot find cimported module '.'


}}

# cython: language_level=3
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought these need to be on top of the file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

they are at the top of the cython file generated by this tp. but it wouldn't hurt to put it right at the top

@adrinjalali
Copy link
Copy Markdown
Member

Otherwise LGTM, thanks. If you feel like checking cython/gcc warnings, we have some gcc warnings complaining about signed/unsigned types being compared. You could even argue they're bugs.


}}

# cython: language_level=3
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

they are at the top of the cython file generated by this tp. but it wouldn't hurt to put it right at the top

WARNING: Do not edit .pyx file directly, it is generated from .pyx.tp
"""

# cython: language_level=3
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't suppose any of these need to be in the loop


from sklearn.neighbors import quad_tree
from sklearn.neighbors cimport quad_tree
from sklearn.neighbors.quad_tree cimport _QuadTree
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice catch!

@thomasjpfan
Copy link
Copy Markdown
Member Author

@adrinjalali Most of the unsigned/signed errors come from using a signed int, such as np.npy_intp, as an index for range, where range is passed an unsigned int. For example:

cdef:
unsigned long long n_samples = shape[0]
unsigned long long n_features = shape[1]
np.ndarray[DOUBLE, ndim=1, mode="c"] norms
np.npy_intp i, j
double sum_
norms = np.zeros(n_samples, dtype=np.float64)
for i in range(n_samples):

This was last changed here: https://github.com/scikit-learn/scikit-learn/pull/9663/files

I will look into the unsigned/signed warning in another PR.

Copy link
Copy Markdown
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Thanks @thomasjpfan!

@jnothman jnothman merged commit 3b35104 into scikit-learn:master Mar 25, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants