Skip to content

MAINT Remove -Wdiscarded-qualifiers compilation warning for sklearn.utils.arrayfuncs #27766

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
Nov 13, 2023
Merged

MAINT Remove -Wdiscarded-qualifiers compilation warning for sklearn.utils.arrayfuncs #27766

merged 1 commit into from
Nov 13, 2023

Conversation

ChVeen
Copy link
Contributor

@ChVeen ChVeen commented Nov 12, 2023

Reference Issues/PRs

Related to #24875.

What does this implement/fix? Explain your changes.

While compiling sklearn, we get the following warnings for module sklearn.utils.arrayfuncs.pyx:

building 'sklearn.utils.arrayfuncs' extension
gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -DNPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION -

I/usr/local/python/3.10.4/include/python3.10 -c sklearn/utils/arrayfuncs.c -o build/temp.linux-x86_64-cpython-310/sklearn/utils/arrayfuncs.o -g0 -O2 -fopenmp
sklearn/utils/arrayfuncs.c: In function ‘__pyx_pf_7sklearn_5utils_10arrayfuncs_10cholesky_delete’:
sklearn/utils/arrayfuncs.c:3942:14: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
 3942 |   __pyx_v_L1 = ((&(*((float const  *) ( /* dim=1 */ (( /* dim=0 */ (__pyx_v_L.data + __pyx_t_1 * __pyx_v_L.strides[0]) ) + __pyx_t_2 * 

__pyx_v_L.strides[1]) )))) + (__pyx_v_go_out * __pyx_v_m));
      |              ^
sklearn/utils/arrayfuncs.c:3984:14: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
 3984 |   __pyx_v_L1 = ((&(*((float const  *) ( /* dim=1 */ (( /* dim=0 */ (__pyx_v_L.data + __pyx_t_2 * __pyx_v_L.strides[0]) ) + __pyx_t_1 * 

__pyx_v_L.strides[1]) )))) + (__pyx_v_go_out * __pyx_v_m));
      |              ^
sklearn/utils/arrayfuncs.c: In function ‘__pyx_pf_7sklearn_5utils_10arrayfuncs_12cholesky_delete’:
sklearn/utils/arrayfuncs.c:4213:14: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
 4213 |   __pyx_v_L1 = ((&(*((double const  *) ( /* dim=1 */ (( /* dim=0 */ (__pyx_v_L.data + __pyx_t_1 * __pyx_v_L.strides[0]) ) + __pyx_t_2 * 

__pyx_v_L.strides[1]) )))) + (__pyx_v_go_out * __pyx_v_m));
      |              ^
sklearn/utils/arrayfuncs.c:4255:14: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
 4255 |   __pyx_v_L1 = ((&(*((double const  *) ( /* dim=1 */ (( /* dim=0 */ (__pyx_v_L.data + __pyx_t_2 * __pyx_v_L.strides[0]) ) + __pyx_t_1 * 

__pyx_v_L.strides[1]) )))) + (__pyx_v_go_out * __pyx_v_m));
      |              ^
gcc -pthread -shared build/temp.linux-x86_64-cpython-310/sklearn/utils/arrayfuncs.o -Lbuild/temp.linux-x86_64-cpython-310 -lm -llibsvm-skl -lliblinear-skl -o 

build/lib.linux-x86_64-cpython-310/sklearn/utils/arrayfuncs.cpython-310-x86_64-linux-gnu.so -fopenmp

Reason: In module sklearn.utils.arrayfuncs.pyx we have the function

def cholesky_delete(const floating[:, :] L, int go_out):

with argument L which is given as const.
This L is assigned to an array of floats floating *L1 in line 47:

L1 = &L[0, 0] + (go_out * m)

The generated C code misses a type cast (floating *) which leads to the warning.
In order to solve this one can either change L1 to const as well.
Since L1 gets further assignments this won't work.
Thus, instead the signature of cholesky_delete() is changed to:

def cholesky_delete(floating[:, :] L, int go_out):

After this fix we get:

building 'sklearn.utils.arrayfuncs' extension
gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -DNPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION -

I/usr/local/python/3.10.4/include/python3.10 -c sklearn/utils/arrayfuncs.c -o build/temp.linux-x86_64-cpython-310/sklearn/utils/arrayfuncs.o -g0 -O2 -fopenmp
gcc -pthread -shared build/temp.linux-x86_64-cpython-310/sklearn/utils/arrayfuncs.o -Lbuild/temp.linux-x86_64-cpython-310 -lm -llibsvm-skl -lliblinear-skl -o 

build/lib.linux-x86_64-cpython-310/sklearn/utils/arrayfuncs.cpython-310-x86_64-linux-gnu.so -fopenmp

Any other comments?

@github-actions GitHub Actions
Copy link

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: ec1e701. Link to the linter CI: here

@lesteve
Copy link
Member

lesteve commented Nov 13, 2023 •
edited
Loading

Thanks for your PR!

I am not a cython expert, but I would keep the signature like this since it is correct that L is const and is not modified.

There should be a way to do the equivalent of the (floating*) cast for L1. It's a bit weird you need it since L1 has has a non-const typedef but oh well ...

For context, t looks like the const was added in #25415, #25415 (comment). I did not find a strong reason for it.

@ogrisel
Copy link
Member

ogrisel commented Nov 13, 2023

/cc @jjerphan.

jjerphan
jjerphan approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Nice catch, this is a mistake and I am the culprit.

ogrisel
ogrisel approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Alright then. Let's merge :)

@ogrisel ogrisel merged commit 2137ee0 into scikit-learn:main Nov 13, 2023
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@ChVeen @REDVM
MAINT Remove -Wdiscarded-qualifiers compilation warning for sklearn.u…
4922cd2 
…tils.arrayfuncs (scikit-learn#27766)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@jjerphan jjerphan jjerphan approved these changes

Assignees
No one assigned
Labels
cython module:utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChVeen @lesteve @ogrisel @jjerphan