Skip to content

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

Merged
rth merged 1 commit intoscikit-learn:masterfrom
jeremiedbb:common-language-directive
Apr 13, 2019
Merged

[MRG] MNT Use a common language_level cython directive#13630
rth merged 1 commit intoscikit-learn:masterfrom
jeremiedbb:common-language-directive

Conversation

@jeremiedbb
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
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

# skip comments
line_cstr = line
hash_ptr = strchr(line_cstr, '#')
hash_ptr = strchr(line_cstr, 35) # ASCII value of '#' is 35
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! 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
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.

4 participants