fix(annotations): preserve negative transmembrane predictions

[FlorinSenoner]

Root cause

Biocentral TMbed emitted the valid negative category none, while the web bundle consumer intentionally reserves none as a missing-value sentinel. Completed negative predictions were therefore normalized to N/A and became indistinguishable from absent predictions.

Fix

• Emit non-transmembrane for completed TMbed predictions with no alpha-helical or beta-barrel segment.
• Preserve empty output when a TMbed prediction is unavailable.
• Cover the real Python → bundle → TypeScript boundary and update annotation documentation.
• Add the OpenSpec change artifacts for the corrected contract.

Reproduction and verification

Using the issue attachments, the original 149-protein bundle displayed N/A: 149 for Transmembrane even though its annotation table contained 37 none predictions and 112 empty values. A regenerated validation bundle now displays non-transmembrane: 37 and N/A: 112, with no browser warnings or errors.

Tests

• pytest apps/protspace/tests/test_biocentral_retriever.py -q — 14 passed
• generated Python-to-TypeScript bundle contract — 12 passed
• ruff check and ruff format --check for apps/protspace — passed
• pytest -m "not slow" -q — 787 passed, 6 deselected
• pnpm precommit — passed before commit and push
• openspec validate fix-biocentral-transmembrane-sentinel --strict — passed

Closes #339

[FlorinSenoner]

CI blocker: the required Bundle format contract check fails before installing dependencies or running this PR's code. actions/setup-python@v6 references the repository-wide missing file apps/protspace/.python-version (run: https://github.com/tsenoner/protspace/actions/runs/30709633198/job/91394694001). The #339 regression passes locally (12/12 contract tests), but this infrastructure defect affects the other issue PRs identically and requires a separate authorized CI-fix PR. No CI/workflow change is included here; this PR remains draft.

[FlorinSenoner]

[P1] Preserve missing TMbed payloads

biocentral-api 1.2.1 declares Prediction.value as optional. On this head, constructing its real Prediction model with value=None or value="" reaches topology = "" above and this line returns non-transmembrane for both cases. That invents a completed negative biological prediction when TMbed supplied no payload, violating the new OpenSpec scenario that unavailable predictions remain missing. Please return "" before scanning when pred.value is absent/empty (and add focused None/empty-payload tests plus a contract assertion that the missing row stays N/A).

[FlorinSenoner]

Fixed in 5ec9fe2. The extractor now returns the established missing representation when the real Biocentral Prediction.value is None or empty, before scanning topology labels. Added focused real-model tests for both payloads and generated-bundle contract coverage proving the indexed missing value reaches TypeScript as NA, while a completed non-empty inside/outside topology remains non-transmembrane. Fresh local results: focused 16 passed, bundle contract 13 passed, full ProtSpace suite 789 passed (6 deselected), Ruff clean, strict OpenSpec valid, and pnpm precommit passed.

[tsenoner]

Automated review

Does it solve #339? Yes — I confirmed the root cause against the reporter's bundle: predicted_transmembrane is the literal none for 37 rows and empty for 112, and none is in MISSING_VALUE_TOKENS, so normalizeMissingValue collapsed all 149 to N/A at ingestion. Renaming the negative category to non-transmembrane at the producer removes the collision on every consumer I traced (web reader, Dash standardize_missing, stats/annotation_select, cli/transfer — all list none, none list non-transmembrane), and the cross-language contract assertion derives its fixture value from the adapter, so a revert to none fails the TypeScript test. After this change the reporter's 37 UniProt-resolved proteins reach parity with the signal-peptide column they said "worked". The remaining 112 blank rows are a separate defect, below.

Found 2 issues:

1. protspace annotate never passes FASTA sequences to the annotation manager, so Biocentral predictions stay empty for every identifier UniProt cannot resolve — 112 of the reporter's 149 proteins will still show N/A after this fix. — annotate reads only identifiers (extract_identifiers_from_fasta) and constructs ProteinAnnotationManager without sequences=, so _build_sequence_map starts empty and _extract_annotation bails at if not seq: return ""; prepare does not have this hole (it builds a sequence map from embedding_sets), and apps/prep runs exactly this command. Worth its own issue before [BUG] Transmembrane prediction using Biocentral not working #339 is closed.

