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

Official ROUGE added #74

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Official ROUGE added #74

wants to merge 10 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 30, 2019

To fix #68 and to have official scores (compared to #69), official ROUGE script is added (through pyrouge)

Dependencies

  • pyrouge package is used.
  • Perl is needed to run the official ROUGE script
  • libxml-parser-perl might be needed : see this issue

_I'm not sure how to add these dependencies in Travis, if someone could provide help here 👍 _

Notes

Implementation works great. However, a few notes :

  • If the ROUGE metric is called several time in a row, the official ROUGE script crash. To fix this, I reloaded scorers after calling compute_metrics. I think the performance are not impacted.
  • ROUGE official script does not work for the japanese test (but it works for the french one...). It's a problem in the official rouge script. In the test, I omit the ROUGE metric for this japanese example. We might need to add a mention of this in the README.
  • Finally, in the case where several candidates are given, I didn't compute the ROUGE metric for each of them : this would mean to write files and run the official script for each of them. This would be way too long. So I run the official script only once and retrieve only the average scores, not individual ones.

Licence

There is issue with licence : pyrouge didn't include the official script in their repo because the licence of the official script is unknown.

To make nlg-eval easy to use I already included the official script. My opinion is :

  • Anyway the official script is not available anymore, and people who want to use ROUGE need to download it from unofficial source.
  • Even if the licence is unknown, people use it everywhere without mentionning any licence. Keeping this separate just make it difficult for user.
  • As long as we are transparent and very clear on the README, this should not be an issue.

@msftclas
Copy link

msftclas commented Jun 30, 2019

CLA assistant check
All CLA requirements met.

@temporaer
Copy link
Member

Hey @astariul , thanks for the PR, this is great! Currently the tests fail because pyrouge isn't found. Could you add it to the dependencies in setup.py please?

@ghost
Copy link
Author

ghost commented Aug 12, 2019

I need help on this one ^^

@temporaer
Copy link
Member

temporaer commented Aug 12, 2019

setting aside that travis complains about a memory error, if I run the tests locally I get a permission denied for the ROUGE-1.5.5.pl script. Apparently, executable permissions aren't being set. This seems to be an issue in the pyrouge package. A workaround would be to subclass Rouge155 from pyrouge/Rouge155.py, and overload its evaluate() method by copying the function body and changing what was in line 333 to command = ["perl", self._bin_path] + options.

The next error that pops up is that the wordnet exceptions db file cannot be opened. It appears that this page suggests to [re]build this database to get things to work on windows. Our build server is on linux though, so that maybe the question is where the file is from?

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.

ROUGE 1 / ROUGE 2
3 participants