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