10000 MAINT Remove -Wcpp warnings when compiling sklearn.svm._libsvm by OmarManzoor · Pull Request #25064 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MAINT Remove -Wcpp warnings when compiling sklearn.svm._libsvm #25064

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 25 commits into from
Jan 3, 2023
Merged

MAINT Remove -Wcpp warnings when compiling sklearn.svm._libsvm #25064

merged 25 commits into from
Jan 3, 2023

Conversation

OmarManzoor
Copy link
Contributor

Reference Issues/PRs

Towards #24875

What does this implement/fix? Explain your changes.

  • Used memory views to replace cnp.ndarray
  • Casted the addresses of memory views to the appropriate types of pointers as expected by the libsvm functions
  • Applied some black formatting

Any other comments?

@jjerphan Since this file is a bit long I only handled the fit function to start with. I think once we can finalize the changes here we can apply a similar pattern to the other functions. This should hopefully make it easier to review.

jjerphan
jjerphan previously approved these changes Nov 29, 2022
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.

Thanks for splitting the removal of -Wcpp warnings in several steps, @OmarManzoor.

@jjerphan jjerphan dismissed their stale review November 30, 2022 07:51

CI errors needs to be resolved first.

@jjerphan jjerphan mentioned this pull request Nov 30, 2022
Closed
22 tasks
@OmarManzoor
Copy link
Contributor Author

@jjerphan I have formatted the code as suggested and also fixed the CI errors. Could you kindly have a look again?
Also is it fine if I adjust the rest of the functions in this same PR?

@jjerphan
Copy link
Member
jjerphan commented Dec 1, 2022

Also is it fine if I adjust the rest of the functions in this same PR?

It's fine and probably preferred. 👍

@jjerphan
Copy link
Member
jjerphan commented Dec 1, 2022

@OmarManzoor: can you please re-request a review when you are done? 🙂

@OmarManzoor
Copy link
Contributor Author

@OmarManzoor: can you please re-request a review when you are done? 🙂

Sure!

@jjerphan
Copy link
Member
jjerphan commented Dec 2, 2022

If you have problems for the run on Windows, do merge upstream/main in your branch to integrate #25091 which fixes those failures.

@OmarManzoor
Copy link
Contributor Author

If you have problems for the run on Windows, do merge upstream/main in your branch to integrate #25091 which fixes those failures.

Right thank you.

@OmarManzoor OmarManzoor requested a review from jjerphan December 2, 2022 14:14
jjerphan
jjerphan reviewed Dec 2, 2022
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.

A few last suggestions.

jjerphan
jjerphan approved these changes Dec 5, 2022
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.

LGTM. Thank you, @OmarManzoor.

@jjerphan jjerphan added the Waiting for Second Reviewer First reviewer is done, need a second one! label Dec 12, 2022
@OmarManzoor
Copy link
Contributor Author

@glemaitre Could you kindly have a look at this PR when you have the time?

@glemaitre glemaitre self-requested a review December 26, 2022 13:28
glemaitre
glemaitre reviewed Dec 27, 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.

Only nitpicks regarding the black style compliancy.

@glemaitre glemaitre removed the Waiting for Second Reviewer First reviewer is done, need a second one! label Dec 28, 2022
@glemaitre glemaitre merged commit 1c6e182 into scikit-learn:main Jan 3, 2023
@glemaitre
Copy link
Member

Thanks @OmarManzoor

@OmarManzoor OmarManzoor deleted the cython_libsvm branch January 3, 2023 09:25
jpangas pushed a commit to jpangas/scikit-learn that referenced this pull request Jan 4, 2023
@OmarManzoor @jpangas
MAINT Remove -Wcpp warnings when compiling sklearn.svm._libsvm (sciki…
c588dce 
…t-learn#25064)
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@OmarManzoor @jjerphan
MAINT Remove -Wcpp warnings when compiling sklearn.svm._libsvm (sciki…
31ee0c8 
…t-learn#25064)
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@OmarManzoor @jjerphan
MAINT Remove -Wcpp warnings when compiling sklearn.svm._libsvm (sciki…
81f4410 
…t-learn#25064)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjerphan jjerphan jjerphan approved these changes

@glemaitre glemaitre Awaiting requested review from glemaitre

Assignees
No one assigned
Labels
Build / CI cython module:svm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@OmarManzoor @jjerphan @glemaitre @jeremiedbb
0