Conversation
Add a team assignee list to the Dependabot config so update PRs are assigned automatically and aren't missed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EjoDJ7ErJ2pjyYSqcsQ3Su
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EjoDJ7ErJ2pjyYSqcsQ3Su
chore(dependabot): add PR assignees
Correct assignee from BrianNejati to briannejati to match the actual GitHub login so Dependabot can assign PRs correctly.
Bumps [actions/checkout](https://github.com/actions/checkout) from 5.0.1 to 5.1.0. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@93cb6ef...fbc6f39) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 5.1.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish) from 1.14.0 to 1.14.1. - [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases) - [Commits](pypa/gh-action-pypi-publish@cef2210...ba38be9) --- updated-dependencies: - dependency-name: pypa/gh-action-pypi-publish dependency-version: 1.14.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
* fix(genept): make Ensembl -> gene symbol mapping reachable
GenePT's embedding table is keyed on gene **symbols** (`get_text_embeddings`
looks up `self.embeddings.get(emb.upper())` over `var_names`), so Ensembl IDs
must be mapped *to* symbols -- the opposite direction from the Ensembl-keyed
models. The guard read `if gene_names == "ensembl_id":`, copied from Geneformer
where the correct comparison is `!=`. That made the mapping unreachable on every
input where it would have been correct:
- `gene_names="ensembl_id"` with real Ensembl IDs raised an error telling the
caller to set the flag they had just set;
- `gene_names="ensembl_id"` with non-ENS values "mapped Ensembl -> symbols" over
data that was not Ensembl;
- `gene_names="index"` (the default, and the only value bio-agent can reach,
since `FineTuning.predict` calls `process_data(data)` positionally) skipped
mapping entirely and looked Ensembl IDs up in a symbol-keyed table.
Detection is now per entry against an anchored Ensembl *gene* ID pattern instead
of `.startswith("ENS").all()`: the latter also matches real symbols (ENSA) and
transcript/protein IDs (ENST.., ENSP..), and `.all()` skips a var index that is
only mostly Ensembl IDs. Entries that are already symbols are preserved, so a
mixed index does not lose them; genes with no symbol are dropped with a logged
count, and an all-unmappable index raises.
The docstring was also copied from Geneformer and described the opposite model
("GenePT uses Ensembl IDs to identify genes"); rewritten to match reality.
Adds ci/tests/test_genept/, which did not exist -- the absence of any GenePT test
directory is why this survived. Both mutants (restoring the unreachable guard;
remapping wholesale instead of per entry) turn the new suite red.
Refs helicalAI/bio-agent#1117, helicalAI/bio-agent#1121
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(genept): strip Ensembl version suffixes and collapse duplicate symbols
Two blocking findings from the committee review of the parent change, both
reproduced locally before fixing.
**Versioned IDs were silently dropped.** `_ENSEMBL_GENE_ID_PATTERN` accepts a
version suffix, but the matched identifiers were passed unstripped to a mapping
table keyed on bare `ENSG…`, so `ENSG00000141510.17` resolved to None and was
dropped as "no symbol". Verified: TP53 and ACTB vanished from a var index of
`[ENSG00000141510.17, ENSG00000075624.9, ENSG00000111640]`, leaving only the
unversioned GAPDH. Versioned IDs are the GENCODE/CellRanger default, so this
deleted real genes from very common inputs. The suffix is now stripped before
lookup, and only for entries matching the Ensembl pattern -- real gene symbols
contain dots too (AC000068.10).
**Duplicate symbols crashed process_data.** Ensembl -> symbol is many-to-one:
10616 of the 48698 symbol-bearing rows in `hsapiens_pybiomart.csv` share a
gene_name (3369 symbols carried by >= 2 ids). Assigning the mapped symbols
straight to `var_names` produced a non-unique index, and this function's own
`adata[:, genes_names]` subset then raised
`InvalidIndexError: Reindexing only valid with uniquely valued Index objects`.
Verified with ENSG00000274144 + ENSG00000105618 (both PRPF31). Colliding symbols
are now collapsed to the copy carrying the most counts, so an all-zero
alt-scaffold copy cannot displace the expressed one, with the accounting logged.
This crash was inside the parent change's blast radius rather than pre-existing:
before the guard was corrected, the default `gene_names="index"` path never ran
the mapping at all, so duplicates were unreachable for exactly the inputs the fix
exists to serve.
Also drops the `adata.copy()` (the helper only reads one var column, so
`convert_list_ensembl_ids_to_gene_symbols` is called directly -- measured 13.9 MB
vs 62.1 MB peak on a 4000x3000 float32 AnnData), which additionally removes the
coupling to the helper's hardcoded "gene_names" output column.
Two shipped tests were passing for the wrong reason and are fixed:
`test_version_suffixed_ids_are_mapped` asserted only that no ENSG prefixes
remained -- which dropping satisfies as well as mapping -- and
`test_gene_names_column_is_honoured` asserted nothing at all, since the fixture's
index holds no ENSG values either way. Both now name the expected symbols; the
committee's mutations against them (no version strip; mapping ignores non-index
columns; positional dedupe) each fail exactly one test.
Refs helicalAI/bio-agent#1117, helicalAI/bio-agent#1121
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…/gh-action-pypi-publish-1.14.1 Bump pypa/gh-action-pypi-publish from 1.14.0 to 1.14.1
…ons/checkout-5.1.0 Bump actions/checkout from 5.0.1 to 5.1.0
…data (#418) * feat: handle Ensembl/symbol gene identifiers in each model's process_data Reconcile gene identifiers at each model's `process_data` -- the single choke point shared by embed, fit, evaluate, eval and run_isp -- rather than in a downstream consumer, which reached only two of those and needed a hand-maintained model->namespace table kept in step by hand. One shared set of primitives in `helical/utils/mapping.py`, replacing five inlined copies of `startswith("ENS")`. That expression had three live bugs, each now covered by a test: - it matches real gene symbols (`ENSA`) and transcript/protein IDs (`ENST..`, `ENSP..`), so anchored `^ENS[A-Z]{0,4}G\d{11}(\.\d+)?$` is used instead; - `.all()`/`.any()` over the column cannot express a var index that is only *mostly* Ensembl IDs, so detection is per entry; - version suffixes were never stripped, and the mapping tables are keyed on bare IDs, so every `ENSG..\.17` -- the GENCODE/CellRanger default -- resolved to None and was dropped as "unmapped". Per model, by what its vocabulary is keyed on: - **scGPT, UCE, GenePT** (symbols): `ensure_gene_symbols` translates Ensembl IDs, leaves existing symbols untouched, and collapses symbols claimed by more than one gene -- 10616 of the 48698 symbol-bearing rows in the bundled table share a gene_name, and a non-unique var index desyncs scGPT's count_matrix from its gene_ids (#377) and raises InvalidIndexError in GenePT. Of a colliding set the copy carrying the most counts wins; choosing positionally lets an all-zero alt-scaffold copy displace the expressed one, after which the gene reads as unexpressed with no error anywhere. - **Geneformer, Tahoe** (Ensembl): `ensure_ensembl_ids` takes Ensembl input **directly** instead of raising. Deliberately no symbol round trip: their vocabularies contain genes with no gene symbol at all, so a round trip would drop them. ENSG00000159239 -- in Geneformer's vocabulary, blank symbol in the table -- is now tokenized rather than lost. - **Nicheformer, Transcriptformer**: already branched on the identifier system correctly; only their detector is swapped. Two protections the removed guards provided by accident are restored explicitly, and better, so the existing `test_ensembl_data_is_caught` passes **unmodified**: - `require_vocabulary_overlap` rejects well-formed identifiers that are simply from another annotation (mouse IDs against a human vocabulary) -- membership rather than shape, so usable Ensembl input is accepted while unusable input still fails loudly instead of tokenizing to nothing. - `reject_null_identifiers` refuses literal "None"/"nan"/empty placeholders, which mean an earlier mapping already failed. Matched exactly rather than by prefix, so real genes like NANOS1 and NAT1 are unaffected. Verified against real cached weights on CPU: an Ensembl-indexed AnnData that previously failed with "No matching genes found between input data and scGPT gene vocabulary" now tokenizes, for a plain Ensembl index, a versioned one, a mixed one and a colliding one; Geneformer keeps the symbol-less in-vocabulary gene; mouse IDs are rejected. 40 new tests for the primitives. Suite: 250 passed, 7 skipped, 1 pre-existing failure (transcriptformer gene mode, "Torch not compiled with CUDA enabled", identical on the base commit); test_tahoe/helix_mrna/mamba2_mrna cannot be collected on this host (flash_attn / mamba-ssm absent). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tahoe): update the two tests that encoded the pre-change contract CI caught these; they cannot run on a host without flash_attn, so the Tahoe path was flagged as read-only-verified in the PR. Both tests asserted the behaviour this change deliberately replaces. - `test_process_data_raises_on_no_mapped_genes` patched `helical.models.tahoe.model.map_gene_symbols_to_ensembl_ids`, which no longer exists, to force an all-None ensembl_id column. Its *intent* -- nothing maps, so raise -- still holds, and UNKNOWN1/UNKNOWN2 are genuinely unmappable, so it now asserts that directly with no mock of an internal. That also stops it breaking the next time the helper changes. - `test_gene_mapping_ensembl_warning` asserted `match="ensemble ids"`: a column of Ensembl IDs with gene_names != "ensembl_id" was refused outright. That is exactly what this change does -- Tahoe's vocabulary *is* Ensembl-keyed, so those identifiers are now used as they are, with no symbol round trip. Rewritten to assert the new contract and renamed accordingly. It asserts on the reconciliation rather than the whole pipeline because the old test never reached the rest of `process_data` either (it always raised first) and the hand-built config in the fixture has none of what tokenization needs -- driving it further only produced an unrelated `KeyError: 'max_length'`. Unlike Geneformer's `test_ensembl_data_is_caught`, which passes unmodified because the protections it really guarded (vocabulary overlap, null sentinels) were restored explicitly, this one asserted the guard's *message* rather than a behaviour worth keeping, so re-baselining it is the honest fix. Both asserted behaviours were verified directly against `ensure_ensembl_ids` on the same data shape; the tests themselves can only be executed by CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(mapping): hoist the numpy import to module scope Review feedback on #418 (oriolpetithelical, mapping.py:275): no reason for it to be function-scoped. numpy is a hard dependency (`numpy>=2.1.3,<2.3` in pyproject) and is imported at module scope across the rest of helical, so the local import bought nothing -- it was an artefact of where the helper was first written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: bump helical minor version to 3.1.0 New capability (every model's process_data accepts either gene-identifier system) plus new public helpers in helical/utils/mapping, so MINOR rather than PATCH. 3.0.4 was claimed by #417's squash merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(mapping): drop the model argument, which only fed log strings Review feedback on #418 (oriolpetithelical, mapping.py:324). It was threaded through five functions purely for string interpolation, and it is redundant even for that: every caller's `process_data` logs "Processing data for <model>." immediately before calling these helpers, so the model already sits directly above the accounting line in the log stream, and an exception's traceback names the calling module. Messages are now model-agnostic. Removed from `ensure_gene_symbols`, `ensure_ensembl_ids`, `reject_null_identifiers`, `require_vocabulary_overlap` and `_log_accounting`; six call sites and two tests updated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(mapping): document that ensure_gene_symbols also rewrites var_names Review feedback on #418 (oriolpetithelical, mapping.py:369): surprising that the index is modified even when the gene names live in a column. It is deliberate, and the docstring now says why: symbol-keyed models do not agree on which identifiers they read. scGPT reads `var[gene_names]`, but GenePT looks its embeddings up on `var_names` (`get_text_embeddings`) regardless of what `gene_names` was -- so leaving the index alone would silently match nothing there. Normalising both is what makes one result usable by any of them. The docstring now also states the rest of what the function writes: the named column is kept in step (callers run `ensure_rna_data_validity` first, which materialises a `var["index"]` from the pre-conversion index, and a lookup reading that stale column would match nothing), the pre-conversion identifiers are kept in `var["original_gene_id"]`, genes can be dropped so `n_vars` shrinks with `X` subset alongside, and the input is returned uncopied only when nothing needs converting. Every claim was verified by execution before being written down, and the two that were unpinned now have tests -- reverting the column sync turns both red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(mapping): state why ensure_ensembl_ids and ensure_gene_symbols differ Review feedback on #418 (oriolpetithelical): "I see an asymmetry between ensure_ensembl_ids versus ensure_gene_symbols. The one is always putting the outputs in a column (even though the values may be coming from an index) while the other one is putting the outputs in the index and optionally in the column." Correct observation; the asymmetry is deliberate, and the code said nothing about it. Each helper performs the **minimum mutation its consumers require**, and the consumers themselves are asymmetric: - Ensembl-keyed models read the column -- Geneformer's tokenizer reads `data.var.ensembl_id`, Tahoe reads `var[gene_id_key]` -- and neither uses `var_names` to identify a gene, so writing the column suffices. - Symbol-keyed models disagree with each other and between them cover both surfaces: GenePT and UCE read `var_names`, scGPT reads `var[gene_names]`. So `ensure_gene_symbols` has to normalise both. Leaving `var_names` alone on the Ensembl side is load-bearing rather than incidental: it keeps the caller's identifiers addressable for `id_to_gene` reverse lookups and caller-supplied gene lists, and rewriting the index would silently change the identifier system of anything reported downstream -- an ISP run would come back keyed on Ensembl IDs even when the caller supplied symbols. Documented on both functions so the pairing is discoverable from either. Every claim re-verified by execution: the column/index write behaviour, and each of the five consumer read-sites cited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: drop cross-repo issue references from comments and tests The helical repo should not carry references to another repository's tracker: they are unresolvable for anyone reading this code, and the bare numbers would render as links to unrelated helical issues. The reasoning each reference accompanied is kept in full -- only the identifiers are removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
dmiv-helical
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.