Skip to content

[MRG] MNT Use a common language_level cython directive #13630

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

Jump to bottom
Merged
merged 1 commit into from
Apr 13, 2019
Merged

[MRG] MNT Use a common language_level cython directive #13630

merged 1 commit into from
Apr 13, 2019

Conversation

jeremiedbb
Copy link
Member

Cython compile directives can be commonly set during cythonization.
This PR does it for the language_level directive.

There was a file (``_svmlight_format.pyx) where it was still level 2 but it was easily fixable.

NicolasHug
NicolasHug approved these changes Apr 12, 2019
View reviewed changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Good idea, LGTM.

Do you think we could(/should?) do the same for other directives like boundchecks, wraparound, cdivision? Seems like we set them most of the time.

@jeremiedbb
Copy link
Member Author

I tried but it was not straightforward :)
I think it should be possible but this requires more work because some code uses negative indexing for instance.
Maybe we could do it globally and override it locally when necessary.

thomasjpfan
thomasjpfan approved these changes Apr 13, 2019
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

rth
rth approved these changes Apr 13, 2019
View reviewed changes
sklearn/datasets/_svmlight_format.pyx
@@ -61,7 +61,7 @@ def _load_svmlight_file(f, dtype, bint multilabel, bint zero_based,
for line in f:
# skip comments
line_cstr = line
hash_ptr = strchr(line_cstr, '#')
hash_ptr = strchr(line_cstr, 35) # ASCII value of '#' is 35
Copy link
Member

Choose a reason for hiding this comment

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

Nice! This was the part missing in #12873 that prevented me to use a global option.

@rth rth merged commit cad0fb4 into scikit-learn:master Apr 13, 2019
jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this pull request Apr 25, 2019
@jeremiedbb
MNT Use a common language_level cython directive (scikit-learn#13630)
d91682a
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@jeremiedbb
MNT Use a common language_level cython directive (scikit-learn#13630)
24c7214
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "MNT Use a common language_level cython directive (scikit-lear…
d7109ba 
…n#13630)"

This reverts commit 24c7214.
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "MNT Use a common language_level cython directive (scikit-lear…
f978381 
…n#13630)"

This reverts commit 24c7214.
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@jeremiedbb @koenvandevelde
MNT Use a common language_level cython directive (scikit-learn#13630)
bccf334
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rth rth rth approved these changes

@NicolasHug NicolasHug NicolasHug approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jeremiedbb @rth @NicolasHug @thomasjpfan