feat(text-metrics): split qa_accuracy#645
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 15db155. Configure here.
| "between prompts and generated images via VQA-style questions." | ||
| ), | ||
| metrics=["clip_score"], # §3.2: Mask2Former; not in Pruna | ||
| metrics=["qa_accuracy", "clip_score"], # strict QA + CLIP score |
There was a problem hiding this comment.
GenEval benchmark uses lenient mean aggregation
Medium Severity
The GenEval benchmark's qa_accuracy metric is intended for "strict QA" (all-or-nothing aggregation). However, Task.from_benchmark doesn't pass the all_or_nothing kwarg, causing QAAccuracyMetric to default to mean aggregation. This leads to inflated scores that are not comparable to the paper's reference.
Reviewed by Cursor Bugbot for commit 15db155. Configure here.
| raise ValueError( | ||
| f"qa_accuracy aggregation must be one of {{'mean', 'all_or_nothing'}}. Got: {self.aggregation!r}." | ||
| ) | ||
| self.metric_units = type(self).metric_units |
There was a problem hiding this comment.
Redundant metric_units self-assignment in __init__
Low Severity
Setting self.metric_units = type(self).metric_units is a no-op because metric_units is already declared as a class attribute and nothing earlier in __init__ (neither super().__init__ nor the surrounding code) overrides it. The line just shadows the class attribute with the same value and adds maintenance noise.
Reviewed by Cursor Bugbot for commit 15db155. Configure here.
davidberenstein1957
force-pushed
the
feat/vlm-pr-2-infrastructure
branch
from
21212de to
7054e53
Compare
davidberenstein1957
force-pushed
the
feat/vlm-pr-3a-qa-accuracy
branch
2 times, most recently
from
04ab2e5 to
161223e
Compare
|
This PR has been inactive for 10 days and is now marked as stale. |
Isolates qa_accuracy metric implementation and GenEval benchmark wiring so it can be reviewed independently before stacking the remaining text metrics. Made-with: Cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
Remove redundant metric_units assignment; aggregation is keyword-only. Co-authored-by: Cursor <cursoragent@cursor.com>
davidberenstein1957
force-pushed
the
feat/vlm-pr-3a-qa-accuracy
branch
from
161223e to
e7fca2e
Compare
|
Follow-up: |
Drop the broken Intel uv index (aligned with main), fix QAAccuracy keyword-only aggregation syntax, pass single/y_gt call types correctly for OneIG alignment, and expose metric_units on results. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace forward-import VLM test module on pre-e2e branches with infrastructure-only tests; propagate docstring and conftest fixes. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Remove verify helper and duplicate infra test template from scripts/; tests live under tests/evaluation/ only. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Splits
qa_accuracyinto its own stacked PR, addsQAAccuracyMetric, and wires GenEval toqa_accuracy + clip_score.Stack Position
feat/vlm-pr-2-infrastructure)feat/vlm-pr-3b-oneig-alignment)feat/vlm-pr-5-e2e-tests)feat/metrics-vlm-support)Files
src/pruna/evaluation/metrics/metric_qa_accuracy.pysrc/pruna/evaluation/benchmarks.pyTest Plan
Review Focus
all_or_nothing)Review Flow (Order)
Review the stack in this exact order:
This PR in the flow (3/10)