Task 8 - Stratified CIDEr Scores#8
Conversation
There was a problem hiding this comment.
@luistafoi if results are fine then comment "good to merge"
There was a problem hiding this comment.
@luistafoi if results are fine then comment "good to merge"
There was a problem hiding this comment.
@luistafoi if results are fine then comment "good to merge"
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Task 8 analysis artifacts to compare pre- vs post-fine-tune performance across models, stratified by topic and difficulty tier to address grant-reviewer concerns (C3/C4).
Changes:
- Adds scripts to compute per-sample metrics once and aggregate by topic/tier/split, persisting JSON outputs.
- Adds generation of a markdown report and topic×difficulty heatmaps from persisted per-sample JSONLs.
- Checks in generated report and aggregated JSON summaries.
Reviewed changes
Copilot reviewed 6 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| outputs/per_topic_finetune/analyze.py | Computes per-sample CIDEr/BLEU/ROUGE once per model/variant and emits stratified JSON outputs + per-sample JSONLs. |
| outputs/per_topic_finetune/make_report.py | Renders report.md from the stratified JSON outputs and adds basic anomaly checks + narrative. |
| outputs/per_topic_finetune/make_heatmap.py | Builds topic×difficulty heatmaps (Δ, base, FT) from persisted per-sample JSONLs. |
| outputs/per_topic_finetune/results/report.md | Generated markdown report summarizing headline, tier, and per-topic deltas plus interpretation. |
| outputs/per_topic_finetune/results/cider_per_split.json | Generated split-level metric summary (test only) per model/variant with deltas. |
| outputs/per_topic_finetune/results/cider_per_difficulty.json | Generated difficulty-tier metric summary per model/variant with deltas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GOLD = "/data/luis/ChemQA/dataset_gold.jsonl" | ||
| EVAL_ROOT = Path("/data/macaulay/ChemQA/ChemQA/eval/results_full") | ||
| MODELS = ["gemma3_12b", "llama3_1_8b", "qwen2_5_14b"] | ||
| VARIANTS = ["base", "ft"] | ||
| METRIC_KEYS = ["CIDEr", "BLEU-1", "BLEU-4", "ROUGE-L"] | ||
| OUT_DIR = Path(__file__).parent / "outputs" / "per_topic_finetune" | ||
| PER_SAMPLE_DIR = OUT_DIR / "per_sample" |
There was a problem hiding this comment.
OUT_DIR is constructed as Path(__file__).parent / 'outputs' / 'per_topic_finetune', but this file already lives in outputs/per_topic_finetune/, so the resulting path becomes outputs/per_topic_finetune/outputs/per_topic_finetune and will write/read outputs in the wrong location. Fix by setting OUT_DIR = Path(__file__).parent (or Path(__file__).resolve().parent) and keeping PER_SAMPLE_DIR = OUT_DIR / 'per_sample'.
| OUT_DIR = Path(__file__).parent / "outputs" / "per_topic_finetune" | ||
| MODELS = ["gemma3_12b", "llama3_1_8b", "qwen2_5_14b"] | ||
| TIERS = ["easy", "medium", "hard"] | ||
| MIN_N = 50 |
There was a problem hiding this comment.
Same path construction issue as in analyze.py: make_report.py already sits under outputs/per_topic_finetune/, so this OUT_DIR points to a nested/nonexistent directory and read_text() calls in main() will fail. Set OUT_DIR = Path(__file__).parent (or make it configurable) so it reads the JSONs that analyze.py actually writes.
| OUT_DIR = Path(__file__).parent / "outputs" / "per_topic_finetune" | ||
| PER_SAMPLE_DIR = OUT_DIR / "per_sample" |
There was a problem hiding this comment.
Same OUT_DIR path bug as the other scripts: this will look for per-sample JSONLs under outputs/per_topic_finetune/outputs/per_topic_finetune/per_sample, which likely doesn't exist. Set OUT_DIR = Path(__file__).parent so PER_SAMPLE_DIR resolves to the directory written by analyze.py.
| n = len(kept) | ||
| corpus = {"n": n} | ||
| if n == 0: | ||
| for k in METRIC_KEYS: | ||
| corpus[k] = None | ||
| return corpus, [] |
There was a problem hiding this comment.
score_corpus is annotated to return a 3-tuple (corpus_metrics, per_sample, kept_rows) and other call sites unpack 3 values, but the n == 0 path returns only two values. Return (corpus, [], []) here to keep the function contract consistent.
| m = meta.get(r["id"], {}) | ||
| r["_split"] = m.get("split") | ||
| r["_tier"] = difficulty_tier(m.get("num_evidence_sentences", 0)) | ||
| rows.append(r) |
There was a problem hiding this comment.
This code allows missing meta entries by defaulting to {}, which makes _split become None. Later, aggregate(..., lambda r: r['_split']) drops None keys, but the code asserts that per-split counts sum to n_total, which will fail if any _split is None. Either (a) require metadata to exist (e.g., raise if r['id'] not in meta), or (b) assign a concrete placeholder split like 'unknown' so per-split aggregation remains lossless.
| signs = {"+" if d > 0 else "-" if d < 0 else "0" for _, d in deltas} | ||
| if signs == {"+", "-"}: | ||
| msgs.append(f"- **Sign flip** on topic `{topic}` (n={n}): " |
There was a problem hiding this comment.
The sign-flip detection misses the case where one model has Δ=0 and others include both positive and negative (i.e., signs becomes {'+', '-', '0'}), which is still a sign inconsistency. Consider changing the condition to if '+' in signs and '-' in signs: so mixed-sign topics are always flagged.
Looking at Pre and Post fine tune performance of 3 different models.