-
Notifications
You must be signed in to change notification settings - Fork 224
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
Fix calculation error when ref is empty #92
base: master
Are you sure you want to change the base?
Conversation
Thanks for pointing this out. The references are the targets that the generated hypothesis should match. It's possible that a target would indeed be an empty string so I think we should correct what is actually causing the error instead of silently ignoring empty strings which could mean that a hypothesis is compared with a target that it wasn't meant to be compared with. For example: References:
Hypotheses:
|
I agree that we should correct what is actually causing the error
vectorize metric(Skip-thought/ glove_metrics) will cause error due to empty input to encode |
add exception for install_reqs
.gitignore
Outdated
@@ -22,3 +22,30 @@ glove2word2vec.py | |||
|
|||
.idea/ | |||
venv/ | |||
|
|||
# General | |||
.DS_Store |
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.
Can you remove changes to your gitignore? They seem rather specific to your setup.
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.
Ok I can remove it.
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.
BTW You can also create a global gitignore: https://docs.github.com/en/github/getting-started-with-github/ignoring-files#configuring-ignored-files-for-all-repositories-on-your-computer
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.
@juharris Thank you for your advice. It is what I need.
nlgeval/tests/test_nlgeval.py
Outdated
|
||
scores = n.compute_individual_metrics(ref=["Chocolate and an Interview "], | ||
hyp="Chocolate , a Healthy Food") | ||
print(scores) |
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.
Can you assert something here, so that tests can detect regressions?
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 found that this case is not necessary. I will remove it
nlgeval/tests/test_nlgeval.py
Outdated
hypothesis = os.path.join(root_dir, 'examples/6.csv') | ||
references = os.path.join(root_dir, 'examples/7.pt_beam_1.csv') | ||
scores = nlgeval.compute_metrics(hypothesis, [references]) | ||
print(scores) |
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.
Same, better to assert the result you expect.
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 found that this case is not necessary. I will remove it
This pull request fixing the issue that any of the ref is empty
When the number of ref is inconsistent, i will fill a empty string as padding. which causing an error.