From 9cee2b6827e6e411148a38a22e138cce86a4b791 Mon Sep 17 00:00:00 2001 From: Florin Senoner <23100806+FlorinSenoner@users.noreply.github.com> Date: Sat, 1 Aug 2026 19:04:25 +0200 Subject: [PATCH 1/6] fix(annotations): preserve negative transmembrane predictions --- apps/protspace/docs/annotations.md | 2 +- .../retrievers/biocentral_retriever.py | 4 +- .../tests/test_biocentral_retriever.py | 2 +- docs/guide/annotations.md | 4 +- docs/scripts/annotation-details.ts | 2 +- .../.openspec.yaml | 2 + .../README.md | 3 + .../design.md | 85 +++++++++++++++++++ .../proposal.md | 33 +++++++ .../specs/bundle-format-contract/spec.md | 21 +++++ .../tasks.md | 16 ++++ .../src/visualization/annotation-metadata.ts | 3 +- tests/contract/bundle.contract.test.ts | 7 ++ tests/contract/emit_bundles.py | 16 ++++ 14 files changed, 192 insertions(+), 8 deletions(-) create mode 100644 openspec/changes/fix-biocentral-transmembrane-sentinel/.openspec.yaml create mode 100644 openspec/changes/fix-biocentral-transmembrane-sentinel/README.md create mode 100644 openspec/changes/fix-biocentral-transmembrane-sentinel/design.md create mode 100644 openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md create mode 100644 openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md create mode 100644 openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md diff --git a/apps/protspace/docs/annotations.md b/apps/protspace/docs/annotations.md index 2bf6e60f5..83fa298d2 100644 --- a/apps/protspace/docs/annotations.md +++ b/apps/protspace/docs/annotations.md @@ -197,7 +197,7 @@ Per-protein predictions from the [Biocentral API](https://biocentral.rostlab.org | `predicted_subcellular_location` | LightAttention | 10-class subcellular localization | | `predicted_membrane` | LightAttention | Membrane / Soluble | | `predicted_signal_peptide` | TMbed | True / False (derived from topology) | -| `predicted_transmembrane` | TMbed | none / alpha-helical / beta-barrel | +| `predicted_transmembrane` | TMbed | non-transmembrane / alpha-helical / beta-barrel | **Data source**: Batch predictions via Biocentral API (`api.predict()`). TMbed provides per-residue topology labels (`H`=TM helix, `B`=TM beta strand, `S`=signal peptide); signal peptide and transmembrane type are summarized from these labels. diff --git a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py index 0946eaa4f..a2e2ec625 100644 --- a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py +++ b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py @@ -212,7 +212,7 @@ def _extract_signal_peptide(predictions: list) -> str: def _extract_transmembrane(predictions: list) -> str: """Derive transmembrane type from TMbed per-residue output. - Returns: 'alpha-helical', 'beta-barrel', or 'none' + Returns: 'alpha-helical', 'beta-barrel', or 'non-transmembrane' """ for pred in predictions: if pred.model_name == "TMbed": @@ -225,5 +225,5 @@ def _extract_transmembrane(predictions: list) -> str: return "alpha-helical" elif has_beta: return "beta-barrel" - return "none" + return "non-transmembrane" return "" diff --git a/apps/protspace/tests/test_biocentral_retriever.py b/apps/protspace/tests/test_biocentral_retriever.py index 82d91754c..037c9ac6a 100644 --- a/apps/protspace/tests/test_biocentral_retriever.py +++ b/apps/protspace/tests/test_biocentral_retriever.py @@ -67,7 +67,7 @@ def test_both_types(self): def test_no_transmembrane(self): preds = [_make_prediction("TMbed", "oooooooooiiiiiiiiiii")] result = BiocentralPredictionRetriever._extract_transmembrane(preds) - assert result == "none" + assert result == "non-transmembrane" def test_lowercase_labels(self): """TMbed uses lowercase h/b for non-TM side of helix/strand.""" diff --git a/docs/guide/annotations.md b/docs/guide/annotations.md index c32ce7097..beed2317d 100644 --- a/docs/guide/annotations.md +++ b/docs/guide/annotations.md @@ -58,9 +58,9 @@ LightAttention is a lightweight neural network that uses softmax-weighted aggreg **Transmembrane** · ⚡ Predicted -Transmembrane type (none / alpha-helical / beta-barrel) predicted by TMbed. +Transmembrane type (non-transmembrane / alpha-helical / beta-barrel) predicted by TMbed. -From the same TMbed per-residue topology (H = transmembrane helix, B = transmembrane beta strand, S = signal peptide), ProtSpace summarizes the membrane-spanning segments into a single protein-level category. Values are `alpha-helical` when transmembrane helices (H) are predicted, `beta-barrel` when transmembrane beta strands (B) are predicted, and `none` when neither is present. See [Bernhofer & Rost, BMC Bioinformatics 2022](https://doi.org/10.1186/s12859-022-04873-x). +From the same TMbed per-residue topology (H = transmembrane helix, B = transmembrane beta strand, S = signal peptide), ProtSpace summarizes the membrane-spanning segments into a single protein-level category. Values are `alpha-helical` when transmembrane helices (H) are predicted, `beta-barrel` when transmembrane beta strands (B) are predicted, and `non-transmembrane` when neither is present. See [Bernhofer & Rost, BMC Bioinformatics 2022](https://doi.org/10.1186/s12859-022-04873-x). ## UniProt diff --git a/docs/scripts/annotation-details.ts b/docs/scripts/annotation-details.ts index dc2aff2b4..73018f976 100644 --- a/docs/scripts/annotation-details.ts +++ b/docs/scripts/annotation-details.ts @@ -57,7 +57,7 @@ export const ANNOTATION_DETAILS: Record = { }, predicted_transmembrane: { detailsMarkdown: - 'From the same TMbed per-residue topology (H = transmembrane helix, B = transmembrane beta strand, S = signal peptide), ProtSpace summarizes the membrane-spanning segments into a single protein-level category. Values are `alpha-helical` when transmembrane helices (H) are predicted, `beta-barrel` when transmembrane beta strands (B) are predicted, and `none` when neither is present. See [Bernhofer & Rost, BMC Bioinformatics 2022](https://doi.org/10.1186/s12859-022-04873-x).', + 'From the same TMbed per-residue topology (H = transmembrane helix, B = transmembrane beta strand, S = signal peptide), ProtSpace summarizes the membrane-spanning segments into a single protein-level category. Values are `alpha-helical` when transmembrane helices (H) are predicted, `beta-barrel` when transmembrane beta strands (B) are predicted, and `non-transmembrane` when neither is present. See [Bernhofer & Rost, BMC Bioinformatics 2022](https://doi.org/10.1186/s12859-022-04873-x).', sourceUrl: 'https://doi.org/10.1186/s12859-022-04873-x', }, diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/.openspec.yaml b/openspec/changes/fix-biocentral-transmembrane-sentinel/.openspec.yaml new file mode 100644 index 000000000..5849c2dbf --- /dev/null +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-01 diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/README.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/README.md new file mode 100644 index 000000000..5f60931b0 --- /dev/null +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/README.md @@ -0,0 +1,3 @@ +# fix-biocentral-transmembrane-sentinel + +Prevent valid no-transmembrane predictions from being rendered as missing. diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md new file mode 100644 index 000000000..39b913379 --- /dev/null +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md @@ -0,0 +1,85 @@ +## Context + +The Biocentral TMbed adapter currently serializes a valid negative prediction as the +categorical string `none`. The web bundle reader applies a shared missing-value policy +to all categorical annotations, and that policy intentionally treats `none` +case-insensitively as absent data. Consequently, a successfully computed negative +TMbed prediction is displayed as `N/A`, making it indistinguishable from proteins for +which no prediction was produced. + +This behavior crosses the Python annotation producer, the parquet bundle contract, and +the TypeScript visualization consumer. The fix must preserve the generic importer's +established missing-value semantics while making newly produced TMbed data unambiguous. + +## Goals / Non-Goals + +**Goals:** + +- Preserve a successful TMbed negative prediction as a visible categorical value in + the web application. +- Keep genuinely absent Biocentral predictions represented as missing values. +- Exercise the producer value through the generated Python-to-TypeScript bundle + contract so future sentinel collisions fail in CI. +- Keep user-facing annotation documentation aligned with the emitted vocabulary. + +**Non-Goals:** + +- Changing the generic web missing-value token set. +- Rewriting or migrating already generated `.parquetbundle` files. +- Changing TMbed topology interpretation for alpha helices, beta barrels, or mixed + predictions. +- Adding contextual bundle-reader rules for one annotation name. + +## Decisions + +### Emit `non-transmembrane` for a successful negative TMbed prediction + +The Biocentral adapter will return `non-transmembrane` when a TMbed result contains +neither an alpha-helical nor a beta-barrel segment. The phrase is explicit, is not in +the consumer's missing-value token set, and does not imply that the protein is soluble +or intracellular. + +Alternatives considered: + +- Keep `none` and remove it from the generic missing-value set. This would change the + interpretation of arbitrary imported datasets and turn commonly used missing + sentinels into visible categories. +- Translate `none` only when reading `predicted_transmembrane`. This would add + domain-specific repair logic to the generic bundle consumer and would still leave + the on-disk producer contract ambiguous. +- Use `soluble`. This is biologically narrower than "no predicted transmembrane + segment" and can mislabel secreted or otherwise non-membrane proteins. + +### Preserve empty-string output for an absent TMbed result + +The adapter will continue returning an empty string when Biocentral provides no TMbed +prediction. This retains the current distinction at the producer: a completed negative +prediction is `non-transmembrane`, while an unavailable prediction remains missing. + +### Cover both the adapter and the cross-language seam + +A focused Python regression will assert the adapter vocabulary. The generated bundle +contract will derive its transmembrane fixture value from the real adapter and assert +that the TypeScript visualization data contains the category. This avoids a duplicated +hand-written constant that could allow producer and contract fixtures to drift apart. + +## Risks / Trade-offs + +- [Existing bundles still contain ambiguous `none` values] → Document that the fix + applies to newly generated annotations; users must regenerate an affected bundle to + recover the distinction. +- [Downstream consumers may expect the old literal] → Treat the value as a categorical + contract correction and document the new vocabulary in both CLI and web metadata. +- [Contract fixture imports more producer code] → Use a minimal synthetic TMbed + prediction and retain the real bundle CLI as the serialization boundary. + +## Migration Plan + +Deploy the producer vocabulary and web documentation together. No storage migration is +performed. Newly annotated bundles will carry `non-transmembrane`; existing bundles can +be regenerated with the updated CLI. Rollback consists of reverting the producer value +and its associated tests/documentation. + +## Open Questions + +None. diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md new file mode 100644 index 000000000..3b57fee0b --- /dev/null +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md @@ -0,0 +1,33 @@ +## Why + +Biocentral emits a valid protein-level `predicted_transmembrane` category named +`none`, but the web reader reserves `none` as a missing-value token. As a result, +proteins with a successful “no transmembrane segment” prediction are displayed as +N/A and cannot be distinguished from proteins whose prediction is actually absent. + +## What Changes + +- Encode successful TMbed predictions with no membrane-spanning segment using a + non-reserved categorical label. +- Preserve genuinely absent Biocentral predictions as missing values. +- Add regression coverage across the Python annotation producer and TypeScript + bundle consumer boundary. +- Update generated annotation documentation to describe the corrected category. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `bundle-format-contract`: Require producer-emitted categorical values to remain + distinguishable from the consumer's missing-value sentinel set. + +## Impact + +- Python TMbed annotation extraction in `apps/protspace`. +- The generated cross-language bundle fixture and contract assertions. +- Annotation metadata/documentation describing `predicted_transmembrane` values. +- No dependency, API, or bundle-layout changes. diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md new file mode 100644 index 000000000..891825a2d --- /dev/null +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md @@ -0,0 +1,21 @@ +## ADDED Requirements + +### Requirement: Produced annotation categories do not collide with missing-value sentinels + +The Python annotation producer SHALL serialize a completed categorical prediction using +a value that the TypeScript bundle consumer preserves as a category rather than +normalizing to missing data. + +#### Scenario: Negative TMbed prediction crosses the bundle boundary + +- **WHEN** Biocentral returns a completed TMbed prediction containing neither an + alpha-helical nor a beta-barrel transmembrane segment +- **THEN** the producer emits `non-transmembrane` +- **AND** the TypeScript visualization data exposes `non-transmembrane` as a categorical + value rather than `N/A` + +#### Scenario: Missing TMbed prediction remains missing + +- **WHEN** Biocentral returns no TMbed prediction for a protein +- **THEN** the producer emits the established missing representation +- **AND** the TypeScript visualization data does not invent a transmembrane category diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md new file mode 100644 index 000000000..0f6815947 --- /dev/null +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md @@ -0,0 +1,16 @@ +## 1. Regression Coverage + +- [x] 1.1 Update the focused Biocentral retriever test to require the unambiguous negative TMbed category and observe it fail. +- [x] 1.2 Extend the generated Python-to-TypeScript bundle contract with a negative TMbed prediction and observe the consumer assertion fail. + +## 2. Producer and Documentation + +- [x] 2.1 Change the Biocentral TMbed adapter to emit `non-transmembrane` for a completed negative prediction. +- [x] 2.2 Update CLI and web annotation descriptions to document the corrected categorical vocabulary. + +## 3. Verification + +- [x] 3.1 Run the focused Python retriever and cross-language contract tests and confirm they pass. +- [x] 3.2 Regenerate annotation documentation and verify generated files are current. +- [x] 3.3 Run the affected Python lint/test gates and the repository-wide `pnpm precommit` gate. +- [x] 3.4 Verify a newly produced bundle displays the negative TMbed category separately from missing values. diff --git a/packages/utils/src/visualization/annotation-metadata.ts b/packages/utils/src/visualization/annotation-metadata.ts index eb8ad9d20..fc506d9ce 100644 --- a/packages/utils/src/visualization/annotation-metadata.ts +++ b/packages/utils/src/visualization/annotation-metadata.ts @@ -354,7 +354,8 @@ export const ANNOTATION_METADATA: Record = { label: 'Transmembrane', source: 'Biocentral', isPredicted: true, - description: 'Transmembrane type (none / alpha-helical / beta-barrel) predicted by TMbed.', + description: + 'Transmembrane type (non-transmembrane / alpha-helical / beta-barrel) predicted by TMbed.', docsUrl: docs('predicted_transmembrane'), }, }; diff --git a/tests/contract/bundle.contract.test.ts b/tests/contract/bundle.contract.test.ts index 4df8038b0..1fe05274c 100644 --- a/tests/contract/bundle.contract.test.ts +++ b/tests/contract/bundle.contract.test.ts @@ -40,6 +40,7 @@ interface Manifest { largeProteinCount: number; projectionCount: number; labelWithReservedChar: string; + negativeTransmembraneCategory: string; nullLengthIndex: number; } @@ -208,6 +209,12 @@ describe('annotation encoding across the language boundary', () => { expect(data.annotations.domains.values).toContain('DomB'); }); + it('preserves a negative TMbed prediction as a category', () => { + expect(data.annotations.predicted_transmembrane.values).toContain( + manifest.negativeTransmembraneCategory, + ); + }); + it('reports a missing numeric value as missing rather than zero', () => { const lengths = data.numeric_annotation_data?.length; // Assert the length first: an out-of-range index yields `undefined`, which diff --git a/tests/contract/emit_bundles.py b/tests/contract/emit_bundles.py index 1dda01e7c..535ba950a 100644 --- a/tests/contract/emit_bundles.py +++ b/tests/contract/emit_bundles.py @@ -37,11 +37,15 @@ import sys from concurrent.futures import ThreadPoolExecutor from pathlib import Path +from types import SimpleNamespace import pyarrow as pa import pyarrow.parquet as pq from protspace.data.annotations.encoding import encode_field +from protspace.data.annotations.retrievers.biocentral_retriever import ( + BiocentralPredictionRetriever, +) # Small enough to eyeball a failure, big enough for a category to have members. PROTEIN_COUNT = 10 @@ -72,6 +76,13 @@ def protein_ids(count: int) -> list[str]: # swallows the second hit, which is exactly the bug the grammar exists to avoid. MULTI_HIT_CELL = f"{encode_field('DomA')}|0.91;{encode_field('DomB')}|0.82" +# Derive this fixture value from the real annotation adapter rather than copying +# its vocabulary into the contract test. A topology with only inside/outside +# labels is a completed TMbed prediction with no membrane-spanning segment. +NEGATIVE_TMBED_CATEGORY = BiocentralPredictionRetriever._extract_transmembrane( + [SimpleNamespace(model_name="TMbed", value="oooooiiiii")] +) + def build_annotations_table(ids: list[str]) -> pa.Table: """Mimic ``protspace annotate`` output: an ``identifier`` column plus annotations. @@ -89,6 +100,7 @@ def build_annotations_table(ids: list[str]) -> pa.Table: encode_field("Hydrolase") ] * rest domains = [MULTI_HIT_CELL] + [f"{encode_field('DomB')}|0.75"] * rest + predicted_transmembrane = [NEGATIVE_TMBED_CATEGORY] + [""] * rest # A genuine double column with a null -- distinguishes "missing" from 0 and # from NaN across the language boundary. Real bundles carry both string-typed @@ -101,6 +113,9 @@ def build_annotations_table(ids: list[str]) -> pa.Table: "identifier": pa.array(ids, pa.string()), "family": pa.array(family, pa.string()), "domains": pa.array(domains, pa.string()), + "predicted_transmembrane": pa.array( + predicted_transmembrane, pa.string() + ), "length": pa.array(length, pa.float64()), } ) @@ -308,6 +323,7 @@ def emit(item: tuple[str, tuple[int, list[str]]]) -> None: "largeProteinCount": LARGE_PROTEIN_COUNT, "projectionCount": len(PROJECTIONS), "labelWithReservedChar": LABEL_WITH_RESERVED_CHAR, + "negativeTransmembraneCategory": NEGATIVE_TMBED_CATEGORY, "nullLengthIndex": NULL_LENGTH_INDEX, } ), From 5ec9fe2da94e8626583670cc8f16cbd37680a029 Mon Sep 17 00:00:00 2001 From: Florin Senoner <23100806+FlorinSenoner@users.noreply.github.com> Date: Sat, 1 Aug 2026 23:29:57 +0200 Subject: [PATCH 2/6] fix(annotations): preserve missing tmbed payloads --- .../retrievers/biocentral_retriever.py | 4 ++- .../tests/test_biocentral_retriever.py | 22 ++++++++++++ .../design.md | 18 ++++++---- .../proposal.md | 3 +- .../specs/bundle-format-contract/spec.md | 9 +++++ .../tasks.md | 15 ++++++++ tests/contract/bundle.contract.test.ts | 12 +++++++ tests/contract/emit_bundles.py | 35 +++++++++++++++---- 8 files changed, 103 insertions(+), 15 deletions(-) diff --git a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py index a2e2ec625..53bf830f1 100644 --- a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py +++ b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py @@ -216,7 +216,9 @@ def _extract_transmembrane(predictions: list) -> str: """ for pred in predictions: if pred.model_name == "TMbed": - topology = str(pred.value) if pred.value else "" + if pred.value is None or pred.value == "": + return "" + topology = str(pred.value) has_helix = bool(re.search(r"[Hh]", topology)) has_beta = bool(re.search(r"[Bb]", topology)) if has_helix and has_beta: diff --git a/apps/protspace/tests/test_biocentral_retriever.py b/apps/protspace/tests/test_biocentral_retriever.py index 037c9ac6a..76a9b0ab4 100644 --- a/apps/protspace/tests/test_biocentral_retriever.py +++ b/apps/protspace/tests/test_biocentral_retriever.py @@ -2,6 +2,8 @@ from unittest.mock import MagicMock, patch +from biocentral_api._generated import Prediction + from src.protspace.data.annotations.retrievers.biocentral_retriever import ( BIOCENTRAL_ANNOTATIONS, BiocentralPredictionRetriever, @@ -16,6 +18,16 @@ def _make_prediction(model_name, value): return pred +def _make_real_tmbed_prediction(value): + """Build the generated model returned by biocentral-api.""" + return Prediction( + model_name="TMbed", + prediction_name="topology", + protocol="per_residue", + value=value, + ) + + class TestBiocentralConstants: def test_biocentral_annotations(self): expected = [ @@ -69,6 +81,16 @@ def test_no_transmembrane(self): result = BiocentralPredictionRetriever._extract_transmembrane(preds) assert result == "non-transmembrane" + def test_none_payload_is_missing(self): + preds = [_make_real_tmbed_prediction(None)] + result = BiocentralPredictionRetriever._extract_transmembrane(preds) + assert result == "" + + def test_empty_payload_is_missing(self): + preds = [_make_real_tmbed_prediction("")] + result = BiocentralPredictionRetriever._extract_transmembrane(preds) + assert result == "" + def test_lowercase_labels(self): """TMbed uses lowercase h/b for non-TM side of helix/strand.""" preds = [_make_prediction("TMbed", "ooohhHHHHHhhoooo")] diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md index 39b913379..5e3bffa99 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md @@ -52,16 +52,20 @@ Alternatives considered: ### Preserve empty-string output for an absent TMbed result -The adapter will continue returning an empty string when Biocentral provides no TMbed -prediction. This retains the current distinction at the producer: a completed negative -prediction is `non-transmembrane`, while an unavailable prediction remains missing. +The adapter will return an empty string when Biocentral provides no TMbed prediction or +provides a TMbed prediction object whose optional `value` payload is `None` or empty. +The payload guard runs before topology labels are scanned. This retains the distinction +at the producer: a completed, non-empty topology with no membrane segment is +`non-transmembrane`, while an unavailable topology remains missing. ### Cover both the adapter and the cross-language seam -A focused Python regression will assert the adapter vocabulary. The generated bundle -contract will derive its transmembrane fixture value from the real adapter and assert -that the TypeScript visualization data contains the category. This avoids a duplicated -hand-written constant that could allow producer and contract fixtures to drift apart. +Focused Python regressions will construct the real Biocentral `Prediction` model and +assert the adapter vocabulary for completed and absent payloads. The generated bundle +contract will derive both its negative and missing transmembrane fixture values from the +real adapter and assert that TypeScript preserves the former as a category and the latter +as `N/A`. This avoids duplicated hand-written constants that could allow producer and +contract fixtures to drift apart. ## Risks / Trade-offs diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md index 3b57fee0b..50af4cd02 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md @@ -9,7 +9,8 @@ N/A and cannot be distinguished from proteins whose prediction is actually absen - Encode successful TMbed predictions with no membrane-spanning segment using a non-reserved categorical label. -- Preserve genuinely absent Biocentral predictions as missing values. +- Preserve genuinely absent Biocentral predictions, including TMbed prediction + objects whose optional payload is `None` or empty, as missing values. - Add regression coverage across the Python annotation producer and TypeScript bundle consumer boundary. - Update generated annotation documentation to describe the corrected category. diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md index 891825a2d..5dbb1ced1 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md @@ -19,3 +19,12 @@ normalizing to missing data. - **WHEN** Biocentral returns no TMbed prediction for a protein - **THEN** the producer emits the established missing representation - **AND** the TypeScript visualization data does not invent a transmembrane category + +#### Scenario: Empty TMbed payload remains missing + +- **WHEN** Biocentral returns a TMbed prediction object whose optional `value` payload + is `None` or an empty string +- **THEN** the producer emits the established missing representation before scanning + topology labels +- **AND** the TypeScript visualization data exposes the protein as `N/A` rather than + `non-transmembrane` diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md index 0f6815947..fab772c9e 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md @@ -14,3 +14,18 @@ - [x] 3.2 Regenerate annotation documentation and verify generated files are current. - [x] 3.3 Run the affected Python lint/test gates and the repository-wide `pnpm precommit` gate. - [x] 3.4 Verify a newly produced bundle displays the negative TMbed category separately from missing values. + +## 4. Review Regression Coverage + +- [x] 4.1 Add focused real-model regressions for `None` and empty TMbed payloads and observe the expected failures. +- [x] 4.2 Extend the generated bundle contract with an adapter-derived missing TMbed payload and observe the expected failure. + +## 5. Missing Payload Fix + +- [x] 5.1 Return the established missing representation before scanning absent TMbed topology payloads. + +## 6. Review Verification + +- [x] 6.1 Run the focused Biocentral and cross-language bundle contract tests and confirm they pass. +- [x] 6.2 Run Ruff, the complete non-slow ProtSpace Python suite, strict OpenSpec validation, and `pnpm precommit`. +- [x] 6.3 Prepare the verified change for the inline review follow-up and CI run. diff --git a/tests/contract/bundle.contract.test.ts b/tests/contract/bundle.contract.test.ts index 1fe05274c..45fb218cb 100644 --- a/tests/contract/bundle.contract.test.ts +++ b/tests/contract/bundle.contract.test.ts @@ -27,6 +27,7 @@ import { // working tree, not built package output. (The vitest config still aliases // `@protspace/utils` — packages/core's own sources import it that way.) import { BUNDLE_DELIMITER_BYTES } from '../../packages/utils/src/parquet/constants'; +import { getProteinAnnotationValues } from '../../packages/utils/src/visualization/plot-data-accessors'; const REPO_ROOT = resolve(__dirname, '../..'); @@ -41,6 +42,7 @@ interface Manifest { projectionCount: number; labelWithReservedChar: string; negativeTransmembraneCategory: string; + missingTransmembraneIndex: number; nullLengthIndex: number; } @@ -215,6 +217,16 @@ describe('annotation encoding across the language boundary', () => { ); }); + it('preserves a missing TMbed payload as N/A', () => { + expect( + getProteinAnnotationValues( + data, + manifest.missingTransmembraneIndex, + 'predicted_transmembrane', + ), + ).toEqual(['__NA__']); + }); + it('reports a missing numeric value as missing rather than zero', () => { const lengths = data.numeric_annotation_data?.length; // Assert the length first: an out-of-range index yields `undefined`, which diff --git a/tests/contract/emit_bundles.py b/tests/contract/emit_bundles.py index 535ba950a..3a8b7ae5b 100644 --- a/tests/contract/emit_bundles.py +++ b/tests/contract/emit_bundles.py @@ -37,10 +37,10 @@ import sys from concurrent.futures import ThreadPoolExecutor from pathlib import Path -from types import SimpleNamespace import pyarrow as pa import pyarrow.parquet as pq +from biocentral_api._generated import Prediction from protspace.data.annotations.encoding import encode_field from protspace.data.annotations.retrievers.biocentral_retriever import ( @@ -80,7 +80,29 @@ def protein_ids(count: int) -> list[str]: # its vocabulary into the contract test. A topology with only inside/outside # labels is a completed TMbed prediction with no membrane-spanning segment. NEGATIVE_TMBED_CATEGORY = BiocentralPredictionRetriever._extract_transmembrane( - [SimpleNamespace(model_name="TMbed", value="oooooiiiii")] + [ + Prediction( + model_name="TMbed", + prediction_name="topology", + protocol="per_residue", + value="oooooiiiii", + ) + ] +) + +# Keep one explicit missing-payload row separate from the fixture's other empty +# rows. This catches an adapter that invents a negative biological result before +# the bundle reader has a chance to normalize the missing representation. +MISSING_TMBED_INDEX = 1 +MISSING_TMBED_VALUE = BiocentralPredictionRetriever._extract_transmembrane( + [ + Prediction( + model_name="TMbed", + prediction_name="topology", + protocol="per_residue", + value=None, + ) + ] ) @@ -100,7 +122,9 @@ def build_annotations_table(ids: list[str]) -> pa.Table: encode_field("Hydrolase") ] * rest domains = [MULTI_HIT_CELL] + [f"{encode_field('DomB')}|0.75"] * rest - predicted_transmembrane = [NEGATIVE_TMBED_CATEGORY] + [""] * rest + predicted_transmembrane = [NEGATIVE_TMBED_CATEGORY, MISSING_TMBED_VALUE] + [""] * ( + rest - 1 + ) # A genuine double column with a null -- distinguishes "missing" from 0 and # from NaN across the language boundary. Real bundles carry both string-typed @@ -113,9 +137,7 @@ def build_annotations_table(ids: list[str]) -> pa.Table: "identifier": pa.array(ids, pa.string()), "family": pa.array(family, pa.string()), "domains": pa.array(domains, pa.string()), - "predicted_transmembrane": pa.array( - predicted_transmembrane, pa.string() - ), + "predicted_transmembrane": pa.array(predicted_transmembrane, pa.string()), "length": pa.array(length, pa.float64()), } ) @@ -324,6 +346,7 @@ def emit(item: tuple[str, tuple[int, list[str]]]) -> None: "projectionCount": len(PROJECTIONS), "labelWithReservedChar": LABEL_WITH_RESERVED_CHAR, "negativeTransmembraneCategory": NEGATIVE_TMBED_CATEGORY, + "missingTransmembraneIndex": MISSING_TMBED_INDEX, "nullLengthIndex": NULL_LENGTH_INDEX, } ), From b7f798b370efacf0ace0cd597c4351807a231505 Mon Sep 17 00:00:00 2001 From: Florin Senoner <23100806+FlorinSenoner@users.noreply.github.com> Date: Wed, 5 Aug 2026 14:03:52 +0200 Subject: [PATCH 3/6] fix(annotations): preserve fasta and tmbed missing data --- apps/prep/tests/test_pipeline.py | 2 +- apps/protspace/src/protspace/cli/annotate.py | 8 ++++ .../retrievers/biocentral_retriever.py | 41 +++++++++++-------- apps/protspace/tests/test_annotate_cli.py | 39 ++++++++++++++++++ .../tests/test_biocentral_retriever.py | 10 +++++ .../design.md | 27 ++++++++++-- .../proposal.md | 8 +++- .../specs/bundle-format-contract/spec.md | 15 +++++++ .../tasks.md | 22 ++++++++++ tests/contract/bundle.contract.test.ts | 10 +++++ tests/contract/emit_bundles.py | 14 +++++++ 11 files changed, 172 insertions(+), 24 deletions(-) create mode 100644 apps/protspace/tests/test_annotate_cli.py diff --git a/apps/prep/tests/test_pipeline.py b/apps/prep/tests/test_pipeline.py index 8b992e87b..896364d85 100644 --- a/apps/prep/tests/test_pipeline.py +++ b/apps/prep/tests/test_pipeline.py @@ -309,7 +309,7 @@ async def fake_create(*args, **kwargs): normalized = ctx.output_dir / "input.normalized.fasta" assert seen_inputs["embed"] == str(normalized) assert seen_inputs["annotate"] == str(normalized) - assert ">P12345" in normalized.read_text() + assert normalized.read_text() == ">P12345\nMAAAAAA\n" async def test_embed_failure_with_connection_refused_is_classified_as_biocentral_unavailable( diff --git a/apps/protspace/src/protspace/cli/annotate.py b/apps/protspace/src/protspace/cli/annotate.py index e2e99fecb..bb96d1e2f 100644 --- a/apps/protspace/src/protspace/cli/annotate.py +++ b/apps/protspace/src/protspace/cli/annotate.py @@ -61,10 +61,17 @@ def annotate( from protspace.data.loaders.h5 import EMBEDDING_EXTENSIONS # Extract identifiers from input + sequences = None if is_fasta_file(input): + from protspace.data.io.fasta import parse_fasta + from protspace.data.loaders.h5 import parse_identifier from protspace.data.loaders.query import extract_identifiers_from_fasta headers = extract_identifiers_from_fasta(input) + sequences = { + parse_identifier(header): sequence + for header, sequence in parse_fasta(input).items() + } elif input.suffix.lower() in EMBEDDING_EXTENSIONS: from protspace.data.loaders.h5 import _collect_datasets @@ -100,6 +107,7 @@ def annotate( headers=headers, annotations=annotations_list, output_path=None, + sequences=sequences, ).to_pd() if not scores: diff --git a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py index 53bf830f1..0cd4117d2 100644 --- a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py +++ b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py @@ -202,11 +202,10 @@ def _extract_signal_peptide(predictions: list) -> str: TMbed labels: S = signal peptide, H/h = TM helix, B/b = TM beta, i/o = non-TM """ - for pred in predictions: - if pred.model_name == "TMbed": - topology = str(pred.value) if pred.value else "" - return "True" if "S" in topology else "False" - return "" + topology = BiocentralPredictionRetriever._extract_tmbed_topology(predictions) + if topology is None: + return "" + return "True" if "S" in topology else "False" @staticmethod def _extract_transmembrane(predictions: list) -> str: @@ -214,18 +213,26 @@ def _extract_transmembrane(predictions: list) -> str: Returns: 'alpha-helical', 'beta-barrel', or 'non-transmembrane' """ + topology = BiocentralPredictionRetriever._extract_tmbed_topology(predictions) + if topology is None: + return "" + + has_helix = bool(re.search(r"[Hh]", topology)) + has_beta = bool(re.search(r"[Bb]", topology)) + if has_helix and has_beta: + return "alpha-helical;beta-barrel" + elif has_helix: + return "alpha-helical" + elif has_beta: + return "beta-barrel" + return "non-transmembrane" + + @staticmethod + def _extract_tmbed_topology(predictions: list) -> str | None: + """Return a non-empty TMbed topology, or ``None`` when unavailable.""" for pred in predictions: if pred.model_name == "TMbed": if pred.value is None or pred.value == "": - return "" - topology = str(pred.value) - has_helix = bool(re.search(r"[Hh]", topology)) - has_beta = bool(re.search(r"[Bb]", topology)) - if has_helix and has_beta: - return "alpha-helical;beta-barrel" - elif has_helix: - return "alpha-helical" - elif has_beta: - return "beta-barrel" - return "non-transmembrane" - return "" + return None + return str(pred.value) + return None diff --git a/apps/protspace/tests/test_annotate_cli.py b/apps/protspace/tests/test_annotate_cli.py new file mode 100644 index 000000000..39b25aad3 --- /dev/null +++ b/apps/protspace/tests/test_annotate_cli.py @@ -0,0 +1,39 @@ +"""Tests for the standalone ``protspace annotate`` command.""" + +import pandas as pd +from typer.testing import CliRunner + +from protspace.cli.app import app + + +def test_fasta_sequences_are_passed_to_annotation_manager(tmp_path, monkeypatch): + """FASTA-only proteins must reach sequence-backed annotation sources.""" + import protspace.data.annotations.manager as manager_module + + fasta = tmp_path / "input.fasta" + fasta.write_text( + ">sp|P12345|KNOWN Known protein\nMKV\n>custom-protein description\nAAAG\n" + ) + captured: dict[str, object] = {} + + class FakeManager: + def __init__(self, *args, **kwargs): + captured.update(kwargs) + + def to_pd(self): + return pd.DataFrame({"identifier": captured["headers"]}) + + monkeypatch.setattr(manager_module, "ProteinAnnotationManager", FakeManager) + + output = tmp_path / "annotations.parquet" + result = CliRunner().invoke( + app, + ["annotate", "-i", str(fasta), "-o", str(output)], + ) + + assert result.exit_code == 0, result.output + assert captured["headers"] == ["P12345", "custom-protein"] + assert captured["sequences"] == { + "P12345": "MKV", + "custom-protein": "AAAG", + } diff --git a/apps/protspace/tests/test_biocentral_retriever.py b/apps/protspace/tests/test_biocentral_retriever.py index 76a9b0ab4..a69183862 100644 --- a/apps/protspace/tests/test_biocentral_retriever.py +++ b/apps/protspace/tests/test_biocentral_retriever.py @@ -57,6 +57,16 @@ def test_no_tmbed_prediction(self): result = BiocentralPredictionRetriever._extract_signal_peptide(preds) assert result == "" + def test_none_payload_is_missing(self): + preds = [_make_real_tmbed_prediction(None)] + result = BiocentralPredictionRetriever._extract_signal_peptide(preds) + assert result == "" + + def test_empty_payload_is_missing(self): + preds = [_make_real_tmbed_prediction("")] + result = BiocentralPredictionRetriever._extract_signal_peptide(preds) + assert result == "" + class TestTransmembraneExtraction: """Test TMbed → transmembrane type derivation.""" diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md index 5e3bffa99..34b7fa490 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md @@ -18,6 +18,8 @@ established missing-value semantics while making newly produced TMbed data unamb - Preserve a successful TMbed negative prediction as a visible categorical value in the web application. - Keep genuinely absent Biocentral predictions represented as missing values. +- Keep FASTA-provided sequences available to sequence-backed annotation sources even + when UniProt cannot resolve their identifiers. - Exercise the producer value through the generated Python-to-TypeScript bundle contract so future sentinel collisions fail in CI. - Keep user-facing annotation documentation aligned with the emitted vocabulary. @@ -58,14 +60,31 @@ The payload guard runs before topology labels are scanned. This retains the dist at the producer: a completed, non-empty topology with no membrane segment is `non-transmembrane`, while an unavailable topology remains missing. +Both `predicted_signal_peptide` and `predicted_transmembrane` derive from that same +optional topology payload. A shared extractor therefore owns the missing-payload check +before either annotation interprets the topology, preventing one annotation from +inventing a completed negative result when the other reports missing data. + +### Pass FASTA sequences through the standalone annotation command + +The standalone `annotate` command will parse the FASTA into a canonical identifier to +sequence map and pass it to `ProteinAnnotationManager`. The manager already gives local +sequences priority and uses UniProt sequences only as fallback, so this closes the input +handoff without changing annotation-source behavior. HDF5 inputs continue to omit a +local sequence map and retain their existing UniProt-fallback behavior. + +The hosted prep service already normalizes FASTA headers and supplies that normalized +file to `protspace annotate`; regression coverage pins that normalization preserves the +sequence as well as the canonical identifier. + ### Cover both the adapter and the cross-language seam Focused Python regressions will construct the real Biocentral `Prediction` model and assert the adapter vocabulary for completed and absent payloads. The generated bundle -contract will derive both its negative and missing transmembrane fixture values from the -real adapter and assert that TypeScript preserves the former as a category and the latter -as `N/A`. This avoids duplicated hand-written constants that could allow producer and -contract fixtures to drift apart. +contract will derive its negative transmembrane and missing TMbed-derived fixture values +from the real adapter and assert that TypeScript preserves the former as a category and +the latter as `N/A`. This avoids duplicated hand-written constants that could allow +producer and contract fixtures to drift apart. ## Risks / Trade-offs diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md index 50af4cd02..082a76323 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md @@ -10,7 +10,10 @@ N/A and cannot be distinguished from proteins whose prediction is actually absen - Encode successful TMbed predictions with no membrane-spanning segment using a non-reserved categorical label. - Preserve genuinely absent Biocentral predictions, including TMbed prediction - objects whose optional payload is `None` or empty, as missing values. + objects whose optional payload is `None` or empty, as missing values for both + derived TMbed annotations. +- Preserve FASTA sequences through the standalone `annotate` command so + sequence-backed predictions do not depend on UniProt resolving the identifier. - Add regression coverage across the Python annotation producer and TypeScript bundle consumer boundary. - Update generated annotation documentation to describe the corrected category. @@ -28,7 +31,8 @@ None. ## Impact -- Python TMbed annotation extraction in `apps/protspace`. +- Python FASTA annotation input and TMbed extraction in `apps/protspace`. +- The hosted prep pipeline's normalized-FASTA handoff to `protspace annotate`. - The generated cross-language bundle fixture and contract assertions. - Annotation metadata/documentation describing `predicted_transmembrane` values. - No dependency, API, or bundle-layout changes. diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md index 5dbb1ced1..ecb412199 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md @@ -28,3 +28,18 @@ normalizing to missing data. topology labels - **AND** the TypeScript visualization data exposes the protein as `N/A` rather than `non-transmembrane` +- **AND** the derived signal-peptide annotation is also exposed as `N/A` rather than + `False` + +### Requirement: FASTA annotation inputs retain their sequences + +The standalone annotation producer SHALL pass sequences supplied in a FASTA input to +sequence-backed annotation sources using the same canonical identifiers as its output. + +#### Scenario: A FASTA identifier is not resolved by UniProt + +- **WHEN** `protspace annotate` receives a FASTA entry whose identifier UniProt cannot + resolve +- **THEN** the entry's FASTA sequence remains available to Biocentral and InterPro +- **AND** the hosted prep pipeline preserves that sequence when it passes its normalized + FASTA to `protspace annotate` diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md index fab772c9e..a024aaadb 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md @@ -29,3 +29,25 @@ - [x] 6.1 Run the focused Biocentral and cross-language bundle contract tests and confirm they pass. - [x] 6.2 Run Ruff, the complete non-slow ProtSpace Python suite, strict OpenSpec validation, and `pnpm precommit`. - [x] 6.3 Prepare the verified change for the inline review follow-up and CI run. + +## 7. Follow-up Review Regression Coverage + +- [x] 7.1 Add focused real-model regressions proving missing TMbed payloads remain + missing for signal peptide and observe the expected failures. +- [x] 7.2 Extend the generated bundle contract to expose missing signal peptide as + `N/A` and observe the expected failure. +- [x] 7.3 Add CLI regression coverage proving FASTA sequences reach the annotation + manager and strengthen the hosted normalized-FASTA handoff assertion. + +## 8. Follow-up Review Fixes + +- [x] 8.1 Parse and pass FASTA sequences through `protspace annotate` while preserving + HDF5 behavior. +- [x] 8.2 Centralize optional TMbed topology extraction for both derived annotations. + +## 9. Follow-up Review Verification + +- [x] 9.1 Run focused ProtSpace, hosted-prep, and cross-language contract tests. +- [x] 9.2 Run Ruff, the complete relevant Python suites, strict OpenSpec validation, + and `pnpm precommit`. +- [x] 9.3 Prepare the verified change for the review reply and pushed CI run. diff --git a/tests/contract/bundle.contract.test.ts b/tests/contract/bundle.contract.test.ts index 45fb218cb..a5b05d141 100644 --- a/tests/contract/bundle.contract.test.ts +++ b/tests/contract/bundle.contract.test.ts @@ -227,6 +227,16 @@ describe('annotation encoding across the language boundary', () => { ).toEqual(['__NA__']); }); + it('preserves a missing TMbed payload as N/A for signal peptide', () => { + expect( + getProteinAnnotationValues( + data, + manifest.missingTransmembraneIndex, + 'predicted_signal_peptide', + ), + ).toEqual(['__NA__']); + }); + it('reports a missing numeric value as missing rather than zero', () => { const lengths = data.numeric_annotation_data?.length; // Assert the length first: an out-of-range index yields `undefined`, which diff --git a/tests/contract/emit_bundles.py b/tests/contract/emit_bundles.py index 3a8b7ae5b..cef14d82c 100644 --- a/tests/contract/emit_bundles.py +++ b/tests/contract/emit_bundles.py @@ -104,6 +104,16 @@ def protein_ids(count: int) -> list[str]: ) ] ) +MISSING_SIGNAL_PEPTIDE_VALUE = BiocentralPredictionRetriever._extract_signal_peptide( + [ + Prediction( + model_name="TMbed", + prediction_name="topology", + protocol="per_residue", + value=None, + ) + ] +) def build_annotations_table(ids: list[str]) -> pa.Table: @@ -125,6 +135,9 @@ def build_annotations_table(ids: list[str]) -> pa.Table: predicted_transmembrane = [NEGATIVE_TMBED_CATEGORY, MISSING_TMBED_VALUE] + [""] * ( rest - 1 ) + predicted_signal_peptide = ["False", MISSING_SIGNAL_PEPTIDE_VALUE] + [""] * ( + rest - 1 + ) # A genuine double column with a null -- distinguishes "missing" from 0 and # from NaN across the language boundary. Real bundles carry both string-typed @@ -137,6 +150,7 @@ def build_annotations_table(ids: list[str]) -> pa.Table: "identifier": pa.array(ids, pa.string()), "family": pa.array(family, pa.string()), "domains": pa.array(domains, pa.string()), + "predicted_signal_peptide": pa.array(predicted_signal_peptide, pa.string()), "predicted_transmembrane": pa.array(predicted_transmembrane, pa.string()), "length": pa.array(length, pa.float64()), } From a30032a2ac858d92062b17aa17481dc4958cf78b Mon Sep 17 00:00:00 2001 From: Florin Senoner <23100806+FlorinSenoner@users.noreply.github.com> Date: Wed, 5 Aug 2026 14:42:58 +0200 Subject: [PATCH 4/6] fix(annotations): reject malformed tmbed payloads --- .../retrievers/biocentral_retriever.py | 12 +++- .../tests/test_biocentral_retriever.py | 19 ++++++ .../design.md | 12 ++-- .../proposal.md | 9 +-- .../specs/annotation-input/spec.md | 14 +++++ .../specs/bundle-format-contract/spec.md | 17 +---- .../tasks.md | 21 +++++++ tests/contract/bundle.contract.test.ts | 10 +++ tests/contract/emit_bundles.py | 63 ++++++++++--------- 9 files changed, 122 insertions(+), 55 deletions(-) create mode 100644 openspec/changes/fix-biocentral-transmembrane-sentinel/specs/annotation-input/spec.md diff --git a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py index 0cd4117d2..14da15fe0 100644 --- a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py +++ b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py @@ -24,6 +24,7 @@ } _BATCH_SIZE = 1000 +_TMBED_TOPOLOGY_LABELS = frozenset("BbHhSio.") class BiocentralPredictionRetriever(BaseAnnotationRetriever): @@ -229,10 +230,15 @@ def _extract_transmembrane(predictions: list) -> str: @staticmethod def _extract_tmbed_topology(predictions: list) -> str | None: - """Return a non-empty TMbed topology, or ``None`` when unavailable.""" + """Return a supported TMbed topology, or ``None`` when unavailable.""" for pred in predictions: if pred.model_name == "TMbed": - if pred.value is None or pred.value == "": + value = pred.value + if ( + not isinstance(value, str) + or not value + or not set(value) <= _TMBED_TOPOLOGY_LABELS + ): return None - return str(pred.value) + return value return None diff --git a/apps/protspace/tests/test_biocentral_retriever.py b/apps/protspace/tests/test_biocentral_retriever.py index a69183862..8487f78ee 100644 --- a/apps/protspace/tests/test_biocentral_retriever.py +++ b/apps/protspace/tests/test_biocentral_retriever.py @@ -2,6 +2,7 @@ from unittest.mock import MagicMock, patch +import pytest from biocentral_api._generated import Prediction from src.protspace.data.annotations.retrievers.biocentral_retriever import ( @@ -91,6 +92,11 @@ def test_no_transmembrane(self): result = BiocentralPredictionRetriever._extract_transmembrane(preds) assert result == "non-transmembrane" + def test_dot_label_is_non_transmembrane(self): + preds = [_make_real_tmbed_prediction("........")] + result = BiocentralPredictionRetriever._extract_transmembrane(preds) + assert result == "non-transmembrane" + def test_none_payload_is_missing(self): preds = [_make_real_tmbed_prediction(None)] result = BiocentralPredictionRetriever._extract_transmembrane(preds) @@ -113,6 +119,19 @@ def test_no_tmbed_prediction(self): assert result == "" +class TestTmbedPayloadValidation: + @pytest.mark.parametrize( + "value", + [0, [], {}, " ", b"abc", "garbage"], + ids=["zero", "list", "dict", "blank", "bytes", "unsupported-labels"], + ) + def test_malformed_payload_is_missing_for_derived_annotations(self, value): + preds = [_make_real_tmbed_prediction(value)] + + assert BiocentralPredictionRetriever._extract_signal_peptide(preds) == "" + assert BiocentralPredictionRetriever._extract_transmembrane(preds) == "" + + class TestPerSequenceExtraction: """Test per-sequence prediction extraction.""" diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md index 34b7fa490..8fdb34e5f 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md @@ -52,13 +52,15 @@ Alternatives considered: - Use `soluble`. This is biologically narrower than "no predicted transmembrane segment" and can mislabel secreted or otherwise non-membrane proteins. -### Preserve empty-string output for an absent TMbed result +### Preserve empty-string output for an unavailable or malformed TMbed result The adapter will return an empty string when Biocentral provides no TMbed prediction or -provides a TMbed prediction object whose optional `value` payload is `None` or empty. -The payload guard runs before topology labels are scanned. This retains the distinction -at the producer: a completed, non-empty topology with no membrane segment is -`non-transmembrane`, while an unavailable topology remains missing. +provides a TMbed prediction object whose optional `value` payload is `None`, empty, +non-string, blank, or contains unsupported labels. A usable topology is a string made +only from TMbed's `B`, `b`, `H`, `h`, `S`, `i`, `o`, and `.` labels. The payload guard +runs before topology labels are scanned. This retains the distinction at the producer: +a valid topology with no membrane segment is `non-transmembrane`, while an unavailable +or malformed topology remains missing. Both `predicted_signal_peptide` and `predicted_transmembrane` derive from that same optional topology payload. A shared extractor therefore owns the missing-payload check diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md index 082a76323..590ee7ccd 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md @@ -9,9 +9,9 @@ N/A and cannot be distinguished from proteins whose prediction is actually absen - Encode successful TMbed predictions with no membrane-spanning segment using a non-reserved categorical label. -- Preserve genuinely absent Biocentral predictions, including TMbed prediction - objects whose optional payload is `None` or empty, as missing values for both - derived TMbed annotations. +- Preserve absent or malformed Biocentral predictions, including TMbed prediction + objects whose optional payload is `None`, empty, non-string, or contains unsupported + topology labels, as missing values for both derived TMbed annotations. - Preserve FASTA sequences through the standalone `annotate` command so sequence-backed predictions do not depend on UniProt resolving the identifier. - Add regression coverage across the Python annotation producer and TypeScript @@ -22,7 +22,8 @@ N/A and cannot be distinguished from proteins whose prediction is actually absen ### New Capabilities -None. +- `annotation-input`: Require standalone FASTA annotation inputs to retain their + sequences for sequence-backed annotation sources. ### Modified Capabilities diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/annotation-input/spec.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/annotation-input/spec.md new file mode 100644 index 000000000..f6ef5467d --- /dev/null +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/annotation-input/spec.md @@ -0,0 +1,14 @@ +## ADDED Requirements + +### Requirement: FASTA annotation inputs retain their sequences + +The standalone annotation producer SHALL pass sequences supplied in a FASTA input to +sequence-backed annotation sources using the same canonical identifiers as its output. + +#### Scenario: A FASTA identifier is not resolved by UniProt + +- **WHEN** `protspace annotate` receives a FASTA entry whose identifier UniProt cannot + resolve +- **THEN** the entry's FASTA sequence remains available to Biocentral and InterPro +- **AND** the hosted prep pipeline preserves that sequence when it passes its normalized + FASTA to `protspace annotate` diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md index ecb412199..6f7ea8c99 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/bundle-format-contract/spec.md @@ -20,26 +20,13 @@ normalizing to missing data. - **THEN** the producer emits the established missing representation - **AND** the TypeScript visualization data does not invent a transmembrane category -#### Scenario: Empty TMbed payload remains missing +#### Scenario: Empty or malformed TMbed payload remains missing - **WHEN** Biocentral returns a TMbed prediction object whose optional `value` payload - is `None` or an empty string + is `None`, empty, non-string, blank, or contains unsupported topology labels - **THEN** the producer emits the established missing representation before scanning topology labels - **AND** the TypeScript visualization data exposes the protein as `N/A` rather than `non-transmembrane` - **AND** the derived signal-peptide annotation is also exposed as `N/A` rather than `False` - -### Requirement: FASTA annotation inputs retain their sequences - -The standalone annotation producer SHALL pass sequences supplied in a FASTA input to -sequence-backed annotation sources using the same canonical identifiers as its output. - -#### Scenario: A FASTA identifier is not resolved by UniProt - -- **WHEN** `protspace annotate` receives a FASTA entry whose identifier UniProt cannot - resolve -- **THEN** the entry's FASTA sequence remains available to Biocentral and InterPro -- **AND** the hosted prep pipeline preserves that sequence when it passes its normalized - FASTA to `protspace annotate` diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md index a024aaadb..29036173f 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md @@ -51,3 +51,24 @@ - [x] 9.2 Run Ruff, the complete relevant Python suites, strict OpenSpec validation, and `pnpm precommit`. - [x] 9.3 Prepare the verified change for the review reply and pushed CI run. + +## 10. Malformed Payload and Spec Ownership Regressions + +- [x] 10.1 Add real-model regressions proving malformed TMbed payloads remain missing + for both derived annotations and observe the expected failures. +- [x] 10.2 Add a producer/consumer regression proving a malformed TMbed payload reaches + TypeScript as `N/A` and observe the expected failure. +- [x] 10.3 Move the FASTA sequence requirement from `bundle-format-contract` to its + owning `annotation-input` capability within this change. + +## 11. Malformed Payload Fix + +- [x] 11.1 Restrict the shared TMbed topology extractor to non-blank strings containing + only supported topology labels while preserving all valid topology semantics. + +## 12. Final Review Verification + +- [x] 12.1 Run focused ProtSpace and cross-language contract tests. +- [x] 12.2 Run Ruff, the complete relevant Python and hosted-prep suites, strict OpenSpec + validation, and `pnpm precommit`. +- [x] 12.3 Prepare the verified change for inline review replies and pushed CI. diff --git a/tests/contract/bundle.contract.test.ts b/tests/contract/bundle.contract.test.ts index a5b05d141..4edbcb09e 100644 --- a/tests/contract/bundle.contract.test.ts +++ b/tests/contract/bundle.contract.test.ts @@ -43,6 +43,7 @@ interface Manifest { labelWithReservedChar: string; negativeTransmembraneCategory: string; missingTransmembraneIndex: number; + malformedTmbedIndex: number; nullLengthIndex: number; } @@ -237,6 +238,15 @@ describe('annotation encoding across the language boundary', () => { ).toEqual(['__NA__']); }); + it('preserves a malformed TMbed payload as N/A', () => { + expect( + getProteinAnnotationValues(data, manifest.malformedTmbedIndex, 'predicted_transmembrane'), + ).toEqual(['__NA__']); + expect( + getProteinAnnotationValues(data, manifest.malformedTmbedIndex, 'predicted_signal_peptide'), + ).toEqual(['__NA__']); + }); + it('reports a missing numeric value as missing rather than zero', () => { const lengths = data.numeric_annotation_data?.length; // Assert the length first: an out-of-range index yields `undefined`, which diff --git a/tests/contract/emit_bundles.py b/tests/contract/emit_bundles.py index cef14d82c..182ba9129 100644 --- a/tests/contract/emit_bundles.py +++ b/tests/contract/emit_bundles.py @@ -76,18 +76,24 @@ def protein_ids(count: int) -> list[str]: # swallows the second hit, which is exactly the bug the grammar exists to avoid. MULTI_HIT_CELL = f"{encode_field('DomA')}|0.91;{encode_field('DomB')}|0.82" -# Derive this fixture value from the real annotation adapter rather than copying -# its vocabulary into the contract test. A topology with only inside/outside -# labels is a completed TMbed prediction with no membrane-spanning segment. -NEGATIVE_TMBED_CATEGORY = BiocentralPredictionRetriever._extract_transmembrane( - [ + +def tmbed_predictions(value: object) -> list[Prediction]: + """Build the real generated prediction shape consumed by the adapter.""" + return [ Prediction( model_name="TMbed", prediction_name="topology", protocol="per_residue", - value="oooooiiiii", + value=value, ) ] + + +# Derive this fixture value from the real annotation adapter rather than copying +# its vocabulary into the contract test. A topology with only inside/outside +# labels is a completed TMbed prediction with no membrane-spanning segment. +NEGATIVE_TMBED_CATEGORY = BiocentralPredictionRetriever._extract_transmembrane( + tmbed_predictions("oooooiiiii") ) # Keep one explicit missing-payload row separate from the fixture's other empty @@ -95,24 +101,20 @@ def protein_ids(count: int) -> list[str]: # the bundle reader has a chance to normalize the missing representation. MISSING_TMBED_INDEX = 1 MISSING_TMBED_VALUE = BiocentralPredictionRetriever._extract_transmembrane( - [ - Prediction( - model_name="TMbed", - prediction_name="topology", - protocol="per_residue", - value=None, - ) - ] + tmbed_predictions(None) ) MISSING_SIGNAL_PEPTIDE_VALUE = BiocentralPredictionRetriever._extract_signal_peptide( - [ - Prediction( - model_name="TMbed", - prediction_name="topology", - protocol="per_residue", - value=None, - ) - ] + tmbed_predictions(None) +) + +# Malformed payloads are unavailable predictions, not completed negatives. Keep +# one explicit row so this producer decision is exercised through bundle ingestion. +MALFORMED_TMBED_INDEX = 2 +MALFORMED_TMBED_VALUE = BiocentralPredictionRetriever._extract_transmembrane( + tmbed_predictions("garbage") +) +MALFORMED_SIGNAL_PEPTIDE_VALUE = BiocentralPredictionRetriever._extract_signal_peptide( + tmbed_predictions("garbage") ) @@ -132,12 +134,16 @@ def build_annotations_table(ids: list[str]) -> pa.Table: encode_field("Hydrolase") ] * rest domains = [MULTI_HIT_CELL] + [f"{encode_field('DomB')}|0.75"] * rest - predicted_transmembrane = [NEGATIVE_TMBED_CATEGORY, MISSING_TMBED_VALUE] + [""] * ( - rest - 1 - ) - predicted_signal_peptide = ["False", MISSING_SIGNAL_PEPTIDE_VALUE] + [""] * ( - rest - 1 - ) + predicted_transmembrane = [ + NEGATIVE_TMBED_CATEGORY, + MISSING_TMBED_VALUE, + MALFORMED_TMBED_VALUE, + ] + [""] * (rest - 2) + predicted_signal_peptide = [ + "False", + MISSING_SIGNAL_PEPTIDE_VALUE, + MALFORMED_SIGNAL_PEPTIDE_VALUE, + ] + [""] * (rest - 2) # A genuine double column with a null -- distinguishes "missing" from 0 and # from NaN across the language boundary. Real bundles carry both string-typed @@ -361,6 +367,7 @@ def emit(item: tuple[str, tuple[int, list[str]]]) -> None: "labelWithReservedChar": LABEL_WITH_RESERVED_CHAR, "negativeTransmembraneCategory": NEGATIVE_TMBED_CATEGORY, "missingTransmembraneIndex": MISSING_TMBED_INDEX, + "malformedTmbedIndex": MALFORMED_TMBED_INDEX, "nullLengthIndex": NULL_LENGTH_INDEX, } ), From 1c0942948abbe1b06637fa47d108141c5d4f2603 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Thu, 6 Aug 2026 11:43:05 +0200 Subject: [PATCH 5/6] refactor(annotations): drop regex label scan and re-pad docs table - Replace the re.search([Hh])/([Bb]) topology probes in BiocentralPredictionRetriever._extract_transmembrane with plain substring membership tests, and drop the now-unused `import re`. - Re-pad the Biocentral annotation table in apps/protspace/docs/annotations.md so every Description cell is 47 characters wide again; apps/protspace/ is prettier-ignored, so the column alignment has to be maintained by hand. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016qoU16kDQxz6U3H2UWbbm2 --- apps/protspace/docs/annotations.md | 10 +++++----- .../annotations/retrievers/biocentral_retriever.py | 5 ++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/apps/protspace/docs/annotations.md b/apps/protspace/docs/annotations.md index 83fa298d2..8474cb0f0 100644 --- a/apps/protspace/docs/annotations.md +++ b/apps/protspace/docs/annotations.md @@ -192,11 +192,11 @@ Structure-based domain annotations from [TED (The Encyclopedia of Domains)](http Per-protein predictions from the [Biocentral API](https://biocentral.rostlab.org/) using pre-trained models. Requires protein sequences (fetched automatically from UniProt). -| Name | Model | Description | -| -------------------------------- | ------------------------------------ | ------------------------------------------ | -| `predicted_subcellular_location` | LightAttention | 10-class subcellular localization | -| `predicted_membrane` | LightAttention | Membrane / Soluble | -| `predicted_signal_peptide` | TMbed | True / False (derived from topology) | +| Name | Model | Description | +| -------------------------------- | ------------------------------------ | ----------------------------------------------- | +| `predicted_subcellular_location` | LightAttention | 10-class subcellular localization | +| `predicted_membrane` | LightAttention | Membrane / Soluble | +| `predicted_signal_peptide` | TMbed | True / False (derived from topology) | | `predicted_transmembrane` | TMbed | non-transmembrane / alpha-helical / beta-barrel | **Data source**: Batch predictions via Biocentral API (`api.predict()`). TMbed provides per-residue topology labels (`H`=TM helix, `B`=TM beta strand, `S`=signal peptide); signal peptide and transmembrane type are summarized from these labels. diff --git a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py index 14da15fe0..3b3d99a93 100644 --- a/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py +++ b/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py @@ -1,7 +1,6 @@ """Biocentral API prediction retriever for per-protein annotations.""" import logging -import re import warnings from protspace.data.annotations.retrievers.base_retriever import BaseAnnotationRetriever @@ -218,8 +217,8 @@ def _extract_transmembrane(predictions: list) -> str: if topology is None: return "" - has_helix = bool(re.search(r"[Hh]", topology)) - has_beta = bool(re.search(r"[Bb]", topology)) + has_helix = "H" in topology or "h" in topology + has_beta = "B" in topology or "b" in topology if has_helix and has_beta: return "alpha-helical;beta-barrel" elif has_helix: From f6cea0a74299a4bd1677da99f07282be9c6b970a Mon Sep 17 00:00:00 2001 From: Florin Senoner <23100806+FlorinSenoner@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:16:53 +0200 Subject: [PATCH 6/6] fix(annotations): preserve source-specific sequences --- apps/protspace/src/protspace/cli/annotate.py | 24 +++++---- .../src/protspace/data/annotations/manager.py | 14 +++-- apps/protspace/tests/test_annotate_cli.py | 51 ++++++++++++++++++- .../tests/test_annotation_manager.py | 32 ++++++++++++ docs/guide/annotations.md | 2 +- docs/scripts/annotation-details.ts | 2 +- .../design.md | 25 ++++++--- .../proposal.md | 5 +- .../specs/annotation-input/spec.md | 14 +++++ .../tasks.md | 24 +++++++++ tests/contract/bundle.contract.test.ts | 10 ++++ 11 files changed, 178 insertions(+), 25 deletions(-) diff --git a/apps/protspace/src/protspace/cli/annotate.py b/apps/protspace/src/protspace/cli/annotate.py index bb96d1e2f..b1e060d6c 100644 --- a/apps/protspace/src/protspace/cli/annotate.py +++ b/apps/protspace/src/protspace/cli/annotate.py @@ -61,17 +61,12 @@ def annotate( from protspace.data.loaders.h5 import EMBEDDING_EXTENSIONS # Extract identifiers from input + input_is_fasta = is_fasta_file(input) sequences = None - if is_fasta_file(input): - from protspace.data.io.fasta import parse_fasta - from protspace.data.loaders.h5 import parse_identifier + if input_is_fasta: from protspace.data.loaders.query import extract_identifiers_from_fasta headers = extract_identifiers_from_fasta(input) - sequences = { - parse_identifier(header): sequence - for header, sequence in parse_fasta(input).items() - } elif input.suffix.lower() in EMBEDDING_EXTENSIONS: from protspace.data.loaders.h5 import _collect_datasets @@ -89,10 +84,10 @@ def annotate( logger.info(f"Found {len(headers)} protein identifiers") # Resolve annotation names + from protspace.data.annotations.configuration import AnnotationConfiguration + annotations_list = None if annotations: - from protspace.data.annotations.configuration import AnnotationConfiguration - names = [] for item in annotations: for part in item.split(","): @@ -102,6 +97,17 @@ def annotate( if names: annotations_list = AnnotationConfiguration(names).user_annotations + if input_is_fasta: + config = AnnotationConfiguration(annotations_list) + if config.interpro_annotations or config.biocentral_annotations: + from protspace.data.io.fasta import parse_fasta + from protspace.data.loaders.h5 import parse_identifier + + sequences = { + parse_identifier(header): sequence + for header, sequence in parse_fasta(input).items() + } + # Fetch annotations df = ProteinAnnotationManager( headers=headers, diff --git a/apps/protspace/src/protspace/data/annotations/manager.py b/apps/protspace/src/protspace/data/annotations/manager.py index 26c3dbc75..94268c0bd 100644 --- a/apps/protspace/src/protspace/data/annotations/manager.py +++ b/apps/protspace/src/protspace/data/annotations/manager.py @@ -241,16 +241,20 @@ def _fetch_taxonomy( return {} def _build_sequence_map( - self, uniprot_annotations: list[ProteinAnnotations] + self, + uniprot_annotations: list[ProteinAnnotations], + *, + prefer_uniprot: bool = False, ) -> dict[str, str]: """Build a mapping from headers to sequences. - Merges local sequences (from FASTA, priority) with UniProt results (fallback). + Local FASTA sequences take priority by default. InterPro can request UniProt + priority because its precomputed matches are keyed by canonical sequence hash. """ sequences = dict(self.sequences) if self.sequences else {} for protein in uniprot_annotations: seq = protein.annotations.get("sequence", "") - if seq and protein.identifier not in sequences: + if seq and (prefer_uniprot or protein.identifier not in sequences): sequences[protein.identifier] = seq return sequences @@ -262,7 +266,9 @@ def _fetch_interpro( return [] try: - sequences = self._build_sequence_map(uniprot_annotations) + sequences = self._build_sequence_map( + uniprot_annotations, prefer_uniprot=True + ) retriever = InterProRetriever( headers=self.headers, diff --git a/apps/protspace/tests/test_annotate_cli.py b/apps/protspace/tests/test_annotate_cli.py index 39b25aad3..598dadf80 100644 --- a/apps/protspace/tests/test_annotate_cli.py +++ b/apps/protspace/tests/test_annotate_cli.py @@ -28,7 +28,15 @@ def to_pd(self): output = tmp_path / "annotations.parquet" result = CliRunner().invoke( app, - ["annotate", "-i", str(fasta), "-o", str(output)], + [ + "annotate", + "-i", + str(fasta), + "-a", + "biocentral", + "-o", + str(output), + ], ) assert result.exit_code == 0, result.output @@ -37,3 +45,44 @@ def to_pd(self): "P12345": "MKV", "custom-protein": "AAAG", } + + +def test_fasta_is_not_parsed_for_uniprot_only_annotations(tmp_path, monkeypatch): + """Sequence-free sources must not materialize an unused FASTA sequence map.""" + import protspace.data.annotations.manager as manager_module + import protspace.data.io.fasta as fasta_module + + fasta = tmp_path / "input.fasta" + fasta.write_text(">sp|P12345|KNOWN Known protein\nMKV\n") + captured: dict[str, object] = {} + + class FakeManager: + def __init__(self, *args, **kwargs): + captured.update(kwargs) + + def to_pd(self): + return pd.DataFrame({"identifier": captured["headers"]}) + + def fail_if_parsed(*args, **kwargs): + raise AssertionError("parse_fasta must not run for UniProt-only annotations") + + monkeypatch.setattr(manager_module, "ProteinAnnotationManager", FakeManager) + monkeypatch.setattr(fasta_module, "parse_fasta", fail_if_parsed) + + output = tmp_path / "annotations.parquet" + result = CliRunner().invoke( + app, + [ + "annotate", + "-i", + str(fasta), + "-a", + "uniprot", + "-o", + str(output), + ], + ) + + assert result.exit_code == 0, result.output + assert captured["headers"] == ["P12345"] + assert captured["sequences"] is None diff --git a/apps/protspace/tests/test_annotation_manager.py b/apps/protspace/tests/test_annotation_manager.py index d7dc308bf..749d14253 100644 --- a/apps/protspace/tests/test_annotation_manager.py +++ b/apps/protspace/tests/test_annotation_manager.py @@ -439,6 +439,38 @@ def test_transform_protein_families_with_semicolon(self): class TestIntegration: """Integration tests for complete workflows.""" + @patch("src.protspace.data.annotations.manager.BiocentralPredictionRetriever") + @patch("src.protspace.data.annotations.manager.InterProRetriever") + def test_sequence_precedence_is_source_specific( + self, mock_interpro_retriever, mock_biocentral_retriever + ): + """InterPro hashes canonical sequences; Biocentral predicts submitted ones.""" + manager = ProteinAnnotationManager( + headers=["P12345", "custom-protein"], + annotations=["pfam", "predicted_transmembrane"], + sequences={"P12345": "LOCAL", "custom-protein": "CUSTOM"}, + ) + uniprot_annotations = [ + ProteinAnnotations( + identifier="P12345", annotations={"sequence": "CANONICAL"} + ), + ProteinAnnotations(identifier="custom-protein", annotations={}), + ] + mock_interpro_retriever.return_value.fetch_annotations.return_value = [] + mock_biocentral_retriever.return_value.fetch_annotations.return_value = [] + + manager._fetch_interpro(uniprot_annotations, []) + manager._fetch_biocentral(uniprot_annotations, []) + + assert mock_interpro_retriever.call_args.kwargs["sequences"] == { + "P12345": "CANONICAL", + "custom-protein": "CUSTOM", + } + assert mock_biocentral_retriever.call_args.kwargs["sequences"] == { + "P12345": "LOCAL", + "custom-protein": "CUSTOM", + } + @patch("src.protspace.data.annotations.manager.TaxonomyRetriever") @patch("src.protspace.data.annotations.manager.UniProtRetriever") def test_to_pd_complete_workflow( diff --git a/docs/guide/annotations.md b/docs/guide/annotations.md index beed2317d..c5778ca47 100644 --- a/docs/guide/annotations.md +++ b/docs/guide/annotations.md @@ -60,7 +60,7 @@ LightAttention is a lightweight neural network that uses softmax-weighted aggreg Transmembrane type (non-transmembrane / alpha-helical / beta-barrel) predicted by TMbed. -From the same TMbed per-residue topology (H = transmembrane helix, B = transmembrane beta strand, S = signal peptide), ProtSpace summarizes the membrane-spanning segments into a single protein-level category. Values are `alpha-helical` when transmembrane helices (H) are predicted, `beta-barrel` when transmembrane beta strands (B) are predicted, and `non-transmembrane` when neither is present. See [Bernhofer & Rost, BMC Bioinformatics 2022](https://doi.org/10.1186/s12859-022-04873-x). +From the same TMbed per-residue topology (H = transmembrane helix, B = transmembrane beta strand, S = signal peptide), ProtSpace summarizes the membrane-spanning segments into protein-level categories. Values are `alpha-helical` when transmembrane helices (H) are predicted, `beta-barrel` when transmembrane beta strands (B) are predicted, and `non-transmembrane` when neither is present. A protein with both segment types carries both the `alpha-helical` and `beta-barrel` categories. See [Bernhofer & Rost, BMC Bioinformatics 2022](https://doi.org/10.1186/s12859-022-04873-x). ## UniProt diff --git a/docs/scripts/annotation-details.ts b/docs/scripts/annotation-details.ts index 73018f976..df972ea0a 100644 --- a/docs/scripts/annotation-details.ts +++ b/docs/scripts/annotation-details.ts @@ -57,7 +57,7 @@ export const ANNOTATION_DETAILS: Record = { }, predicted_transmembrane: { detailsMarkdown: - 'From the same TMbed per-residue topology (H = transmembrane helix, B = transmembrane beta strand, S = signal peptide), ProtSpace summarizes the membrane-spanning segments into a single protein-level category. Values are `alpha-helical` when transmembrane helices (H) are predicted, `beta-barrel` when transmembrane beta strands (B) are predicted, and `non-transmembrane` when neither is present. See [Bernhofer & Rost, BMC Bioinformatics 2022](https://doi.org/10.1186/s12859-022-04873-x).', + 'From the same TMbed per-residue topology (H = transmembrane helix, B = transmembrane beta strand, S = signal peptide), ProtSpace summarizes the membrane-spanning segments into protein-level categories. Values are `alpha-helical` when transmembrane helices (H) are predicted, `beta-barrel` when transmembrane beta strands (B) are predicted, and `non-transmembrane` when neither is present. A protein with both segment types carries both the `alpha-helical` and `beta-barrel` categories. See [Bernhofer & Rost, BMC Bioinformatics 2022](https://doi.org/10.1186/s12859-022-04873-x).', sourceUrl: 'https://doi.org/10.1186/s12859-022-04873-x', }, diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md index 8fdb34e5f..3481c7613 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/design.md @@ -67,13 +67,21 @@ optional topology payload. A shared extractor therefore owns the missing-payload before either annotation interprets the topology, preventing one annotation from inventing a completed negative result when the other reports missing data. -### Pass FASTA sequences through the standalone annotation command - -The standalone `annotate` command will parse the FASTA into a canonical identifier to -sequence map and pass it to `ProteinAnnotationManager`. The manager already gives local -sequences priority and uses UniProt sequences only as fallback, so this closes the input -handoff without changing annotation-source behavior. HDF5 inputs continue to omit a -local sequence map and retain their existing UniProt-fallback behavior. +### Pass FASTA sequences to the sources that consume them + +When a requested source consumes sequences, the standalone `annotate` command will +parse the FASTA into a canonical identifier-to-sequence map and pass it to +`ProteinAnnotationManager`. UniProt-only and taxonomy-only requests will retain the +streaming identifier scan without materializing the full sequence map. HDF5 inputs +continue to omit a local sequence map and retain their existing UniProt-fallback +behavior. + +Sequence precedence is source-specific. Biocentral predicts the sequence supplied in +the FASTA, using UniProt only as a fallback for identifiers with no local sequence. +InterPro indexes precomputed matches by sequence hash, so it uses UniProt's canonical +sequence when available and falls back to the FASTA sequence for identifiers UniProt +cannot resolve. This keeps FASTA-only identifiers annotatable without replacing valid +canonical InterPro matches when the submitted sequence differs. The hosted prep service already normalizes FASTA headers and supplies that normalized file to `protspace annotate`; regression coverage pins that normalization preserves the @@ -97,6 +105,9 @@ producer and contract fixtures to drift apart. contract correction and document the new vocabulary in both CLI and web metadata. - [Contract fixture imports more producer code] → Use a minimal synthetic TMbed prediction and retain the real bundle CLI as the serialization boundary. +- [FASTA and UniProt sequences differ] → Keep the submitted sequence authoritative + for Biocentral predictions and the canonical sequence authoritative for InterPro's + precomputed hash lookup, with the other source as fallback in each case. ## Migration Plan diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md index 590ee7ccd..546adf648 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/proposal.md @@ -12,8 +12,9 @@ N/A and cannot be distinguished from proteins whose prediction is actually absen - Preserve absent or malformed Biocentral predictions, including TMbed prediction objects whose optional payload is `None`, empty, non-string, or contains unsupported topology labels, as missing values for both derived TMbed annotations. -- Preserve FASTA sequences through the standalone `annotate` command so - sequence-backed predictions do not depend on UniProt resolving the identifier. +- Preserve FASTA sequences through the standalone `annotate` command when a requested + source needs them, using source-appropriate precedence when UniProt also supplies a + canonical sequence. - Add regression coverage across the Python annotation producer and TypeScript bundle consumer boundary. - Update generated annotation documentation to describe the corrected category. diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/annotation-input/spec.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/annotation-input/spec.md index f6ef5467d..b87929169 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/annotation-input/spec.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/specs/annotation-input/spec.md @@ -12,3 +12,17 @@ sequence-backed annotation sources using the same canonical identifiers as its o - **THEN** the entry's FASTA sequence remains available to Biocentral and InterPro - **AND** the hosted prep pipeline preserves that sequence when it passes its normalized FASTA to `protspace annotate` + +#### Scenario: A FASTA sequence differs from the UniProt canonical sequence + +- **WHEN** `protspace annotate` receives a FASTA sequence for an identifier that UniProt + resolves to a different canonical sequence +- **THEN** Biocentral receives the FASTA sequence it is being asked to predict +- **AND** InterPro receives the UniProt canonical sequence used by its precomputed + sequence-hash index + +#### Scenario: Requested annotations do not consume sequences + +- **WHEN** `protspace annotate` receives a FASTA input and the requested annotation + sources are UniProt-only or taxonomy-only +- **THEN** the command extracts identifiers without materializing the FASTA sequences diff --git a/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md index 29036173f..5c17af02d 100644 --- a/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md +++ b/openspec/changes/fix-biocentral-transmembrane-sentinel/tasks.md @@ -72,3 +72,27 @@ - [x] 12.2 Run Ruff, the complete relevant Python and hosted-prep suites, strict OpenSpec validation, and `pnpm precommit`. - [x] 12.3 Prepare the verified change for inline review replies and pushed CI. + +## 13. Source Precedence and Optimized-Path Regressions + +- [x] 13.1 Add a manager regression proving InterPro prefers a UniProt canonical + sequence while Biocentral prefers the submitted FASTA sequence, and observe it + fail. +- [x] 13.2 Add a CLI regression proving UniProt-only FASTA annotation does not parse or + retain sequences, and observe it fail. +- [x] 13.3 Extend the optimized bundle conversion contract with the negative and missing + TMbed sentinel assertions. + +## 14. Source-Aware Sequence Handling and Documentation + +- [x] 14.1 Apply source-specific sequence precedence in the annotation manager. +- [x] 14.2 Parse FASTA sequences only when a requested annotation source consumes them. +- [x] 14.3 Document that mixed TMbed predictions expose both transmembrane categories. + +## 15. Latest Review Verification + +- [x] 15.1 Run focused annotation-manager, CLI, and bundle-contract checks. +- [x] 15.2 Run Ruff, the complete relevant Python and hosted-prep suites, strict OpenSpec + validation, and `pnpm precommit`. +- [x] 15.3 Prepare the verified changes and per-item dispositions for the newest review + reply. diff --git a/tests/contract/bundle.contract.test.ts b/tests/contract/bundle.contract.test.ts index 4edbcb09e..dc3eac728 100644 --- a/tests/contract/bundle.contract.test.ts +++ b/tests/contract/bundle.contract.test.ts @@ -306,6 +306,16 @@ describe('the optimized conversion path real datasets take', () => { expect(data.annotations.family.values.join('|')).not.toContain('%3B'); expect(data.annotations.domains.values).toContain('DomA'); expect(data.annotations.domains.values).toContain('DomB'); + expect(data.annotations.predicted_transmembrane.values).toContain( + manifest.negativeTransmembraneCategory, + ); + expect( + getProteinAnnotationValues( + data, + manifest.missingTransmembraneIndex, + 'predicted_transmembrane', + ), + ).toEqual(['__NA__']); const lengths = data.numeric_annotation_data?.length; expect(lengths).toHaveLength(manifest.largeProteinCount);