Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Rouge #103

Closed
wants to merge 10 commits into from
Closed

Rouge #103

wants to merge 10 commits into from

Conversation

YuhengHuang42
Copy link

Related to issue #6
implement ROUGE-N, ROUGE-W, ROUGE-L

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020

Codecov Report

Merging #103 into master will decrease coverage by 0.77%.
The diff coverage is 86.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   94.41%   93.63%   -0.78%     
==========================================
  Files          64       65       +1     
  Lines        1611     1776     +165     
==========================================
+ Hits         1521     1663     +142     
- Misses         90      113      +23     
Impacted Files Coverage Δ
torchnlp/word_to_vector/fast_text.py 100.00% <ø> (ø)
torchnlp/metrics/rouge.py 86.06% <86.06%> (ø)
torchnlp/metrics/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cde86ba...6d60aa8. Read the comment docs.

Copy link
Owner

@PetrochukM PetrochukM left a comment

Choose a reason for hiding this comment

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

Thanks for the large contribution! With such a large contribution, there is a large responsibility to write maintainable and correct code.

Can you please take more time to ensure the code is readable and tested?

Thanks again!

class Ngrams(object):
"""
datastructure for n grams.
if `exclusive`, datastructure is set
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please formally define Args?

return ngram_set


def _frp_rouge_n(eval_count, ref_count, overlapping_count):
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be factored out as a generic f1 score utility function?

p_lcs = llcs / n
if not beta:
beta = p_lcs / (r_lcs + 1e-12)
num = (1 + (beta**2)) * r_lcs * p_lcs
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have a paper or link to learn more about this algorithm?



def get_rouge_w(evaluated_sentence, reference_sentence,
f=lambda x: x**2, inv_f=lambda x: math.sqrt(x)):
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
f=lambda x: x**2, inv_f=lambda x: math.sqrt(x)):
f=lambda x: x**2, inv_f=math.sqrt):

Computes ROUGE-W of two sequences, namely evaluated_sentence and reference sentece
Reference: https://www.aclweb.org/anthology/W04-1013.pdf
Args:
evaluated_sentence: a sentence that have been produced by the summarizer
Copy link
Owner

Choose a reason for hiding this comment

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

Can you make sure all the arguments are defined, including the optional arguments?

return lcs_seq_wrd(len(x), len(y))


def _w_lcs(x, y, func=lambda x: x**2):
Copy link
Owner

Choose a reason for hiding this comment

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

This algorithm is fairly complex. Can you provide a full test suite to make sure it's correct?

dictionary. WLCS-based F-measure score, P-score and R-score
"""
if strict:
assert(check_increase(f))
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for adding invariant checks!

Returns:
bool, if f(x + y) > f(x) + f(y) or not
"""
for i in range(100000):
Copy link
Owner

Choose a reason for hiding this comment

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

Should this constant (10000) be parameterized?

return True


def _frp_rouge_w(wlcs, m, n, f=lambda x: x**2, inv_f=lambda x: math.sqrt(x),
Copy link
Owner

Choose a reason for hiding this comment

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

Do you need to repeat these lambdas a second time? I'm thinking about the principles of DRY.

n: number of words in candidate summary
f: weighting function
inv_f: inverse function of weighting function
beta: beta = P_lcs / R_lcs when ∂ F_lcs / ∂ R_lcs = ∂ F_lcs / ∂ P_lcs.
Copy link
Owner

Choose a reason for hiding this comment

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

strict is not defined :(

@YuhengHuang42
Copy link
Author

YuhengHuang42 commented Jul 4, 2020

Thanks for the large contribution! With such a large contribution, there is a large responsibility to write maintainable and correct code.

Can you please take more time to ensure the code is readable and tested?

Thanks again!

Sorry for the inconvenience, I should have mentioned that the codes here are mainly for learning the project, and so I didn't consider making it maintainable.
Some undefined variable may happen because of merge problem.

I should take the responsibility here. However, I am currently not available to rewrite the codes and do the testing. And I can not promise to do it in the future. I will try to fix the problems when I am available.
Still, I will list some of the references so that someone interested in this project can help.

Rouge. This implementation is more complete than tensorflow one.

What is ROUGE and how it works for evaluation of
summarization tasks?
. I referenced this documentation for ROUGE-N. And tested for the examples there.

My friend may give more paper references, since ROUGE-L and ROUGE-W are implemented by him (@ST-Saint)

@PetrochukM
Copy link
Owner

:) No problem!

Do you feel like you learned a lot?

If you'd like, I can close the PR and you don't have to worry about fixing the implementation.

@YuhengHuang42
Copy link
Author

:) No problem!

Do you feel like you learned a lot?

If you'd like, I can close the PR and you don't have to worry about fixing the implementation.

Yes, sorry for the inconvenience again. I will open another PR if I rewrite the codes and do the testing. I shouldn't bother you mainly for the learning. I hope I can contribute to the project in the future. Thanks a lot!

@PetrochukM
Copy link
Owner

I'm happy to help! Thanks for contributing!

@PetrochukM PetrochukM closed this Jul 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants