MAINT Remove -Wsign-compare when compiling sklearn.linear_model._cd_fast #24895
Conversation
…learn.linear_model._cd_fast
|
@jjerphan For the warning https://gist.github.com/jjerphan/8cfbb5349f5680856460ff80152ab69d#file-full-trace-log-L429 |
There was a problem hiding this comment.
Thanks @OmarManzoor for this PR and the observation you made.
I think it is preferable to restrain the scope of this PR to only remove -Wsign-compare warnings for two reasons:
- this PR addresses two goals at the moment (removal of two kind of warning). For scikit-learn, we prefer having independent and atomic goals (and changes) is more readable and understanble in logs, and is more easily reviewable.
- all the
-Woverflowwarnings when compiling the whole code-base are coming from the currentinlinesklearn.utils._random.our_rand_rfunction, and their removal might better be addressed in a dedicated PR.
Can you please restrain the scope of this PR to the removal of -Wsign-compare warnings solely? Thank you.
Here is a comment to address it better.
sklearn/linear_model/_cd_fast.pyx
Outdated
| @@ -171,7 +171,7 @@ def enet_coordinate_descent( | |||
| # tol *= np.dot(y, y) | |||
| tol *= _dot(n_samples, &y[0], 1, &y[0], 1) | |||
|
|
|||
| for n_iter in range(max_iter): | |||
| for n_iter in range(<Py_ssize_t>max_iter): | |||
There was a problem hiding this comment.
I think it is preferable to change the type of n_iter to the one of max_iter (i.e. int) instead of casting max_iter to Py_ssize_t here.
There was a problem hiding this comment.
As for SGD, max_iter will be validated by the Python class to be strictly positive.
In terms of semantic, it is more correct to use unsigned and the casting will be safe.
It can be done once for all in the signature of the function.
This removes the -Woverflow warnings observed
when building scikit-learn.
RAND_R_MAX is the max value for uint8, incrementing
it causes an overflow (hence the warning).
I think this commit fixes the implementation, yet
I comes with a backwards incompatible results and
tests for implementation relying on `our_rand_r`
fails because results are now different.
I see several alternatives to remove the warning while
having tests pass
- prefered solution: adapt the test suite using the
new results so that all tests pass and ackowledge
the change of behavior for impacted user-facing APIs
in the changelog
- accept the quirk of this implementation but hardcode
and rename the effective constant
- silent the -Woverflow warning by another mean
Relates to:
scikit-learn#13422
scikit-learn#24895
This removes the -Woverflow warnings observed when building scikit-learn. RAND_R_MAX is the max value for uint8, incrementing it causes an overflow (hence the warning). Elements were originaly mentionned in scikit-learn#13422 (comment) but left unreviewed, it seems. I think this commit fixes the implementation, yet I comes with a backwards incompatible results and tests for implementation relying on `our_rand_r` fails because results are now different. I see several alternatives to remove the warning while having tests pass - prefered solution: adapt the test suite using the new results so that all tests pass and ackowledge the change of behavior for impacted user-facing APIs in the changelog - accept the quirk of this implementation but hardcode and rename the effective constant - silent the -Woverflow warning by another mean Relates to: scikit-learn#13422 scikit-learn#24895
This removes the -Woverflow warnings observed when building scikit-learn. RAND_R_MAX is the max value for uint8, incrementing it causes an overflow (hence the warning). Elements were originally mentioned but seem to have been left unreviewed, see: scikit-learn#13422 (comment) I think this commit fixes the implementation, yet I comes with a backwards incompatible results and tests for implementation relying on `our_rand_r` fails because results are now different. I see several alternatives to remove the warning while having tests pass - prefered solution: adapt the test suite using the new results so that all tests pass and ackowledge the change of behavior for impacted user-facing APIs in the changelog - accept the quirk of this implementation but hardcode and rename the effective constant - silent the -Woverflow warning by another mean Relates to: scikit-learn#13422 scikit-learn#24895
This removes the -Woverflow warnings observed when building scikit-learn. RAND_R_MAX is the max value for uint8, incrementing it causes an overflow (hence the warning). Elements were originally mentioned but seem to have been left unreviewed, see: scikit-learn#13422 (comment) I think this commit fixes the implementation, yet I comes with a backwards incompatible results and tests for implementation relying on `our_rand_r` fails because results are now different. I see several alternatives to remove the warning while having tests pass - preferred solution: adapt the test suite using the new results so that all tests pass and acknowledge the change of behavior for impacted user-facing APIs in the change-log - accept the quirk of this implementation but hardcode and rename the effective constant - silent the -Woverflow warning by another mean Relates to: scikit-learn#13422 scikit-learn#24895
|
#24919 has been created to treat |
|
@jjerphan, @glemaitre |
|
Thanks for mentioning that. I think we better remove |
|
@OmarManzoor: I meant that it's better to integrate changes made in 5e66af3 in another PR (one removing |
Oh I understand. However I don't think we will be able to remove the -Wcpp warning from this module completely because the const fused types complication. I think most of the other applicable instances already use memory views? |
|
We can try to use fused-typed memoryview which aren't |
Reference Issues/PRs
Towards #24875
What does this implement/fix? Explain your changes.
Any other comments?