protspace/apps/protspace/src/protspace/cli/annotate.py

Lines 98 to 104 in 5ec9fe2

	# Fetch annotations
	df = ProteinAnnotationManager(
	headers=headers,
	annotations=annotations_list,
	output_path=None,
	).to_pd()

2. The new empty-payload guard was added to _extract_transmembrane only; _extract_signal_peptide reads the same TMbed prediction object and still turns an absent payload into a confident "False". — Unlike the old none, False is not a missing-value token, so it renders as a real category indistinguishable from a genuine negative call; proposal.md states the rule generally for "TMbed prediction objects whose optional payload is None or empty". A shared _tmbed_topology(predictions) -> str | None would state it once for both extractors.

protspace/apps/protspace/src/protspace/data/annotations/retrievers/biocentral_retriever.py

Lines 204 to 210 in 5ec9fe2

	"""
	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 ""

🤖 Generated with Claude Code

_{Reviewed at 5ec9fe2 against issue #339.}

[FlorinSenoner]

Verified independently against current head 5ec9fe2d, the full diff, OpenSpec delta, and surrounding call paths/tests.

1. Actionable; separate follow-up. protspace annotate extracts only FASTA identifiers and constructs ProteinAnnotationManager without sequences=; _build_sequence_map can therefore fall back only to sequences returned by UniProt. apps/prep does invoke that CLI on the normalized FASTA, so non-UniProt identifiers remain unpredicted. This is distinct from this PR's sentinel-collision fix, but it should be tracked before [BUG] Transmembrane prediction using Biocentral not working #339 is closed. Required direction: parse/pass the FASTA sequence map and add CLI plus hosted-prep regression coverage.
2. Actionable on the current TMbed contract. _extract_signal_peptide converts None/"" to an empty topology and then returns "False", while _extract_transmembrane now preserves the same payload as missing. Required direction: centralize TMbed payload extraction (or apply an equivalent missing-payload guard) and add real-model None/empty signal-peptide coverage, including the bundle-visible result.

Neither case is addressed on this head. Current CI is green, but its existing tests do not exercise these paths.

[FlorinSenoner]

Implemented both actionable items in b7f798b3.

1. protspace annotate now passes a canonical FASTA sequence map into ProteinAnnotationManager; focused CLI coverage pins non-UniProt IDs, and hosted-prep coverage pins the normalized FASTA sequence handoff.
2. Signal-peptide and transmembrane extraction now share one missing-payload guard, so real TMbed None/empty payloads remain missing for both annotations. The generated bundle contract confirms missing signal peptide reaches TypeScript as N/A.

Fresh verification: focused Python 19 passed; hosted-prep full suite passed; ProtSpace non-slow suite 792 passed, 6 deselected; bundle contract 14 passed; Ruff clean; strict OpenSpec valid; pnpm precommit passed.

[FlorinSenoner]

[P2] Reject malformed TMbed payloads before deriving categories

biocentral-api 1.2.1 generates Prediction.value as Optional[Any], so the real model accepts non-string values. The new str() fallback makes them look like topology: in a fresh reproduction, 0, [], {}, and whitespace all produced signal peptide False plus non-transmembrane; b"abc" and "garbage" even produced beta-barrel because their string representations contain b. Those are malformed/unavailable payloads, not completed negative calls. Please require a non-blank string containing only supported TMbed topology labels before returning it (otherwise preserve missing), with real-model and bundle-visible regressions for malformed payloads.

[FlorinSenoner]

Fixed in a30032a. The shared extractor now accepts only non-empty strings composed of supported TMbed labels (B/b, H/h, S, i/o, .); non-string, blank, and unsupported payloads remain missing for both derived annotations. Added real generated-model coverage for zero/list/dict/blank/bytes/unsupported text plus a generated-bundle assertion that malformed signal/transmembrane values reach TypeScript as N/A. Fresh verification: focused 26 passed, contract 15 passed, full ProtSpace 799 passed (6 deselected), hosted-prep full suite passed, Ruff, strict OpenSpec, and pnpm precommit passed.

[FlorinSenoner]

[P2] Keep the FASTA-input contract in its own capability

This delta lives under bundle-format-contract, whose canonical spec is explicitly scoped to .parquetbundle layout, encoding, fixture behavior, and the cross-language CI boundary. Lines 34–45 specify standalone protspace annotate input behavior instead, so archiving this change would put an unrelated runtime requirement into the bundle-format source of truth even though strict validation passes. Please move this requirement to a dedicated annotation-input/producer capability within the same change and update the proposal capability list accordingly.

[FlorinSenoner]

Fixed in a30032a. The FASTA sequence requirement now lives only under the annotation-input capability delta in the existing change; bundle-format-contract retains only the producer/bundle/TypeScript output contract. The proposal declares annotation-input under New Capabilities, and strict OpenSpec validation passes.

[tsenoner]

Adversarial review

Reviewed in an isolated worktree by three independent lenses (code quality, adversarial correctness, issue-resolution audit), with every finding then put through a refuter whose default position was that it is a false positive. 9 raised, 4 survived refutation.

Applied and pushed (1c094294)

Behavior-preserving cleanups, verified green before pushing:

• Fix 1 (biocentral_retriever.py): replaced bool(re.search(r"[Hh]", topology)) / bool(re.search(r"[Bb]", topology)) at lines 221-222 with "H" in topology or "h" in topology / "B" in topology or "b" in topology, and deleted the now-unused import re at line 4. The if/elif chain below was left untouched. Verified re had no other uses in the module (grep showed only lines 4, 221, 222).
• Fix 2 (apps/protspace/docs/annotations.md): re-padded the Biocentral table (lines 195-200) so every Description cell is 47 chars wide and all six lines are 125 chars. Header Description padded with 36 trailing spaces, separator cell widened to 47 dashes, 10-class subcellular localization +14 spaces, Membrane / Soluble +29, True / False (derived from topology) +11, and the already-47-char non-transmembrane / alpha-helical / beta-barrel row left as is. No cell text changed.

Issue resolution — resolves the issue

The triage is sound and, unusually, verifiable — I reproduced the producer behavior against the live
Biocentral API rather than taking the description on faith. Nothing was narrowed or declined
improperly; if anything the PR is slightly wider than #339.
On the three judgment calls:

1. Choosing non-transmembrane over the alternatives holds up. Removing 'none' from
   MISSING_VALUE_TOKENS would change how arbitrary user CSV imports are read (the same helper serves
   both paths — conversion.ts:100/548/863), and special-casing one column name in the generic reader
   would leave the on-disk producer contract ambiguous. Both rejections are accurate as written.
   soluble was correctly rejected as biologically wrong.
2. The extra scope — routing FASTA sequences through protspace annotate (cli/annotate.py:64-74) —
   is not [BUG] Transmembrane prediction using Biocentral not working #339's bug, and the PR correctly does not claim it is. It is nonetheless a genuine adjacent
   defect: ProteinAnnotationManager._build_sequence_map (manager.py:243-255) only had UniProt-derived
   sequences on the standalone path, so a FASTA of non-UniProt IDs got "" for all four Biocentral
   columns. Notably the hosted service runs exactly this command
   (apps/prep/src/protspace_prep/pipeline.py:116-124), so this closes a real hosted-path hole. Key
   alignment is correct: extract_identifiers_from_fasta (query.py:85-95) and the new map both key on
   parse_identifier, and the added test pins it. Two small residual notes: parse_fasta loads the
   whole FASTA into memory, and two raw headers normalizing to the same identifier silently collapse
   (last wins).
3. The 'malformed payload is missing, not a negative' decision is the right call and is the one
   place where I would still push back — see the fail-closed/silent gap above. It is a correctness
   improvement (the old code invented predicted_signal_peptide = "False" from an empty payload)
   bought with a new total-failure mode that has no telemetry.
   Closing semantics: 'Closes [BUG] Transmembrane prediction using Biocentral not working #339' is warranted. The defect is in the producer, the producer is fixed,
   both consumers (web reader and the Python serve/stats viewer, which has its own token list at
   core/constants.py:8) accept the new value, the change is covered by a cross-language CI contract,
   and delivery to the hosted backend is automatic via publish-images.yml. The one caveat that should
   go in the closing comment: the reporter must re-run the pipeline, because nothing migrates their
   existing .parquetbundle — and the same is true of the repo's own phosphatase example, which should
   be regenerated before or shortly after this merges.

Gaps found by the issue audit (5)

• No remediation for already-generated bundles — including the repo's own shipped example. I decoded every bundle under apps/web/public/data/: apps/web/public/data/phosphatase.parquetbundle still carries predicted_transmembrane = 'none' for 1383 of 1587 proteins (87%), plus 203 'alpha-helical' and 1 'alpha-helical;beta-barrel'. That file is git-tracked, listed in apps/web/public/data/datasets.json, and Vite copies public/ into the Pages deploy, so after this PR merges the project's own dataset still reproduces the exact symptom of [BUG] Transmembrane prediction using Biocentral not working #339. The design doc's non-goal ('Rewriting or migrating already generated .parquetbundle files') is defensible for third-party files but did not account for first-party shipped data.
  • Why it matters: The reporter (and anyone else) must regenerate to see the fix, and the repo/site still ships data that demonstrates the bug — an easy way for [BUG] Transmembrane prediction using Biocentral not working #339 to be re-reported against a 'fixed' build.
  • Suggested follow-up: Regenerate apps/web/public/data/phosphatase.parquetbundle with the fixed CLI (or drop the predicted_* columns from it), and add a line to the release notes / docs/guide/annotations.md telling users with existing bundles to re-run protspace annotate + protspace bundle to recover the distinction.
• The new payload guard fails closed and fails silently. _extract_tmbed_topology (biocentral_retriever.py:231-244) rejects the whole payload unless set(value) <= frozenset("BbHhSio.") and returns None with no logging. I confirmed the allowlist matches the server today (metadata classes B,b,H,h,S,i,o; live payloads used only S,o,H,h,i — note '.' is a hedge that the server does not currently emit). But a single unexpected character in a future server version — a trailing newline, a new class, space-separated tokens — silently turns BOTH predicted_transmembrane and predicted_signal_peptide into N/A for every protein, i.e. it regenerates issue [BUG] Transmembrane prediction using Biocentral not working #339 in a strictly worse form with zero diagnostics.
  • Why it matters: The failure mode is indistinguishable from 'Biocentral had no prediction', which is precisely what made [BUG] Transmembrane prediction using Biocentral not working #339 hard to diagnose in the first place; the guard converts a partial-vocabulary drift into a total data loss.
  • Suggested follow-up: Emit a logger.warning (rate-limited or once-per-run with a count) naming the model and the offending characters when a TMbed payload is rejected, and consider narrowing the guard to 'is a non-blank str' + label scan, treating unknown labels as non-membrane rather than as missing.
• The new contract assertions only cover the small-data conversion path. tests/contract/bundle.contract.test.ts:215-248 read the 10-protein minimal bundle through convertParquetToVisualizationData. The 'the optimized conversion path real datasets take' block (line 285-318) — the path every production-sized dataset takes, per the fixture's own comment — asserts family/domains/length but never predicted_transmembrane, even though build_annotations_table emits the column into the large fixture too.
  • Why it matters: Both paths currently share splitCategoricalAnnotationValues, so behavior is identical today, but the suite exists precisely to catch the two decoders diverging; the sentinel regression it was extended to prevent is unguarded on the decoder real users hit.
  • Suggested follow-up: Add the two lines to the optimized describe block: assert data.annotations.predicted_transmembrane.values contains manifest.negativeTransmembraneCategory and that getProteinAnnotationValues(data, manifest.missingTransmembraneIndex, 'predicted_transmembrane') is ['__NA__'].
• The underlying bug class is broader than the producer obligation the new spec encodes, and pass-through columns cannot satisfy it. Scanning all string columns of the shipped bundles turned up the same collision in data the producer does not own: gene_name = 'NA' in 145 rows of 573K_swissprot.parquetbundle, 15 rows of 35K_ec_brenda, and 'na'/'nan' in 105K_homoSapiens_drosophilaMelanogaster. 'NA' is a legitimate gene symbol (neuraminidase), and the producer cannot rename it. The design's alternatives list considered 'remove none from the token set' and 'special-case predicted_transmembrane in the reader', but not the option that would have covered both cases and fixed existing files: scope the text-token normalization by input format, since a ProtSpace-written bundle already encodes missing as empty string (apps/protspace/src/protspace/data/processors/base_processor.py:199 df.fillna("")), so text sentinels inside a produced bundle are always real values.
  • Why it matters: Users are silently losing real categorical values in the flagship Swiss-Prot dataset today, by the same mechanism as [BUG] Transmembrane prediction using Biocentral not working #339, and the new bundle-format-contract requirement is unsatisfiable for UniProt pass-through fields.
  • Suggested follow-up: Open a follow-up issue to apply MISSING_VALUE_TOKENS only on the user-import (CSV/table) path and not to columns read from a producer-stamped (protspace_format_version) bundle, where '' is already the sole missing encoding; that would also retroactively fix every existing bundle including the reporter's.
• Documentation still omits the fourth emitted value. The PR rewrote the vocabulary sentence in docs/guide/annotations.md:61-63, docs/scripts/annotation-details.ts:60, packages/utils/src/visualization/annotation-metadata.ts:358 and apps/protspace/docs/annotations.md:200 as 'non-transmembrane / alpha-helical / beta-barrel', but the adapter also emits the multi-hit cell 'alpha-helical;beta-barrel' (biocentral_retriever.py:224), which the reader splits into two legend entries.
  • Why it matters: The PR was explicitly correcting these strings for accuracy, and a user who sees a protein in both categories has no documentation for it.
  • Suggested follow-up: Add one clause to the generated description noting that a protein predicted to have both segment types carries both categories, then re-run pnpm docs:annotations.

Findings needing a decision (2)

These were left for you rather than auto-applied: each changes behavior, needs a product call, or reaches outside this diff.

1. Passing FASTA sequences from annotate makes local sequences outrank UniProt's canonical sequence, which silently empties every InterPro column when the two differ.

apps/protspace/src/protspace/cli/annotate.py:71 · medium · correctness

ProteinAnnotationManager._build_sequence_map (manager.py:250-254) seeds the map with
self.sequences and only fills in a UniProt sequence if seq and protein.identifier not in sequences — i.e. a locally supplied sequence always wins. InterProRetriever is not accession-
based: it looks proteins up by hashlib.md5(sequence).hexdigest().upper() against InterPro's
precomputed match set (interpro_retriever.py:163-166, 271-275). Concrete failure: protspace annotate -i mine.fasta -a pfam where mine.fasta holds >sp|P00750|TPA_HUMAN with the mature
(signal-cleaved) chain — or merely a trailing *, or lowercase-masked residues, as many tool-
emitted FASTAs carry. Before this PR sequences=None, so the map came only from the UniProt-fetched
sequence annotation (configuration.py:244 forces sequence into the UniProt request whenever
InterPro or Biocentral is requested), the MD5 matched, and
pfam/superfamily/cath/smart/cdd/panther/prosite/prints/pfam_clan were populated. After this PR the
MD5 is of the user's sequence, InterPro answers found: false, and all nine InterPro columns come
back empty for that protein — with no warning, because missing_sequences is empty (a sequence
was present). design.md's claim that this "closes the input handoff without changing annotation-
source behavior" is wrong for InterPro.

Suggested fix

Product decision required - pick one and pin it with a test. Minimal option (does not touch
prepare): in apps/protspace/src/protspace/data/annotations/manager.py, stop sharing one map
between the two sources - keep _build_sequence_map (local-first) for _fetch_biocentral, and give
_fetch_interpro a UniProt-first map, e.g. add
def _build_uniprot_first_sequence_map(self, uniprot_annotations):
sequences = dict(self.sequences) if self.sequences else {}
for protein in uniprot_annotations:
seq = protein.annotations.get("sequence", "")
if seq:
sequences[protein.identifier] = seq
return sequences
and call it from _fetch_interpro instead of _build_sequence_map. FASTA-only identifiers UniProt
cannot resolve have no UniProt sequence, so they still reach InterPro via the local fallback. Add a
regression test asserting that when both a local and a UniProt sequence exist for the same
accession, InterPro hashes the UniProt one while Biocentral receives the local one, and correct the
design.md sentence "closes the input handoff without changing annotation-source behavior" to state
which source consumes which sequence.

2. The FASTA is now read twice and every sequence held in memory even when no requested annotation source consumes sequences.

apps/protspace/src/protspace/cli/annotate.py:65 · low · efficiency

extract_identifiers_from_fasta(input) streams the file, then parse_fasta(input) reads it a
second time and materializes {header: sequence} for the whole file — unconditionally, before
annotations_list is even resolved (that happens 20 lines later). protspace annotate -i uniprot_sprot.fasta -a organism,kingdom (taxonomy only, no InterPro/Biocentral) now performs a
second full pass over a ~280 MB file and retains ~570K Python strings (roughly 250-350 MB resident
with dict overhead) that no retriever will ever read; previously that command was streaming and
allocation-free. configuration.py:244 already knows the answer (needs_sequence = bool(interpro_annotations or biocentral_annotations)).

Suggested fix

In apps/protspace/src/protspace/cli/annotate.py, hoist the annotation-name resolution block
(currently lines 91-103) above the input-parsing block, then gate the sequence parse on it:
sequences = None
if is_fasta_file(input):
from protspace.data.loaders.query import extract_identifiers_from_fasta
headers = extract_identifiers_from_fasta(input)
cfg = AnnotationConfiguration(annotations_list) # None => defaults
if cfg.interpro_annotations or cfg.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()
}
Pass None (not []) to AnnotationConfiguration so the default group is still selected. Keep the
AnnotationConfiguration import at the top of the hoisted block. Verify the moved
typer.BadParameter ordering is still acceptable (an unknown annotation name will now be rejected
before an unsupported input suffix), and update
apps/protspace/tests/test_annotate_cli.py::test_fasta_sequences_are_passed_to_annotation_manager to
invoke with an explicit sequence-backed source (e.g. -a biocentral) so it still exercises the map.

5 further finding(s) were raised and refuted during verification.

[FlorinSenoner]

Addressed the actionable items from the 2026-08-06 review in f6cea0a7.

1. Tracked example bundle — deferred by scope. This change intentionally does not rewrite existing .parquetbundle files; that remains an explicit OpenSpec non-goal and migration risk. Newly generated annotations carry non-transmembrane. Existing user and first-party bundles must be regenerated, so the closing guidance will call that out rather than silently changing committed binary data in this producer-contract PR.
2. Malformed-payload telemetry — no code change. The fail-closed behavior remains intentional: an unsupported label cannot safely be reclassified as a completed biological negative. Logging inside _extract_tmbed_topology would fire once for each derived annotation (twice per protein) and can flood a large run; the repository has no once-per-run aggregation boundary here. Telemetry can be designed separately without weakening this PR's validated missing-data contract.
3. Optimized conversion coverage — implemented. The large-fixture path now asserts both the adapter-derived negative category and the indexed missing TMbed payload as __NA__, matching the small-data contract.
4. Format-scoped missing tokens — deferred by scope. Restricting sentinel normalization for producer-stamped bundles is a broader reader/import contract change affecting every annotation column and existing bundles. This OpenSpec change explicitly keeps the generic missing-token policy unchanged; that behavior needs its own spec and regression matrix rather than being folded into [BUG] Transmembrane prediction using Biocentral not working #339.
5. Mixed TMbed documentation — implemented. The generated long-form docs now say a mixed topology carries both alpha-helical and beta-barrel. The semicolon wire cell is a multi-value encoding, not a fourth legend category.
6. InterPro/Biocentral sequence precedence — implemented. InterPro now prefers UniProt's canonical sequence when present and falls back to FASTA for unresolved identifiers; Biocentral continues to prefer the submitted FASTA sequence. A focused manager regression pins both maps.
7. Unneeded FASTA materialization — implemented. annotate always streams identifiers, but parses/materializes sequences only when the resolved configuration requests InterPro or Biocentral. The CLI regression makes any parse_fasta call fail for a UniProt-only request.

Red/green evidence: the new source-precedence and sequence-free CLI tests first failed with InterPro receiving LOCAL and the UniProt-only path calling parse_fasta; after the implementation, the focused set passed 3/3. Fresh verification: bundle contract 15/15; ProtSpace non-slow suite 801 passed, 6 deselected; hosted prep 81 passed; exact CI Ruff check/format paths clean (144 ProtSpace files, 20 prep files); strict OpenSpec validation passed; generated annotation docs are current; and pnpm precommit passed in the commit hook.