-
Notifications
You must be signed in to change notification settings - Fork 326
Add Gold Commodity News scenario #3065
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
Conversation
Hi @ryokawajp, please review this pull request. By approving, you acknowledge that:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for confirmation, this scenario "gold_commodity_news" corresponds to "news_headline" in the original source code, right?
Yes, this corresponds to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it locally.
news_headline is renamed to gold_commodity_news, and the values of category
option is changed to a style like price_or_not
from Price or Not
. I think these changes are reasonable.
I am not sure about test_ewok_scenario.py.
LGTM.
37c0439
to
b69bf75
Compare
Resolved the merge conflict - it was required to get an existing failing test to pass, but this has since been resolved by #3069. |
Gold commodity news is a classification benchmark based on a dataset of human-annotated gold commodity news headlines (Sinha & Khandait, 2019).