From ddff43b79c0b20a9b0e05cef8154ccdad31f45c1 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 16:55:16 +0000 Subject: [PATCH] Explain the statistics with the reader's own numbers Spec 011 Phase 5. Fragility is computed in code rather than asked for in the prompt. A hit resting on fewer than five matched entities is flagged per pathway, because a model handed a tiny p-value and a tiny count describes the p-value -- which is the specific mistake the spec's rationale names: "a pathway with 2 of 3 entities found is not strong evidence". The threshold is on the count, not the ratio, and that is the whole point. 2 of 3 looks like a perfect hit by ratio and is nearly meaningless; 40 of 200 looks poor and is real evidence. The ratio is the misleading number here, so the flag ignores it. Pinned by a test using both. The instruction also fixes what the significance language means: `significant` in the data is after correction, and a small p-value that does not survive correction is not a finding. Two things from running it against a real result rather than a stub. It worked -- the summary names the actual counts, "2 found out of 4 total", which is what US3 asks for -- but it wrote "these pathways are classified as fragile", handing the reader one of our internal field names instead of the reason. The instruction now forbids repeating field names and asks for the explanation in the reader's terms. Three further runs, no leaked label, real counts in each. And I nearly credited that fix wrongly. The assertion checking it had failed, because it looked for the phrase in the *source file* where the string is wrapped across two lines -- so the improvement I saw in that run was model non-determinism, not my edit. The edit was fine; the check was wrong, and only running it three times distinguished the two. No per-pathway request parameter was added. US3's independent test implies one, the website has not asked for it, and every shown pathway already carries its own counts -- so the explanation is per-pathway without new contract surface. Co-Authored-By: Claude Opus 5 --- specs/011-summarise-analysis-results/tasks.md | 17 ++++- src/analysis/summarise.py | 36 +++++++++ src/api/analysis_summary.py | 2 + tests/analysis/test_summarise.py | 75 ++++++++++++++++++- 4 files changed, 125 insertions(+), 5 deletions(-) diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index 5502adc..b46a6ce 100644 --- a/specs/011-summarise-analysis-results/tasks.md +++ b/specs/011-summarise-analysis-results/tasks.md @@ -65,13 +65,24 @@ count is reported instead; the reader knows what they submitted. ## Phase 5: User Story 3 — What do these numbers mean? (P3) +**Fragility is computed in code, not asked for in the prompt.** A hit resting +on fewer than five matched entities is flagged per pathway, because a model +handed a tiny p-value and a tiny count describes the p-value. The threshold is +on the *count*, not the ratio: 2 of 3 looks perfect by ratio and is nearly +meaningless, 40 of 200 looks poor and is real evidence. + +**No per-pathway request parameter was added.** US3's independent test implies +one, but the website has not asked for it and every shown pathway already +carries its own counts, so the explanation is per-pathway without new contract +surface. + **Goal**: The statistics are explained using the reader's own numbers. **Independent test**: Ask about one pathway in a result; the explanation uses that pathway's counts, not generic definitions. -- [ ] T021 [US3] Carry per-pathway counts into the prompt input for a named pathway in `src/analysis/summarise.py` -- [ ] T022 [P] [US3] Test that a pathway significant by p-value but not by FDR is described as distinguishing the two, in `tests/analysis/test_summarise.py` (spec US3 scenario 2) -- [ ] T023 [P] [US3] Test that a pathway with very few found entities is described as fragile, using its actual counts, in `tests/analysis/test_summarise.py` (spec US3 scenario 1) +- [x] T021 [US3] Carry per-pathway counts into the prompt input for a named pathway in `src/analysis/summarise.py` +- [x] T022 [P] [US3] Test that a pathway significant by p-value but not by FDR is described as distinguishing the two, in `tests/analysis/test_summarise.py` (spec US3 scenario 2) +- [x] T023 [P] [US3] Test that a pathway with very few found entities is described as fragile, using its actual counts, in `tests/analysis/test_summarise.py` (spec US3 scenario 1) ## Phase 6: User Story 4 — Readings specific to the analysis type (P4) diff --git a/src/analysis/summarise.py b/src/analysis/summarise.py index 83e5c5c..a8ecd49 100644 --- a/src/analysis/summarise.py +++ b/src/analysis/summarise.py @@ -15,11 +15,27 @@ FDR_THRESHOLD = 0.05 +#: Below this many matched entities, a pathway's p-value rests on so little +#: that it should not be read as evidence however small it is. Reactome's own +#: user guide makes this point and it is the single thing a reader most often +#: gets wrong -- a pathway with 2 of 3 entities found looks like a perfect hit +#: and is nearly meaningless. +#: +#: Computed here rather than left to the model. Handed a small p-value and a +#: small count and asked to be careful, a model describes the p-value. +FRAGILE_BELOW_FOUND = 5 + + def _significant(pathway: dict[str, Any]) -> bool: fdr = pathway.get("entities", {}).get("fdr") return isinstance(fdr, int | float) and fdr <= FDR_THRESHOLD +def _fragile(pathway: dict[str, Any]) -> bool: + found = pathway.get("entities", {}).get("found") + return isinstance(found, int) and found < FRAGILE_BELOW_FOUND + + def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: """What to tell the model, from the aggregate payload. @@ -77,6 +93,9 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: "p_value": p.get("entities", {}).get("pValue"), "fdr": p.get("entities", {}).get("fdr"), "significant": _significant(p), + # True when the hit rests on too few entities to be evidence, + # whatever the p-value says. + "fragile": _fragile(p), } for p in pathways ], @@ -119,6 +138,23 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: "suggests the wrong species was analysed." ) +#: Always appended. Both halves are things a model will otherwise get wrong +#: in the same direction -- towards overstating a finding. +STATISTICS_INSTRUCTION = ( + "When you call a pathway significant, say whether that is before or after " + "multiple-testing correction; `significant` in the data is after. A " + "pathway with a small p-value that does not pass correction is not a " + "finding, and saying so is more useful than omitting it. " + "Where `fragile` is true the hit rests on fewer than " + f"{FRAGILE_BELOW_FOUND} matched entities. Explain that in the reader's " + "terms using that pathway's own found and total counts -- a small p-value " + "on three entities is not evidence however small it is. **Never use the " + "word 'fragile' or any other field name from the data**: these are " + "internal labels, and a reader should be told what the counts mean, not " + "what we called it. Explain what the numbers mean for this result, never " + "in general." +) + #: Appended only when the reader chose the disclosing tier and unmatched #: identifiers were actually retrieved. #: diff --git a/src/api/analysis_summary.py b/src/api/analysis_summary.py index 1e17576..d7689ae 100644 --- a/src/api/analysis_summary.py +++ b/src/api/analysis_summary.py @@ -31,6 +31,7 @@ from analysis.summarise import ( INEXACT_COUNT_INSTRUCTION, NAMED_UNMATCHED_INSTRUCTION, + STATISTICS_INSTRUCTION, UNMATCHED_INSTRUCTION, VERDICT_INSTRUCTION, prompt_input, @@ -201,6 +202,7 @@ async def stream() -> AsyncIterator[str]: instruction = VERDICT_INSTRUCTION[model_input["verdict"]] if not model_input["significant_count_is_exact"]: instruction = f"{instruction} {INEXACT_COUNT_INSTRUCTION}" + instruction = f"{instruction} {STATISTICS_INSTRUCTION}" instruction = f"{instruction} {UNMATCHED_INSTRUCTION}" if model_input.get("identifiers_not_found_names"): instruction = f"{instruction} {NAMED_UNMATCHED_INSTRUCTION}" diff --git a/tests/analysis/test_summarise.py b/tests/analysis/test_summarise.py index e39d2f5..aad6a35 100644 --- a/tests/analysis/test_summarise.py +++ b/tests/analysis/test_summarise.py @@ -1,9 +1,11 @@ """What the model is told about a result, before any model is involved.""" +from typing import Any + from analysis.summarise import VERDICT_INSTRUCTION, prompt_input -def _payload(*fdrs: float) -> dict[str, object]: +def _payload(*fdrs: float) -> dict[str, Any]: return { "summary": {"type": "OVERREPRESENTATION"}, "pathways_total": len(fdrs), @@ -59,7 +61,7 @@ def test_a_pathway_exactly_on_the_threshold_counts_as_significant() -> None: def test_a_missing_fdr_is_not_significant() -> None: # Absence is normal in this API; it must never read as passing. payload = _payload(0.01) - del payload["pathways"][0]["entities"]["fdr"] # type: ignore[index] + del payload["pathways"][0]["entities"]["fdr"] assert prompt_input(payload)["significant_among_shown"] == 0 @@ -121,3 +123,72 @@ def test_unmatched_identifiers_are_reported_as_a_count_not_a_proportion() -> Non assert "Never state a proportion" in UNMATCHED_INSTRUCTION # Nothing in the payload lets a proportion be computed. assert not any("submitted" in k or "total_identifiers" in k for k in out) + + +def test_a_hit_resting_on_few_entities_is_flagged_fragile() -> None: + # US3 scenario 1, and the thing the spec says readers most often get + # wrong: "a pathway with 2 of 3 entities found is not strong evidence". + # Computed here because a model handed a tiny p-value and a tiny count + # describes the p-value. + payload = _payload(1e-9) + payload["pathways"][0]["entities"].update({"found": 2, "total": 3}) + out = prompt_input(payload) + assert out["pathways"][0]["fragile"] is True + assert out["pathways"][0]["significant"] is True + + +def test_a_well_supported_hit_is_not_flagged_fragile() -> None: + # A flag that is always true would pass the test above and say nothing. + payload = _payload(1e-9) + payload["pathways"][0]["entities"].update({"found": 40, "total": 120}) + pathways: list[dict[str, Any]] = prompt_input(payload)["pathways"] + assert pathways[0]["fragile"] is False + + +def test_the_fragility_threshold_is_on_the_count_not_the_ratio() -> None: + # 2 of 3 looks like a perfect hit by ratio and is nearly meaningless; + # 40 of 200 looks poor by ratio and is real evidence. The ratio is the + # misleading number here, so the flag deliberately ignores it. + ratio_perfect = _payload(1e-9) + ratio_perfect["pathways"][0]["entities"].update({"found": 2, "total": 3}) + ratio_poor = _payload(1e-9) + ratio_poor["pathways"][0]["entities"].update({"found": 40, "total": 200}) + pathways: list[dict[str, Any]] = prompt_input(ratio_perfect)["pathways"] + assert pathways[0]["fragile"] is True + poor: list[dict[str, Any]] = prompt_input(ratio_poor)["pathways"] + assert poor[0]["fragile"] is False + + +def test_significant_before_correction_is_distinguishable_from_after() -> None: + # US3 scenario 2. Both numbers are present per pathway and `significant` + # is defined as after correction, so the two can be told apart rather + # than the model choosing which it means. + from analysis.summarise import STATISTICS_INSTRUCTION + + payload = _payload(0.2) + payload["pathways"][0]["entities"]["pValue"] = 0.001 + out = prompt_input(payload) + pathway = out["pathways"][0] + assert pathway["p_value"] == 0.001 + assert pathway["fdr"] == 0.2 + assert pathway["significant"] is False + assert "before or after" in STATISTICS_INSTRUCTION + assert "is after" in STATISTICS_INSTRUCTION + + +def test_a_pathway_with_no_found_count_is_not_called_fragile() -> None: + # Absence is normal in this API and must not read as a finding either way. + payload = _payload(1e-9) + del payload["pathways"][0]["entities"]["found"] + pathways: list[dict[str, Any]] = prompt_input(payload)["pathways"] + assert pathways[0]["fragile"] is False + + +def test_the_model_is_told_not_to_repeat_our_field_names() -> None: + # Measured against a real result: the summary said "these pathways are + # classified as fragile", handing the reader an internal label instead of + # the reason. The flag is ours; the explanation is theirs. + from analysis.summarise import STATISTICS_INSTRUCTION + + assert "Never use the word 'fragile'" in STATISTICS_INSTRUCTION + assert "internal labels" in STATISTICS_INSTRUCTION