Skip to content
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.

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

Adding sklearn logreg example #748

Merged
merged 5 commits into from
Jun 30, 2021
Merged

Adding sklearn logreg example #748

merged 5 commits into from
Jun 30, 2021

Conversation

cozek
Copy link
Contributor

@cozek cozek commented Jun 6, 2021

Adding a scikit-learn logistic regression example using MNIST dataset.
The openml helper in scikit-learn is broken, so I used the openml python library directly.
I tried to make this as simple as possible following the quickstart examples but it turned out little complicated since model initialisation and parameter setting is a not so obvious with scikit-learn.

@danieljanes
Copy link
Member

@cozek thanks, this is amazing! We wanted to add a scikit-learn example for quite some time, thanks for the initiative. I'll try to review it soon, just wanted to mention that we should advertise this (along with the other frameworks) in the README.md.

@cozek
Copy link
Contributor Author

cozek commented Jun 9, 2021

@cozek thanks, this is amazing! We wanted to add a scikit-learn example for quite some time, thanks for the initiative. I'll try to review it soon, just wanted to mention that we should advertise this (along with the other frameworks) in the README.md.

Thanks!

Although I took additional care to ensure the federation and learning are working correctly but I might have missed something. I would like to ask you to check the correctness of the recipe.

@cozek
Copy link
Contributor Author

cozek commented Jun 24, 2021

@danieljanes Hi, have you had a chance to go through the code ? 😄

Copy link
Member

@danieljanes danieljanes left a comment

Choose a reason for hiding this comment

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

Hi @cozek, sorry for the delay. This PR looks pretty good overall, thanks again! I left a couple of small comments and I'll also run it on my machine before merging. You could also add a reference to this example in the main README.md, I think our SciKit users will be excited about it 😃

examples/sklearn-logreg-mnist/README.md Outdated Show resolved Hide resolved
examples/sklearn-logreg-mnist/client.py Outdated Show resolved Hide resolved
examples/sklearn-logreg-mnist/utils.py Outdated Show resolved Hide resolved
@danieljanes
Copy link
Member

Thanks for the changes @cozek. Results from a quick test run look good, seems like we can merge 👍

@danieljanes danieljanes merged commit 570788c into adap:main Jun 30, 2021
@cozek cozek deleted the sklearn-mnist branch January 3, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants