MAINT Remove -Wcpp warnings when compiling sklearn.linear_model._sgd_fast #24883
Conversation
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
So it is just missing the casting to not compare signed and unsigned int.
|
@jjerphan Should we cast the loop variables to int here as well instead of casting the max_iter? |
jjerphan
left a comment
jjerphan
left a comment
There was a problem hiding this comment.
Thanks for this series of PR, @OmarManzoor!
Here are a few comments before approval.
* Used const with memory views in arguments that aren't expected to be modified * Used memory view's base attribute to return the underlying numpy array * Changed the type of epoch to int * Removed some unused variables
jjerphan
left a comment
jjerphan
left a comment
There was a problem hiding this comment.
LGTM given that CI is green.
Thanks, @OmarManzoor!
|
|
||
|
|
||
| cdef bint any_nonfinite(double *w, int n) nogil: | ||
| cdef bint any_nonfinite(const double *w, int n) nogil: |
There was a problem hiding this comment.
@jjerphan do you think that we should delay the use of const and only restrict to the memoryview when changing it? I know it is a good practice, but we have that many variables that are not defined as const (and pointers), I assume it would be best to have a dedicated PR for a specific function or file.
There was a problem hiding this comment.
This changes is necessary for removing the warning (i.e. not using the old NumPy API hence not using cnp.ndarray hence using memoryviews instead) while supporting readonly/memmaped data since the pointer references the buffer of a const-qualified memoryview. I do not think we can get our way around the modification here without casting const- to non-const-qualified variable.
But I agree generally and also think that we must not change signatures to add const-qualifier if they aren't required for removing compilation warnings.
What do you think?
There was a problem hiding this comment.
This changes is necessary for removing the warning
OK I missed it. I thought it was introduced in the last commit. My bad.
|
Merging then. Thanks @OmarManzoor |
3 participants
Reference Issues/PRs
Towards #24875
What does this implement/fix? Explain your changes.
Any other comments?