Skip to content

Fix inverted span for minus-strand multi-exon GenBank locations - #8677

Open
Linxiushen wants to merge 1 commit into
huggingface:mainfrom
Linxiushen:fix-genbank-minus-strand-multiexon-span
Open

Linxiushen wants to merge 1 commit into
huggingface:mainfrom
Linxiushen:fix-genbank-minus-strand-multiexon-span

Conversation

@Linxiushen

Copy link
Copy Markdown

What was wrong

GenBank._parse_location_node computes the aggregate span of a join/order location from the positional first and last parts:

location["start"] = located[0]["start"]
location["end"] = located[-1]["end"]

This assumes the parts are listed in ascending genomic order. They are not for minus-strand multi-exon features: GenBank/NCBI (and Bio.SeqIO.write) list the exons in transcription order, which is descending genomic order, e.g. a two-exon minus-strand CDS is written as complement(join(121..180,11..60)). For such records the code takes the larger coordinate as start and the smaller as end, producing an inverted span (start > end, i.e. a negative length). The start_partial/end_partial flags are taken from the same wrong positional parts.

This contradicts the method's own documented contract (join(1..100,200..300) -> {"start": 1, "end": 300}) and its stated goal of matching Biopython's CompoundLocation, whose .start/.end are always the min/max regardless of part order.

Trigger

Load any GenBank file with a minus-strand multi-exon feature:

CDS   complement(join(121..180,11..60))

datasets.load_dataset("genbank", data_files=...) then yields a location dict with "start": 121, "end": 60 — an inverted span (start > end, negative length). By Biopython's CompoundLocation semantics the aggregate span is min-start / max-end (here start=11, end=180 in the parser's 1-based convention), so the inverted output is a bug rather than a convention difference. This complement(join(...)) form is what Bio.SeqIO.write(..., "genbank") emits for a naturally-constructed two-exon strand=-1 feature, so any Biopython round-trip or real NCBI record of this shape is affected.

Fix

Aggregate the span over all parts: start = min(part starts), end = max(part ends), and take each partiality flag from whichever part actually carries the min-start / max-end coordinate. This matches Biopython's CompoundLocation.start/.end semantics. For already-correct ascending (plus-strand) inputs the result is byte-identical, so this is a pure correctness fix with no behavior change for the common case. parts (segment order) is left untouched.

Red / green evidence

Added test_genbank_minus_strand_multi_exon_join_span_is_min_max.

  • Before the fix: the test fails with assert 121 == 11; end-to-end via load_dataset the row's location is {"parts": [[121, 180], [11, 60]], "start": 121, "end": 60} — an inverted span.
  • After the fix: the test passes and the location is {"parts": [[121, 180], [11, 60]], "start": 11, "end": 180}, matching Biopython. The full tests/packaged_modules/test_genbank.py suite passes (71 tests, no regressions). ruff check and ruff format --check are clean.

Run: python -m pytest tests/packaged_modules/test_genbank.py -q


Prepared with AI assistance; verified locally by the submitter.

_parse_location_node took the aggregate span of a join/order location from
the positional first/last parts (located[0].start, located[-1].end). That
assumes ascending genomic order, which does not hold for minus-strand
multi-exon features: GenBank/NCBI and Bio.SeqIO.write list exons in
transcription (descending genomic) order, e.g. complement(join(121..180,11..60)).
The parser then produced start=121, end=60 -- an inverted span. Aggregate
with min(start)/max(end) and take each partial flag from the part carrying
that coordinate, matching Biopython's CompoundLocation. Plus-strand
ascending inputs are byte-identical. Adds a regression test.
@lhoestq

lhoestq commented Sep 30, 2026

Copy link
Copy Markdown
Member

good catch ! cc @behroozazarkhalili for viz

related to #7951

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@behroozazarkhalili

Copy link
Copy Markdown
Contributor

Thanks for the cc. Confirmed: the new test fails on main (start=121, end=60) and passes with this change, and the rest of test_genbank.py stays green. Min-start/max-end is the right aggregate. LGTM.

@Linxiushen

Copy link
Copy Markdown
Author

I checked the existing hosted CI logs; I did not rerun the workflow. The Python 3.10 Ubuntu unit job reports 3510 passed and one collection error: Transformers 5.17.0 requires huggingface-hub<2.0, but the dependency-upgrade step installed 2.0.0. All 71 GenBank tests, including the new regression test, passed in that job.

The same dependency error appears in the same-day main Windows unit job and main Ubuntu integration job, with identical workflow and failing-test source. These are cross-matrix comparisons, not an exact-base rerun. GitHub annotations attribute the seven cancelled PR jobs to the failed unit matrix job; those cancellations still leave the full CI matrix incomplete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants