Skip to content

MAINT remove -Wsign-compare when compiling sklearn.utils.sparsefuncs_fast #25244

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 8 commits into from
Jan 5, 2023
Merged

MAINT remove -Wsign-compare when compiling sklearn.utils.sparsefuncs_fast #25244

merged 8 commits into from
Jan 5, 2023
+57 −43

Conversation

Vincent-Maladiere
Copy link
Contributor

@Vincent-Maladiere Vincent-Maladiere commented Dec 28, 2022 •
edited
Loading

Reference Issues/PRs

Towards #24875
Useful reads: #9663, #24426 (comment)

What does this implement/fix? Explain your changes.

Replace some indices types with their range value type.

Any other comments?

Remove 41 -Wsign-compare warnings :)

@glemaitre glemaitre self-requested a review December 29, 2022 10:30
glemaitre
glemaitre approved these changes Dec 29, 2022
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

sklearn/utils/sparsefuncs_fast.pyx Outdated
unsigned long long col_ind
integral row_ind
integral i, row_ind
unsigned long long k, col_ind
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
unsigned long long k, col_ind
unsigned long long feature_idx, col_ind

jjerphan
jjerphan approved these changes Jan 2, 2023
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you, @Vincent-Maladiere!

I do not observe such -Wsign-compare warnings for this compilation unit with my compiler (gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)). Are you using LLVM via clang?

I appreciate the renaming of k for feature_idx which is a more informative. Yet, I think it extends the scope of this PR and makes the diff harder to read with respect to the change of logic (which is be small and should rather be transparent). Moreover, I think this renaming is partial here and might preferably be done once entirely.

Hence would prefer having this renaming be done in another dedicated MAINT pull request.

What do you think?

Apart from those two remarks, this contribution LGTM.

@Vincent-Maladiere
Copy link
Contributor Author

Hi @jjerphan, thank you for the review! Yes, I'm using LLVM via clang on macOS. I wasn't aware there could be some differences regarding compilation warnings.
I agree that the renaming creates some distraction compared to the purpose of this PR. If this suggestion suits @glemaitre, I can apply renaming in another PR.

@glemaitre
Copy link
Member

Hence would prefer having this renaming be done in another dedicated MAINT pull request.

I would not do that. We already change the name of the variable from i to k so why make a new PR to change from k to feature_idx?

Note that I limit the proposal to changing the variable only to the already modified variable.

@jjerphan
Copy link
Member

jjerphan commented Jan 3, 2023

I think that this renaming is out of the scope of this PR and makes the implementation of the intend harder to understand. What do you think?

@glemaitre
Copy link
Member

I still think that you are renaming i to k but whatever.

@jjerphan
Copy link
Member

jjerphan commented Jan 3, 2023

Ah, I was mentioning the renaming of k to feature_idx but I did not mean any renaming should be perform (I think no renaming should be performed for such PRs removing warnings not to obfuscate the logic). Are those explanations clearer?

@jeremiedbb
Copy link
Member

jeremiedbb commented Jan 3, 2023 •
edited
Loading

I think you're not on the same page :)
@glemaitre says that before his remark to change k into feature_idx, the PR was already changing ì into k in the same places. So I agree with him that we should either do the good renaming in this PR or no renaming at all.

@Vincent-Maladiere
Copy link
Contributor Author

So I agree with him that we should either do the good renaming in this PR or no renaming at all.

I also agree. To clarify the situation, I initially added the variable k because we were comparing i to variables with different types. For all impacted functions, the variable i remains, but k has replaced it for some comparisons.

@jjerphan
Copy link
Member

jjerphan commented Jan 3, 2023

#25244 (comment) is the comment I have not understood: previously there was a renaming of i to k, but currently there isn't it, right?

So I agree with him that we should either do the good renaming in this PR or no renaming at all.

That's what I understood, and I think we should not do renaming at all. Anyway, I let @glemaitre when this LGTH, I do not want to bikeshed too much and block the removal of warnings.

@jjerphan
Copy link
Member

jjerphan commented Jan 5, 2023

Merging because @glemaitre and I agree on the current state. Thanks, @Vincent-Maladiere.

@jjerphan jjerphan merged commit e0a29df into scikit-learn:main Jan 5, 2023
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@Vincent-Maladiere @jjerphan
MAINT remove -Wsign-compare when compiling `sklearn.utils.sparsefuncs…
1ca6825 
…_fast` (scikit-learn#25244)
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@Vincent-Maladiere @jjerphan
MAINT remove -Wsign-compare when compiling `sklearn.utils.sparsefuncs…
029c25d 
…_fast` (scikit-learn#25244)
@jjerphan jjerphan mentioned this pull request Feb 28, 2023
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre 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
@Vincent-Maladiere @glemaitre @jjerphan @jeremiedbb