Skip to content

Refactor load_metric to load #40

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
May 17, 2022
Merged

Refactor load_metric to load #40

merged 8 commits into from
May 17, 2022

Conversation

lvwerra
Copy link
Member

@lvwerra lvwerra commented May 13, 2022

This PR refactors the load_metric function to load and makes it module type agnostic as discussed in #38. The logic is as follows:

import evaluate

evaluate.load("mse") # -> works
evaluate.load("mse", type="metric") # -> also works
evaluate.load("mse", type="comparison") # -> throws an error

# e.g. perplexity can be a metric and measurement:
evaluate.load("perplexity") # -> loads metric (order is metric, comparison, measurement)
evaluate.load("perplexity", type="metric") # -> same as above
evaluate.load("perplexity", type="measurement") # -> loads measurement
evaluate.load("perplexity", type="comparison") # -> throws error

I'll add some more tests once we agree on the structure. There is also a pretty big commit renaming Metric -> EvaluationModule and similar changes but it affects a lot of file and might be a bit harder to review so I leave it for now and might add it at the end or in a separate PR.

What do you think?

@lvwerra lvwerra changed the title Refactor load Refactor load_metric to load May 13, 2022
@sashavor
Copy link
Contributor

I like this set up! Would metric, measurement and comparison all take similar inputs (i.e. dictionaries?) and output similar data structures (dictionaries too, I guess)?

@lvwerra
Copy link
Member Author

lvwerra commented May 13, 2022

Yes, the idea is that the others would work exactly as Metric metrics do now. I added a type attribute to the class so that we can differentiate what they should be but besides they work all the same way.

@sashavor
Copy link
Contributor

Cool! I think this will make using the whole library simpler for users 🚀
(we just have to do a good job documenting it and providing lots of examples! 🥳 )

douwekiela
douwekiela approved these changes May 14, 2022
View reviewed changes
Copy link
Contributor

@douwekiela douwekiela left a comment

Choose a reason for hiding this comment

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

Some minor nits, but LGTM!

lhoestq
lhoestq approved these changes May 16, 2022
View reviewed changes
Copy link
Member

@lhoestq lhoestq 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.

Thanks !

Feel free to update the load() docstring to mention the measurements directory and the comparisons directory

@lvwerra lvwerra merged commit 5b6db93 into main May 17, 2022
@lvwerra lvwerra mentioned this pull request May 17, 2022
Merged
@lvwerra lvwerra mentioned this pull request May 26, 2022
Closed
@lhoestq lhoestq mentioned this pull request Jun 20, 2022
Merged
@lvwerra lvwerra deleted the refactor-load branch July 24, 2022 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhoestq lhoestq lhoestq approved these changes

@sashavor sashavor Awaiting requested review from sashavor

+1 more reviewer

@douwekiela douwekiela douwekiela approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lvwerra @sashavor @douwekiela @lhoestq