Skip to content

Add Evaluate usage for scikit-learn#368

Merged
lvwerra merged 9 commits into
huggingface:mainfrom
awinml:metrics_sklearn_integration
Nov 29, 2022
Merged

Add Evaluate usage for scikit-learn#368
lvwerra merged 9 commits into
huggingface:mainfrom
awinml:metrics_sklearn_integration

Conversation

@awinml

@awinml awinml commented Nov 19, 2022

Copy link
Copy Markdown
Contributor

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

HuggingFaceDocBuilderDev commented Nov 24, 2022

Copy link
Copy Markdown

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

@osanseviero
osanseviero requested a review from lvwerra November 25, 2022 09:46

@lvwerra lvwerra left a comment

Copy link
Copy Markdown
Member

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.

Comment thread docs/source/sklearn_integrations.mdx Outdated
Comment thread docs/source/sklearn_integrations.mdx Outdated
Comment thread docs/source/sklearn_integrations.mdx Outdated
Comment thread docs/source/sklearn_integrations.mdx Outdated
Comment thread docs/source/sklearn_integrations.mdx Outdated
@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

@lvwerra lvwerra left a comment

Copy link
Copy Markdown
Member

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.

Comment thread docs/source/sklearn_integrations.mdx Outdated
Comment thread docs/source/sklearn_integrations.mdx Outdated
@awinml

awinml commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

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
Comment thread docs/source/sklearn_integrations.mdx Outdated

@lvwerra lvwerra left a comment

Copy link
Copy Markdown
Member

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

awinml commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

I have added the actual value for the accuracy.

@awinml
awinml requested a review from lvwerra November 29, 2022 12:47
@awinml

awinml commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

@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

lvwerra commented Nov 29, 2022

Copy link
Copy Markdown
Member

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
awinml force-pushed the metrics_sklearn_integration branch from c7afba9 to da5dd2c Compare November 29, 2022 16:33
@awinml

awinml commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

@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

lvwerra commented Nov 29, 2022

Copy link
Copy Markdown
Member

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