diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index b46a6ce..52bc52e 100644 --- a/specs/011-summarise-analysis-results/tasks.md +++ b/specs/011-summarise-analysis-results/tasks.md @@ -86,13 +86,23 @@ surface. ## Phase 6: User Story 4 — Readings specific to the analysis type (P4) +**The type enum, read from the API on 2026-09-20 rather than guessed**: +`SPECIES_COMPARISON`, `OVERREPRESENTATION`, `EXPRESSION`, `GSA_REGULATION`, +`GSA_STATISTICS`, `GSVA`. Three are summarised; the three GSA ones are +declined. + +That reading also closed a gap in T009: `is_gsa` recognised ReactomeGSA by +its `gsaMethod` field alone, so a result carrying one of those types +*without* that field would have been summarised confidently — the outcome D8 +exists to prevent. Both signals are checked now. + **Goal**: An expression result and a species comparison each get the reading that fits them. **Independent test**: Submit one of each; neither summary describes the other's kind of result. -- [ ] T024 [US4] Branch the prompt input on `summary.type` in `src/analysis/summarise.py`, and for `EXPRESSION` carry `entities.exp[]` and the value range **without** `expression.columnNames`, which is user-supplied text -- [ ] T025 [US4] For `SPECIES_COMPARISON`, state in the prompt input that findings are inferred by orthology in `src/analysis/summarise.py`, so the summary cannot present them as observed -- [ ] T026 [P] [US4] Test that an expression result's summary refers to behaviour across columns and a species comparison's does not, and vice versa, in `tests/analysis/test_summarise.py` +- [x] T024 [US4] Branch the prompt input on `summary.type` in `src/analysis/summarise.py`, and for `EXPRESSION` carry `entities.exp[]` and the value range **without** `expression.columnNames`, which is user-supplied text +- [x] T025 [US4] For `SPECIES_COMPARISON`, state in the prompt input that findings are inferred by orthology in `src/analysis/summarise.py`, so the summary cannot present them as observed +- [x] T026 [P] [US4] Test that an expression result's summary refers to behaviour across columns and a species comparison's does not, and vice versa, in `tests/analysis/test_summarise.py` ## Phase 7: Stability and transparency (FR-014, FR-015) diff --git a/src/analysis/client.py b/src/analysis/client.py index 65f7fd9..74f2ae8 100644 --- a/src/analysis/client.py +++ b/src/analysis/client.py @@ -81,15 +81,32 @@ def _headers(accept: str = "application/json") -> dict[str, str]: VERSION_ACCEPT = "text/plain, */*" +#: The analysis types this service does not model. Read from the API's own +#: enum on 2026-09-20 -- `ExternalAnalysisSummary.type` is +#: SPECIES_COMPARISON, OVERREPRESENTATION, EXPRESSION, GSA_REGULATION, +#: GSA_STATISTICS, GSVA -- rather than guessed at, because the first version +#: recognised GSA by its `gsaMethod` field alone and a result carrying the +#: type without that field would have been summarised confidently. That is +#: precisely the outcome D8 exists to prevent. +GSA_TYPES = frozenset({"GSA_REGULATION", "GSA_STATISTICS", "GSVA"}) + + def is_gsa(result: dict[str, Any]) -> bool: """A ReactomeGSA result, which this does not model and will not summarise. GSA is a separate service on a different host with its own result shape. - Recognising it costs one field check and prevents the worst outcome: a - confident summary of something we do not actually understand. + Recognising it prevents the worst outcome: a confident summary of + something we do not actually understand. + + Two independent signals, because either alone has a gap. The fields catch + a result whose type is unset or new; the type catches one that carries no + `gsaMethod`. Neither is known to be sufficient on its own and there is no + cost to checking both. """ summary = result.get("summary") or {} - return bool(summary.get("gsaMethod") or summary.get("gsaToken")) + if summary.get("gsaMethod") or summary.get("gsaToken"): + return True + return str(summary.get("type") or "").upper() in GSA_TYPES async def fetch_result( diff --git a/src/analysis/disclosure.py b/src/analysis/disclosure.py index 468a434..1d0bd0c 100644 --- a/src/analysis/disclosure.py +++ b/src/analysis/disclosure.py @@ -44,6 +44,12 @@ "curatedFound", "interactorsFound", "resource", + # Per-pathway expression values, one per column. Aggregated across the + # pathway's entities rather than the reader's raw measurements, and the + # data model puts them in the aggregate tier -- but they are only + # interpretable *with* `expression.columnNames`, which is user-supplied + # text and never sent. Numbers without labels, which is the point. + "exp", ) PATHWAY_FIELDS: tuple[str, ...] = ("stId", "name", "species", "inDisease") diff --git a/src/analysis/summarise.py b/src/analysis/summarise.py index 248e5c8..3654459 100644 --- a/src/analysis/summarise.py +++ b/src/analysis/summarise.py @@ -150,6 +150,36 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: "suggests the wrong species was analysed." ) +#: What each analysis type supports saying, and what it does not. A summary +#: that ignores the type either says nothing useful or says something wrong, +#: and the wrong thing is the likelier: an expression result read as a plain +#: enrichment loses the whole point of it, and a species comparison read as +#: observation states as fact what was inferred. +TYPE_INSTRUCTION = { + "EXPRESSION": ( + "This is an expression analysis. Each pathway carries `exp`, its " + "values across the submitted columns in order. Describe how the " + "highlighted pathways behave *across* those columns -- rising, " + "falling, mixed -- rather than treating the result as a single " + "enrichment. **The columns are unlabelled here and you must not " + "guess what they are**: say 'the first column' and so on, never a " + "condition, timepoint or sample name." + ), + "SPECIES_COMPARISON": ( + "This is a species comparison. The findings are **inferred by " + "orthology**, not observed in the compared species, and you must say " + "so. An inferred event means Reactome projected a human event onto " + "that species because the proteins correspond; it is not evidence " + "the event has been measured there." + ), + "OVERREPRESENTATION": ( + "This is an over-representation analysis: which pathways contain " + "more of the submitted identifiers than chance would give. It says " + "nothing about direction, magnitude or regulation, so do not " + "describe anything as up, down, increased or activated." + ), +} + #: Always appended. Both halves are things a model will otherwise get wrong #: in the same direction -- towards overstating a finding. STATISTICS_INSTRUCTION = ( diff --git a/src/api/analysis_summary.py b/src/api/analysis_summary.py index d7689ae..65dd884 100644 --- a/src/api/analysis_summary.py +++ b/src/api/analysis_summary.py @@ -32,6 +32,7 @@ INEXACT_COUNT_INSTRUCTION, NAMED_UNMATCHED_INSTRUCTION, STATISTICS_INSTRUCTION, + TYPE_INSTRUCTION, UNMATCHED_INSTRUCTION, VERDICT_INSTRUCTION, prompt_input, @@ -202,6 +203,11 @@ 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}" + by_type = TYPE_INSTRUCTION.get( + str(model_input.get("analysis_type") or "").upper() + ) + if by_type: + instruction = f"{instruction} {by_type}" instruction = f"{instruction} {STATISTICS_INSTRUCTION}" instruction = f"{instruction} {UNMATCHED_INSTRUCTION}" if model_input.get("identifiers_not_found_names"): diff --git a/tests/analysis/test_analysis_client.py b/tests/analysis/test_analysis_client.py index 75e1fec..3095b7b 100644 --- a/tests/analysis/test_analysis_client.py +++ b/tests/analysis/test_analysis_client.py @@ -196,3 +196,37 @@ async def go() -> object: return await analysis_client.fetch_result(SAMPLE_TOKEN, client=http) assert asyncio.run(go()).outcome == "failed" # type: ignore[attr-defined] + + +@pytest.mark.parametrize("gsa_type", ["GSA_REGULATION", "GSA_STATISTICS", "GSVA"]) +def test_a_gsa_type_is_declined_even_without_a_gsa_method(gsa_type: str) -> None: + # The first version recognised GSA by `gsaMethod` alone. A result + # carrying one of these types without that field would have been + # summarised confidently -- the exact outcome D8 exists to prevent. The + # type list is read from the API's own enum, not guessed. + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, json={"summary": {"type": gsa_type}, "pathways": []}) + + async def go() -> object: + async with _client(handler) as http: + return await analysis_client.fetch_result(SAMPLE_TOKEN, client=http) + + assert asyncio.run(go()).outcome == "unsupported" # type: ignore[attr-defined] + + +@pytest.mark.parametrize( + "supported", ["OVERREPRESENTATION", "EXPRESSION", "SPECIES_COMPARISON"] +) +def test_the_types_this_service_models_are_not_declined(supported: str) -> None: + # A check that declined everything would pass the test above and make the + # feature useless. + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response( + 200, json={"summary": {"type": supported}, "pathways": []} + ) + + async def go() -> object: + async with _client(handler) as http: + return await analysis_client.fetch_result(SAMPLE_TOKEN, client=http) + + assert asyncio.run(go()).outcome == "ok" # type: ignore[attr-defined] diff --git a/tests/analysis/test_disclosure.py b/tests/analysis/test_disclosure.py index b9b4215..8184ca0 100644 --- a/tests/analysis/test_disclosure.py +++ b/tests/analysis/test_disclosure.py @@ -137,3 +137,14 @@ def test_no_warnings_key_when_the_service_sent_none() -> None: result = json.loads(json.dumps(RESULT)) result["warnings"] = [] assert "warnings" not in aggregate(result) + + +def test_expression_values_survive_but_their_labels_do_not() -> None: + # The expression story needs the per-pathway values; it must never need + # the column names, which are user-supplied text like "Patient_001_tumour". + result = json.loads(json.dumps(RESULT)) + result["pathways"][0]["entities"]["exp"] = [1.2, -0.4, 0.9] + payload = aggregate(result) + assert payload["pathways"][0]["entities"]["exp"] == [1.2, -0.4, 0.9] + assert "columnNames" not in json.dumps(payload) + assert "Patient_001_tumour" not in json.dumps(payload) diff --git a/tests/analysis/test_summarise.py b/tests/analysis/test_summarise.py index d8c4ed2..9d63439 100644 --- a/tests/analysis/test_summarise.py +++ b/tests/analysis/test_summarise.py @@ -210,3 +210,32 @@ def test_the_fragility_threshold_discriminates_on_realistic_inputs() -> None: real_flags = [p["fragile"] for p in prompt_input(realistic)["pathways"]] assert all(tiny_flags), "a hit on two entities must be flagged" assert not any(real_flags), "ordinary hits must not all be flagged" + + +def test_each_analysis_type_is_told_what_it_may_and_may_not_say() -> None: + # US4. A summary that ignores the type either says nothing useful or + # says something wrong, and wrong is likelier: an expression result read + # as a plain enrichment loses the point, and a species comparison read as + # observation states as fact what was inferred. + from analysis.summarise import TYPE_INSTRUCTION + + assert "across" in TYPE_INSTRUCTION["EXPRESSION"] + assert "must not" in TYPE_INSTRUCTION["EXPRESSION"] + assert "orthology" in TYPE_INSTRUCTION["SPECIES_COMPARISON"] + assert "not evidence the event has been measured" in ( + TYPE_INSTRUCTION["SPECIES_COMPARISON"].replace(" ", " ") + ) + # The over-representation reading exists to forbid direction language, + # which is the thing that result cannot support at all. + assert "up, down" in TYPE_INSTRUCTION["OVERREPRESENTATION"] + + +def test_no_type_instruction_claims_another_types_reading() -> None: + # The cross-check US4 asks for: neither summary may describe the other's + # kind of result. Asserted on the instructions, since that is where the + # confusion would originate. + from analysis.summarise import TYPE_INSTRUCTION + + assert "orthology" not in TYPE_INSTRUCTION["EXPRESSION"] + assert "columns" not in TYPE_INSTRUCTION["SPECIES_COMPARISON"] + assert "columns" not in TYPE_INSTRUCTION["OVERREPRESENTATION"] diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py index 412e306..56043d2 100644 --- a/tests/api/test_analysis_summary.py +++ b/tests/api/test_analysis_summary.py @@ -431,3 +431,42 @@ async def _empty(_token: str, **_kwargs: Any) -> list[str]: _post(public, caller_token=_token(private), disclosure="identifiers").text )[0][1] assert start["disclosure"] == "identifiers" + + +@pytest.mark.parametrize( + ("analysis_type", "expected", "forbidden"), + [ + ("EXPRESSION", "across", "orthology"), + ("SPECIES_COMPARISON", "orthology", "columns"), + ("OVERREPRESENTATION", "up, down", "orthology"), + ], +) +def test_each_type_gets_its_own_reading_on_the_served_path( + keys: tuple[str, str], + monkeypatch: pytest.MonkeyPatch, + analysis_type: str, + expected: str, + forbidden: str, +) -> None: + # US4's independent test: submit each type and check neither summary is + # told the other's reading. Asserted on what reaches the model, over + # HTTP, because the branch is in the endpoint and not in `summarise`. + monkeypatch.setitem(RESULT["summary"], "type", analysis_type) + sent: list[Any] = [] + + class _Recording(_Counter): + async def astream(self, messages: Any) -> AsyncIterator[Any]: + sent.append(messages) + async for chunk in super().astream(messages): + yield chunk + + recording = _Recording() + private, public = keys + with pytest.MonkeyPatch.context() as patch: + patch.setattr("api.analysis_summary.get_llm", lambda *a, **k: recording) + response = _post(public, caller_token=_token(private)) + assert _events(response.text)[0][1]["analysis_type"] == analysis_type + assert sent, "the model was never called, so this proves nothing" + prompt = json.dumps(sent, default=str) + assert expected in prompt, f"{analysis_type} was not given its own reading" + assert forbidden not in prompt, f"{analysis_type} was given another's"