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

Add Evaluate usage for scikit-learn #368

Merged
merged 9 commits into from
Nov 29, 2022

Conversation

awinml
Copy link
Contributor

@awinml awinml commented Nov 19, 2022

Reference Issues/PRs

Fixes #326

What does this implement/fix?

Adds an example demonstrating the integration of evaluate with the scikit-learn framework.

Note:

Instead of creating a section in metrics_integrations.mdx, I have decided to create sklearn_integrations.mdx so that it is consistent with the previous section on Transformers.

cc @lvwerra @osanseviero

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 24, 2022

The documentation is not available anymore as the PR was closed or merged.

Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

Hi @awinml, this looks great - thanks for adding! Left a few minor comments.

docs/source/sklearn_integrations.mdx Outdated Show resolved Hide resolved
docs/source/sklearn_integrations.mdx Outdated Show resolved Hide resolved
docs/source/sklearn_integrations.mdx Outdated Show resolved Hide resolved
docs/source/sklearn_integrations.mdx Outdated Show resolved Hide resolved
docs/source/sklearn_integrations.mdx Outdated Show resolved Hide resolved
@awinml awinml requested a review from lvwerra November 25, 2022 17:31
@awinml awinml changed the title [WIP] Add Evaluate usage in scikit-learn Add Evaluate usage in scikit-learn Nov 25, 2022
@awinml awinml changed the title Add Evaluate usage in scikit-learn Add Evaluate usage for scikit-learn Nov 25, 2022
Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

Two nits and then we can merge :) Something is a bit off with the PR since it also shows changes to some previous changes. Looking at the history it seems like there have been multiple branches starting in different points of the git history that got merged for this PR. I think it's just git that's confused but and when we merge it should be ok.

docs/source/sklearn_integrations.mdx Outdated Show resolved Hide resolved
docs/source/sklearn_integrations.mdx Outdated Show resolved Hide resolved
@awinml
Copy link
Contributor Author

awinml commented Nov 29, 2022

Two nits and then we can merge :) Something is a bit off with the PR since it also shows changes to some previous changes. Looking at the history it seems like there have been multiple branches starting in different points of the git history that got merged for this PR. I think it's just git that's confused but and when we merge it should be ok.

I rebased the merge with master by mistake. So it pulled in all those commits.
I chose not to remove them individually, because it would make the PR extremely messy.
It should be ok when we merge.

@awinml awinml requested a review from lvwerra November 29, 2022 10:02
Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

Just one minor comment left then we can merge. Let's add this and wait for the CI to get green to merge.

@awinml
Copy link
Contributor Author

awinml commented Nov 29, 2022

I have added the actual value for the accuracy.

@awinml awinml requested a review from lvwerra November 29, 2022 12:47
@awinml
Copy link
Contributor Author

awinml commented Nov 29, 2022

@lvwerra I reverted the incorrect commits and merged with main. The CI tests fail on unrelated tests. Do you have any ideas on how to fix it?

@lvwerra
Copy link
Member

lvwerra commented Nov 29, 2022

You should not revert the changes. Ideally you just merge main from evaluate into your branch. There was a fix for the error in the CI on main.

awinml and others added 8 commits November 29, 2022 21:47
Co-authored-by: Leandro von Werra <lvwerra@users.noreply.github.com>
Co-authored-by: Leandro von Werra <lvwerra@users.noreply.github.com>
Co-authored-by: Leandro von Werra <lvwerra@users.noreply.github.com>
Co-authored-by: Leandro von Werra <lvwerra@users.noreply.github.com>
@awinml
Copy link
Contributor Author

awinml commented Nov 29, 2022

@lvwerra I am extremely sorry for the messy commit history. I did a complete rebase and cherry picked all the correct commits.
A merge from main fixed all the errors and the CI passed successfully. Thank you for your help!

@lvwerra
Copy link
Member

lvwerra commented Nov 29, 2022

No worries, that's why we squash :) Thanks for your contribution!

@lvwerra lvwerra merged commit 23b255e into huggingface:main Nov 29, 2022
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.

Evaluate usage in other frameworks
3 participants