Repository navigation
fix(vep): translate with the transcript codon table (MT table 2) - #274
Conversation
|
@codex review |
|
@claude review |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f4b511c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn is_stop_codon_in(table: CodonTable, allele: &str) -> bool { | ||
| match table { | ||
| CodonTable::Standard => is_stop_codon(allele), | ||
| CodonTable::VertebrateMitochondrial => matches!( | ||
| allele.to_ascii_uppercase().as_str(), | ||
| "TAA" | "TAG" | "AGA" | "AGG" | "*" |
There was a problem hiding this comment.
Use the transcript table in fallback stop detection
When a mitochondrial transcript has no usable translation/CDS data, the code falls back to the allele-pattern checks in classify_coding and add_start_stop_heuristic_terms, but those still call the table-1-only is_stop_codon. Consequently, a positive-strand MT codon-sized substitution such as CGA>AGA misses stop_gained even though AGA is a stop in table 2, while TGA>TAA can be treated as stop-retained even though TGA encodes Trp. Route these fallback checks through the transcript's codon_table as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c684d8e. The allele-pattern stop checks in the no-CDS fallback, in classify_coding and add_start_stop_heuristic_terms, now use is_stop_codon_in(tx.codon_table, …).
New test: stop_heuristic_uses_the_transcript_codon_table. MT AGA>TAA at the stop codon gives stop_retained_variant under table 2 and not under table 1. It fails with the old calls.
Start checks in the fallback still compare the literal ATG, as VEP does. The commit also moves the bio-formats pins to #262's follow-up (397f15d). vepyr#206 is re-pinned. Its chrMT output is byte-identical to the previous head, and the regression test passes.
…7f15d Review follow-up on #274: classify_coding's allele-pattern stop check and add_start_stop_heuristic_terms, used when a transcript has no usable CDS, called the table-1 is_stop_codon, so on MT AGA was not a stop and TGA was. Route them through the transcript's codon table. Moves the bio-formats pins to #262's review follow-up (397f15d). Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@codex review |
|
@claude review |
|
Claude finished @mwiewior's task in 23s —— View job Review of PR #274: MT codon table 2The change looks correct and I found nothing blocking. I read the diff against What I checked
Minor, non-blocking
Tests and verification I have no changes to request. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c684d8ed75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| translate_codon_bytes(codon) | ||
| } | ||
|
|
||
| fn is_stop_codon_in(table: CodonTable, allele: &str) -> bool { |
There was a problem hiding this comment.
Route the CIL stop fallback through the transcript table
For VEP 116 terminal indels whose alternate peptide is absent or contains X, vep116_stop_retained_without_alt_peptide still checks the reconstructed terminal codon with the table-1-only is_stop_codon at line 4524 rather than this table-aware helper. On an MT transcript, for example, deleting the final base from a CDS ending AGA while an A shifts in from the 3′ UTR leaves AGA in the original stop slot, but this path returns false and omits stop_retained_variant; the inverse error occurs when the shifted codon is TGA. Pass tx.codon_table through this CIL fallback as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5c10a90. vep116_stop_retained_without_alt_peptide (the engine copy of _ins_del_stop_altered_cil) now checks the stop-slot codon with is_stop_codon_in(tx.codon_table, …).
New test: vep116_cil_stop_slot_uses_the_transcript_codon_table. An MT CDS ending AGA with a 3′ UTR A: deleting the last base keeps AGA in the slot, which is stop_retained under table 2 and not under table 1. It fails with the old call.
In the engine, is_stop_codon is now only called through is_stop_codon_in. vepyr#206 is re-pinned (75e86cf), and its chrMT output is unchanged.
Review follow-up on #274: vep116_stop_retained_without_alt_peptide (VEP's _ins_del_stop_altered_cil) checked the codon left in the original stop slot with the table-1 is_stop_codon. Use the transcript's table, as VEP's _codon_table does: on MT a CDS ending in AGA whose final base is deleted, with an A shifting in from the 3' UTR, keeps AGA in the slot and is stop_retained. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@codex review |
|
@claude review |
|
Claude finished @mwiewior's task in 26s —— View job Review of PR #274: MT codon table 2I found nothing blocking. I read the diff against Checked
Minor, non-blocking
Tests No changes requested. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
VEP translates a transcript with BaseTranscriptVariation::_codon_table, which is 2 (vertebrate mitochondrial) for every human MT transcript. The engine only knew the standard code, so on chrMT ATA read as Ile instead of Met, AGA/AGG as Arg instead of stop and TGA as stop instead of Trp, and Amino_acids, HGVSp, Consequence, IMPACT, SIFT and PolyPhen followed (294 of 3,134 ClinVar chrMT records, biodatageeks/vepyr#200). TranscriptFeature now carries a CodonTable, read from the cache's codon_table column; a null on a mitochondrial contig resolves to table 2, so caches converted before the bio-formats reader fix work unchanged. The table is used exactly where VEP uses it: - with the transcript's table: everything VEP derives from TranscriptVariationAllele::peptide() (Amino_acids, the consequence predicates, SNV and in-frame HGVSp peptides, the SIFT/PolyPhen key), the reference protein, and the codon at the old stop position (_ins_del_stop_altered); - table 1, as VEP does even on MT because BioPerl's translate() gets no table there: the alternate proteins behind frameshift, deletion and stop-loss HGVSp (_get_fs_peptides, _get_del_peptides, _stop_loss_extra_AA, _check_for_peptide_duplication); - literal ATG for start_retained, as VEP compares the string. Also moves the bio-formats pins to the reader fix (3fe9f8b), which reads codon_table from _variation_effect_feature_cache. Co-Authored-By: Claude Opus 5.5 <[email protected]>
VEP's _snp_start_altered / start_retained_variant compare the mutated start codon with the string "ATG" (VariationEffect.pm:1022). The engine checked whether the new first residue is Met, which is the same thing under the standard code but not under table 2, where ATA is Met too: MT:8529 G>A (ATP6 ATG>ATA) came out start_retained_variant where VEP writes synonymous_variant, p.Met1=. Check the codon itself, in the SNV and the insertion paths. Table-1 output cannot change. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…7f15d Review follow-up on #274: classify_coding's allele-pattern stop check and add_start_stop_heuristic_terms, used when a transcript has no usable CDS, called the table-1 is_stop_codon, so on MT AGA was not a stop and TGA was. Route them through the transcript's codon table. Moves the bio-formats pins to #262's review follow-up (397f15d). Co-Authored-By: Claude Opus 5.5 <[email protected]>
Review follow-up on #274: vep116_stop_retained_without_alt_peptide (VEP's _ins_del_stop_altered_cil) checked the codon left in the original stop slot with the table-1 is_stop_codon. Use the transcript's table, as VEP's _codon_table does: on MT a CDS ending in AGA whose final base is deleted, with an A shifting in from the 3' UTR, keeps AGA in the slot and is stop_retained. Co-Authored-By: Claude Opus 5.5 <[email protected]>
5c10a90 to
3f684d0
Compare
|
@codex review |
|
@claude review |
|
Claude finished @mwiewior's task in 18s —— View job Review of PR #274: MT codon table 2I found nothing blocking. I read the diff against What looks right
Minor, non-blocking
Tests No changes requested. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
datafusion-bio-formats#262 was squash-merged as abac8df, whose tree is identical to the PR head 397f15d these pins named. Moving all seven formats lines (and the lockfile) to the merge commit keeps no pin on a commit that exists only on a branch. No code change. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…rcent-encoding master pins formats abac8df for #274; this branch keeps the formats#266 head 64c4001, which already contains abac8df (#262) together with the #207 writer fix and the #263 Character INFO reader fix. All seven formats lines and Cargo.lock stay on 64c4001. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Move every datafusion-bio-format-* dependency (vep and pileup) from the pre-release rev to tag v1.16.0, which contains the DataFusion 55.2 bump (biodatageeks/datafusion-bio-formats#248) and the OpenDAL 0.59.5 fix for remote reads that retried forever under tokio 1.54 (#274). The lockfile keeps a single copy of datafusion-bio-format-core, datafusion, arrow and parquet, and drops the OpenDAL git revision. Also import Session and PhysicalPlanningContext in the ranges planners instead of spelling out their paths inline (review nit). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…332) * chore: bump DataFusion to 55.2.0 and bio-formats to its DF 55 branch Move the workspace from DataFusion 53.0.0 to 55.2.0 (arrow/parquet 59.3.2, rust-toolchain and CI on 1.94.0, DataFusion 55.2's MSRV) and pin every datafusion-bio-format-* dependency to biodatageeks/datafusion-bio-formats#248 (d1eb0c3), which carries the same bump rebased on formats v1.15.0, so it includes everything the previous e0379fb pin did. pileup and vep share that one rev so the graph holds one copy of datafusion-bio-format-core and DataFusion. Mechanical fallout of the 54/55 API changes: - `as_any` removed from DataFusion traits: dropped from every impl, the `.as_any()` hop removed at plan/expr downcast sites (including the multi-line chains in the interval planner), unused `Any` imports removed. fastqc's own `QcModule::as_any` is unrelated and kept. - `ExecutionPlan::apply_expressions` required: leaf implementations for the nodes without expressions; `IntervalJoinExec` visits its join keys and filter like DataFusion's own `HashJoinExec`. - `QueryPlanner` / `PhysicalPlanner` take `&dyn Session`, and `create_physical_expr` gains the `PhysicalPlanningContext` argument. - `create_hashes` takes DataFusion's `HashState`; the interval join uses `datafusion::common::hash_utils::RandomState` (ahash stays for the plain hash maps). - `Distribution::HashPartitioned` -> `KeyPartitioned`. - `MemoryPool` now requires `Display` and `name()`: TracingPool and the test-only RejectHashPool. - `MutableArrayData::extend` is deprecated in arrow 59.3: the SIFT page gather uses `try_extend` and propagates the overflow error. - `partition_statistics` returns `Arc<Statistics>` and is deprecated for callers; the interval join estimator and the plugin-cache join selector read statistics through `StatisticsContext`. That last item is the one behavioural change. Under DataFusion 55 the deprecated `partition_statistics` returns unknown statistics for DataFusion's own nodes, so `choose_for_hash_plan` would have reported MissingStatistics and silently forced every plugin-cache tier join onto SortMergeJoin; the adaptive-join tests cover it. Why: DataFusion 53's external sorter frees its merge headroom and then re-acquires it from an empty reservation, which starves under `FairSpillPool` when several partitions sort concurrently (#235). apache/datafusion#20642 fixes that and shipped in 54.0.0. Not touched: `research/lance_encoding_sandbox` is outside the workspace and still pins arrow 58 / datafusion-common 53. Supersedes #236 and dependabot #8. Co-Authored-By: Claude Opus 5.5 <[email protected]> * refactor: address review nits on the DataFusion 55.2 bump - interval join: say why `hash_utils::RandomState::default()` is safe (foldhash `FixedState` with a compile-time constant seed, shared by both join sides), and fold `partition_statistics_inner` back into `partition_statistics` where it lived before the bump, with a note on why the partition argument is ignored. Drop the stray blank line left by the `Any` import removal. - plugin-cache join selector: import `StatisticsContext` / `StatisticsArgs` instead of spelling the full paths inline. - the ten leaf `apply_expressions` impls (fastqc, pileup, ranges, vep): use imported `Arc` / `PhysicalExpr` / `Result` / `TreeNodeRecursion` instead of fully qualified paths. No behaviour change. Co-Authored-By: Claude Opus 5.5 <[email protected]> * chore: re-pin datafusion-bio-formats to 99d7f81 Follow biodatageeks/datafusion-bio-formats#248 to its review-nit commit (test renames, shorter `apply_expressions` signatures, CHANGELOG arrow version). No API change; vep and pileup stay on one formats rev. Co-Authored-By: Claude Opus 5.5 <[email protected]> * chore: pin datafusion-bio-formats to the v1.16.0 release Move every datafusion-bio-format-* dependency (vep and pileup) from the pre-release rev to tag v1.16.0, which contains the DataFusion 55.2 bump (biodatageeks/datafusion-bio-formats#248) and the OpenDAL 0.59.5 fix for remote reads that retried forever under tokio 1.54 (#274). The lockfile keeps a single copy of datafusion-bio-format-core, datafusion, arrow and parquet, and drops the OpenDAL git revision. Also import Session and PhysicalPlanningContext in the ranges planners instead of spelling out their paths inline (review nit). Co-Authored-By: Claude Opus 5.5 <[email protected]> --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
Engine side of biodatageeks/vepyr#200. Based on functions master after #273 (vepyr#199) merged as
4704ff9; this PR now targetsmaster. Reader fix: biodatageeks/datafusion-bio-formats#262, pinned here by rev. vepyr PR: biodatageeks/vepyr#206.Problem
VEP translates a transcript with
BaseTranscriptVariation::_codon_table, which is table 2 (vertebrate mitochondrial) for every human MT transcript. The engine only knew the standard code. On chrMT, ATA read as Ile instead of Met, AGA/AGG as Arg instead of stop, and TGA as stop instead of Trp.Amino_acids,HGVSp,Consequence,IMPACT,SIFTandPolyPhenfollowed: 294 of 3,134 ClinVar chrMT records differed.Where VEP uses the table (ensembl-variation
2fb834b= 116.0,db7394a= 116.2; the modules are identical)TranscriptVariationAllele::peptide()callstranslate(undef, undef, undef, $codon_table)(TVA.pm:692-788). That feedsAmino_acids, every peptide-based predicate, SNV/in-frame HGVSp peptides and the SIFT/PolyPhen key. Also with the table: the reference protein_peptide(the cached peptide, table 2) and the codon now at the old stop position in_ins_del_stop_altered/_cil(VariationEffect.pm:1472-1480, 1527-1535)._get_fs_peptides,_get_del_peptides,_stop_loss_extra_AAand_check_for_peptide_duplicationcall BioPerl'stranslate()with no table. VEP's own output confirms this: MT:10121p.Trp23LeufsTer44is the table-1 count (table 2 would give 10), and MT:14488fsTer14is too (table 2 would give 23)."ATG":start_retained_variant/_snp_start_alteredcompare the mutated codon with the string (VariationEffect.pm:1022), so table 2's extra start codons never matter.Change
CodonTable(Standard/VertebrateMitochondrial) lives onTranscriptFeature. It is resolved at load from the cache'scodon_tablecolumn. A null on a mitochondrial contig resolves to table 2, so caches converted before perf(vep): scale the LazyFrame run pool with workers (output budget, stateful run floor) #262 (all published ones) work unchanged. This matches VEP's GFF path, which assigns table 2 toMTby name.translate_codon_in,translate_codon_bytes_in,translate_protein_from_cds_in,is_stop_codon_in) delegate to the existing table-1 functions, which are unchanged.peptide(), the reference protein and_ins_del_stop_altered. The HGVSp alternate-protein translations stay on table 1, andtranslate_protein_from_cdsdocuments why.start_retainednow checks for a literalATG(mutated_cds_starts_with_atg) instead of "the new first residue is Met". The two are identical under table 1, but under table 2 ATA is Met too: MT:8529 G>A (ATP6 ATG>ATA) issynonymous_variantin VEP.is_mitochondrial_chrominannotate_provider.rsnow uses the sharedis_mitochondrial_contig.abac8df(91d6679). Its tree is identical to the PR head397f15dthey named before.Tests
Written first:
mt_ata_start_codon_is_met_under_table_2(MT:3307M/Lstart_lost; ATA>ATTM/I)mt_aga_is_a_stop_under_table_2(MT:3308M/*stop_gained&start_lost; an AGA terminal stop to AGC gives*/Sstop_lost)mt_tga_is_trp_under_table_2(MT:3669Wsynonymous)mt_atg_to_ata_start_is_synonymous_not_start_retained(MT:8529)vertebrate_mitochondrial_table_differs_from_standard_only_on_four_codons(all 64 codons)cached_transcript_codon_table_falls_back_to_table_2_on_mtWith the translators forced to table 1, the four table-2 classification tests fail, and the MT:8529 test failed before its fix.
cargo test -p datafusion-bio-function-vep(1,181 unit tests plus the integration tests),cargo clippy --all-targets --all-features -- -D warningsandcargo fmt --all -- --checkpass.Verification (vepyr built against the previous head)
The results below are pre-rebase evidence. On
3f684d0(the head before the re-pin toabac8df, with the same code), build/test CI passed and Claude's review found no blocking issue; formats#262 has merged, and the final vepyr gates will run once #274 merges and #206 is re-pinned.codon_tablefixture (12 records): empty diff against VEP 116.0 and VEP 116.2. This holds on the published cache (null column, fallback) and on chrMT rebuilt from the raw cache with perf(vep): scale the LazyFrame run pool with workers (output budget, stateful run floor) #262 (codon_table = 2on all 74 transcripts).delextTer?). The published and rebuilt caches give byte-identical output.🤖 Generated with Claude Code