fix metric_class names in avg_at_n / avg_at_n_math / maj_at_n test cases#1305
Open
jayzuccarelli wants to merge 1 commit into
Open
fix metric_class names in avg_at_n / avg_at_n_math / maj_at_n test cases#1305jayzuccarelli wants to merge 1 commit into
jayzuccarelli wants to merge 1 commit into
Conversation
The fixtures used avg_at_k / avg_at_k_math / maj_at_k, but the metrics are
registered as avg_at_n / avg_at_n_math / maj_at_n. Unknown names fall into the
"not available in METRIC_CLASSES" branch, which returns success: True, so 10
test cases have been reported as passing without ever calling the metric.
The names were not the only thing off: metric_params used {"k": N} where the
constructors take n, and expected_output keyed on avg@k:k=N instead of
avg@n:n=N. Renaming metric_class alone just gets you "You did not set the
value of n", so all three are fixed here, and the files are renamed to match
their metric like the rest of test_cases/.
An unresolvable metric_class now raises instead of silently skipping, unless
it is listed in SKIPPED_METRICS. That is what let this go unnoticed.
maj_at_n is added to SKIPPED_METRICS for now: once enabled it fails with
IndexError in Doc.get_golds, because its first case has gold_index [1] against
a single choice. That is the bug PR huggingface#1274 fixes, so it can be unskipped once
that lands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1304.
Summary
tests/unit/metrics/test_cases/avg_at_k.json,avg_at_k_math.jsonandmaj_at_k.jsonsetmetric_classtoavg_at_k/avg_at_k_math/maj_at_k, but those metrics are registered asavg_at_n/avg_at_n_math/maj_at_n(metrics.py:154,:161,:434).An unresolvable name falls into the
not available in METRIC_CLASSESbranch ofrun_test_case, which returns"success": True.test_automated_metrics_pytest.pyfilters skips out offailed_testsand only prints them, so all 10 cases have been reporting as passing without ever callingcompute():Not just a rename
Fixing
metric_classalone givesException: You did not set the value of n. Two more things were wrong in each fixture:metric_paramsused{"k": N}, but the constructors areAvgAtN(n=...)/MajAtN(n=...).Metric.__call__setattrs whatever key it is given, so{"k": 3}set a dead.kand left.nasNone.expected_outputkeyed onavg@k:k=N, but the key isf"{metric_name}:{params}", so it needsavg@n:n=N.Files are renamed to match their metric, consistent with the rest of
test_cases/.Guard against a repeat
An unresolvable
metric_classnow raises instead of silently skipping, unless it is listed inSKIPPED_METRICS. That silent branch is what let this sit unnoticed.maj_at_n is skipped for now
With the names fixed,
avg_at_nandavg_at_n_mathpass (6 cases).maj_at_nfails:Its first case has
gold_index: [1]against a single-elementchoices, which is the bug #1274 fixes. Rather than land red CI,maj_at_nis added toSKIPPED_METRICSwith a pointer to that PR; it can be unskipped once #1274 merges. Happy to drop the skip and rebase on #1274 instead if you would rather they land together.Testing
tests/unit/metrics/is green: 46 passed. The one remaining failure,extractiveness.json, fails identically on unmodifiedmain(missing optionalsumm_evaldependency) and is unrelated.ruff checkandruff format --checkare clean.Note:
.gitattributesmarkstests/unit/metrics/test_cases/*.jsonas-diff, so GitHub renders these three as binary. The changes are thektonsubstitutions described above onmetric_class,metric_paramsand theexpected_outputkeys, plus the human-readablename/descriptionstrings.