Sentence CHRF silently accepts single reference as string

[bricksdont]

Hi Matt, all

While trying to debug an issue related to sentence-level CHRF, I noticed that the compat function sentence_chrf expects a list of references as input, but happily accepts a single string (a list of characters). But then only uses the first character as the reference:

>>> from sacrebleu import sentence_chrf
>>> sentence_chrf("hi there", "hi there")
chrF2 = 0.455
>>> sentence_chrf("hi there", "h")
chrF2 = 0.455

While the sentence_score function for BLEU intercepts strings instead of lists of strings:

sacrebleu/sacrebleu/metrics/bleu.py

Line 227 in 65747b6

assert not isinstance(references, str), "sentence_score needs a list of references, not a single string"

the one for CHRF does not:

sacrebleu/sacrebleu/metrics/chrf.py

Line 125 in 65747b6

def sentence_score(self, hypothesis: str, references: List[str]) -> CHRFScore:

Could this perhaps be changed / standardized for all metrics?

[ozancaglayan]

Thanks for reporting this. The PR synchronizes the behaviors across metrics.

[bricksdont]

Thanks!

A similar issue, I think, is that CHRF corpus_score expects List[Iterable[str]] as input, and silently accepts List[str] which will fail in zip_longest, but the error message will be confusing. In the edge case where the number of hypothesis sentences is equal to the number of references, it will even not fail at all and produce erroneous results.. I do not think that is true, sorry!

sacrebleu/sacrebleu/metrics/chrf.py

Line 136 in 65747b6

def corpus_score(self, sys_stream: Union[str, Iterable[str]],

If you agree, perhaps you could change this as well?

[ozancaglayan]

I did not understand the latter issue. The signature of corpus_chrf was synchronized from BLEU to share the common signature. Could you elaborate more?

I accept that having a compat API and also object-oriented API at the same time is kind of confusing, and the latter should be supported with more examples/docs and maybe unit tests to discover potential flaws & argument inconsistencies.

[bricksdont]

In this function, there is already code to "add some robustness to the input arguments":

sacrebleu/sacrebleu/metrics/chrf.py

Line 146 in 65747b6

# Add some robustness to the input arguments

The code intercepts strings as inputs to this function and makes them into lists. Could it also intercept / give an error message if the references argument is a simple list List[str] instead of the expected List[List[str]]?

If the input is List[str], i.e. a single reference where the user forgot to wrap in an extra [], then this function will zip the hypothesis tokens which one character each of the reference tokens, and the error message in zip_longest will be confusing (the streams were of different lengths, or similar).

Is that perhaps something worth changing as well?

Let me know if this is more clear.

[bricksdont]

Suggestion:

if isinstance(ref_streams[0], str):
        ref_streams = [ref_streams]

[ozancaglayan]

oh I see. As I am not the original author of those "robustness" parts, I wasn't able myself as well to clearly enumerate which scenarios are handled by these isinstance checks. I would actually suggest to remove all these stuff and select a single stable signature for corpus_* methods, and assume that the users obey to the documentation.

[bricksdont]

Agreed, I totally understand your reasoning.

[ozancaglayan]

You suggest the latter for all metrics, right?

[bricksdont]

Sorry, I somehow did not read your post a month ago closely enough (= my answer was a bit strange).

In principle yes, but for backwards compatibility that would be a problem I think and would not so much break existing code, as still work but silently produce the wrong results.

Edit: ah, you probably mean only corpus_* methods? There removing isinstance checks would perhaps throw errors in existing code that relies on them. SacreBLEU does have the concept of major versions that are backwards-incompatible right? If yes, then let's remove those checks.

[ozancaglayan]

not removing but, didn't you suggest adding the following as a 3rd check, to fix the issue?

if isinstance(ref_streams[0], str):
        ref_streams = [ref_streams]

[bricksdont]

Hm, I don't think I understand sorry :(

I thought you were saying that you would not want to add another of those checks, rather explain to users how to use the functions correctly?

(Yes, I did suggest this check for corpus CHRF and just checked - works the same way for all metrics, yes.)

[ozancaglayan]

sorry, as a quick fix, I wanted to incorporate your suggestion rather than the API fix that I've previously suggested. Could you also paste a small example to exploit the current issue so that we can add it to the tests?

[bricksdont]

Sure, sth like this?

>>> import sacrebleu
>>> hyp = "Es wird mit Gesamtkosten von 18.4 Millionen Franken gerechnet."
>>> ref = "Es werden Gesamtkosten von 18.4 Millionen Franken veranschlagt."
# correct
>>> sacrebleu.corpus_chrf(hyp, [[ref]])
chrF2 = 0.678
# incorrect, but there is an isinstance check for this
>>> sacrebleu.corpus_chrf(hyp, ref)
chrF2 = 0.678
# no check for this situation
>>> sacrebleu.corpus_chrf(hyp, [ref])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/net/cephfs/scratch/mathmu/map-volatility/venvs/sockeye3-cpu/lib/python3.6/site-packages/sacrebleu/compat.py", line 103, in corpus_chrf
    return metric.corpus_score(hypotheses, references)
  File "/net/cephfs/scratch/mathmu/map-volatility/venvs/sockeye3-cpu/lib/python3.6/site-packages/sacrebleu/metrics/chrf.py", line 158, in corpus_score
    raise EOFError("Source and reference streams have different lengths!")
EOFError: Source and reference streams have different lengths!