MNT Removing Cython compilation warnings in WeightVector template #20739
Conversation
This removes spurious checks in Cython which can slow down the execution.
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
| # cython: profile=False | ||
| # cython: linetrace=False | ||
| # cython: initializedcheck=False | ||
| # cython: binding=False | ||
| # distutils: define_macros=CYTHON_TRACE_NOGIL=0 |
There was a problem hiding this comment.
The profiling and linetracing is false by default.
| # cython: profile=False | |
| # cython: linetrace=False | |
| # cython: initializedcheck=False | |
| # cython: binding=False | |
| # distutils: define_macros=CYTHON_TRACE_NOGIL=0 | |
| # cython: initializedcheck=False | |
| # cython: binding=False |
Is binding=False required here?
There was a problem hiding this comment.
The purpose of setting directives explicitly for files is that it allows generating code for given file (using cythonize for instance), but they aren't needed.
We should probably remove them anyway, yes.
The binding directive is False by default but will change to True in Cython 3 according to the docs.
There was a problem hiding this comment.
If it is relevant setting binding's value, it's probably better to do so in scikit-learn/sklearn/_build_utils/__init__.py .
There was a problem hiding this comment.
Okay lets keep binding=False.
| @@ -18,16 +18,24 @@ dtypes = [('64', 'double', 1e-9), | |||
|
|
|||
| }} | |||
|
|
|||
| # cython: language_level=3 | |||
There was a problem hiding this comment.
Hmm we do set language_level globally here:
but it is a little nicer to set it explicitly.
There was a problem hiding this comment.
As per indicated in the other thread, it helps generating code for this file solely if needed, but probably this should be removed.
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
…ikit-learn#20739) Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Reference Issues/PRs
Fixup for #20481
What does this implement/fix? Explain your changes.
Cython raises a warning at compile time because the
WeightVectortemplate importssqrtfromlibc.mathtwice with different signatures.This import
sqrtonce, removing the warning.Any other comments?
Shouldn't we be stricter and we make the CI fail on Cython (but not on C or C++) compilation warnings?