Repository navigation
Remove all-gap alignment columns without repeated list deletion - #1
Closed
DylanCosto wants to merge 12 commits into
Closed
DylanCosto wants to merge 12 commits into
DylanCosto wants to merge 12 commits into
Conversation
Commit fd08da3 replaced `if (revcomp == True):` with `if revcomp:` in `SeqFile.__init__()`. Since any non-empty string is truthy, every string value of `revcomp` was then normalised to `"-5'"`, so with `"+5'"` or `"-3'"` `SeqFile.get()` silently returned the reverse complement of the wrong interval (`"+3'"` and `"maf"` were unaffected, as they map to `"-5'"` anyway). Check for `revcomp is True` instead, and add a test covering all the supported `revcomp` values. Also simplify `SeqFile.get()` to fetch the raw sequence only once, and add type annotations for `revcomp`. `SeqFile.get()` now only accepts `False`, `"-3'"` or `"-5'"` as the normalised scheme, so other falsy values (e.g. `0`, `None` or `""`) now raise `AssertionError` instead of being treated as no reverse complement. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and `get_indexed()`
`get_reader()` and `get_indexed()` passed `species_to_lengths` as a
positional argument, but in the reader/indexed-access classes it isn't the
next positional parameter:
- `get_reader("maf", ...)` failed because `maf.Reader.__init__()` only
accepts keyword arguments after `file`:
```
TypeError: Reader.__init__() takes 2 positional arguments but 3 were given
```
- `get_reader("axt", ...)` passed it as `species1`, so reading a block
failed with:
```
TypeError: unsupported operand type(s) for +: 'dict' and 'str'
```
- `get_indexed()` passed it as `use_cache` for MAF and as `species1` for
AXT, so it was never used as the species-to-lengths mapping.
Pass it as a keyword argument instead, and add tests.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For an empty component, `remove_all_gap_columns()` appended `None` to `seqs`, then also tried `list(c.text)`, which raised `TypeError` (`text` is `None`) and appended a second `None`. `seqs` therefore got out of step with `self.components`: gap columns were checked against, and the new texts were assigned from, the wrong sequences, so all-gap columns could be left in the components after an empty one. E.g. for components with texts `"A-C"`, `None` (empty) and `"G-T"`, the result was `"AC"`, `None` and `"G-T"` instead of `"GT"` for the last one. Append exactly one entry to `seqs` per component, and add a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`read_lengths_file()` only closed the file after reading it successfully, so a malformed line, which raises `ValueError`, left the file handle open. Use a `with` statement instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`Alignment.__deepcopy__()` called `deepcopy()` on its attributes and components without passing `memo` on, so each call started a new memo and objects shared within an outer deep copy were duplicated. E.g. after `deepcopy([alignment1, alignment2])`, where both alignments share the same `species_to_lengths` dict, each copy got its own copy of the dict. Pass `memo` on, as the `copy` module documentation requires, and add a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Annotate all of `lib/bx/align/core.py`, adding the `SpeciesToLengths` and `AlignmentFormat` type aliases, and annotate the functions in other modules that it relies on or that use it: `read_lengths_file()`, `Reader.open_seqs()` in `lav.py`, `Writer.write()` in `axt.py` and `maf.py`, and `readline()` in `maf.py`. Small runtime changes needed to make the code type-check, or found while annotating it: - `Alignment.slice_by_component()` raises `ValueError` when no component has the given `src`, instead of `AttributeError` on `None`. - `Alignment.column_iter()` and `Component.coord_to_col()` raise `ValueError` for a non-empty component without text, instead of `TypeError`. - `Component.get_src_size()` raises its usual "component has no src_size" exception when the alignment it belongs to has been garbage-collected, instead of `AttributeError` on `None`. - `maf.Writer.write()` asserts that `strand`, `synteny_empty` (for empty components) and `text` are set, instead of failing with `TypeError` in `format_tabular()`. - `get_indexed()` uses `maf.MAFIndexedAccess` instead of its deprecated `maf.Indexed` alias. - `shuffle_columns()` reads `text` once per component instead of once per column. - The error raised by `lav.Reader.open_seqs()` when it can't open the seq2 file names `seq2_filename` instead of `seq1_filename`. Also: - Add `__all__` to `bx.align.core`. As a result `from bx.align import *` (and `bx.align`, which star-imports `bx.align.core`) no longer exposes `random`, `weakref`, `read_lengths_file` or `coord_to_col`, nor the new typing imports. `coord_to_col` is still available from `bx.align.core`. - Drop the `__ne__()` methods of `Alignment` and `Component`, which were equivalent to the default implementation. - Build `species_to_lengths` directly in `lav.Reader.open_seqs()`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
which reached end of life on 2026-10-01.
NumPy 2.5.0 removed support for PyPy3.12, can be restored when numpy/numpy#32332 is merged.
Resolve the conflict in `Alignment.remove_all_gap_columns()`, where upstream/main had fixed the misalignment of the sequences after an empty component (commit 3ba2cc9) and added type annotations (commit 7a02fcd), by keeping the new single-pass algorithm from this branch, which already includes that fix, adapted to type-check: - Collect `(component, text)` pairs instead of only the components. With `Component.text` annotated as `str | None`, mypy only narrows `c.text` to `str` inside the comprehension that filters out `None`, so using `c.text` later failed with: ``` lib/bx/align/core.py:225: error: Unsupported right operand type for in lib/bx/align/core.py:229: error: Value of type "str | None" is not indexable lib/bx/align/core.py:233: error: Value of type "str | None" is not indexable ``` Storing the text in the pair keeps it typed as `str`, without needing `cast()` or `# type: ignore`. - Deduplicate the components by `id()` in a single dict comprehension and keep only its values as a list, since the keys are not needed afterwards. Checked against the implementation on upstream/main with 3000 random alignments, including empty components at the start, middle and end and components repeated in `components`: the results are identical. For 80,000 columns of which 90% are all-gap, `remove_all_gap_columns()` now takes about 0.02 s instead of 3 s. With 100 components, its peak extra memory use (measured with `tracemalloc`) drops from 61 MiB to 1.1 MiB, since it no longer converts each component's text into a list of characters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
Submitted upstream as bxlab#124. |
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
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.
Alignment.remove_all_gap_columns()deleted each all-gap position from every sequence list, repeatedly shifting the remaining alignment. Select the retained columns once and rebuild each affected component once. Return immediately when a gap-free component proves that every column must remain, and avoid rebuilding strings when the scan finds no removable columns.Repeated references to the same component are rebuilt only once, preserving shared-object behavior. Empty MAF rows are excluded directly, which also prevents sequence text from being assigned to the wrong component when an empty row occurs before sequence rows.
Before the shared-component correction, local synthetic benchmarks used eight components and five alternating upstream/patched runs, with identical output and metadata:
Initial validation: native package build, all 171 existing/parameterized tests, and Ruff/Black checks for the changed files passed. The existing MAF test now places empty rows first, in the middle and last; the first two placements fail upstream and pass after this change. Separate deterministic checks covered 2,006 oracle cases, including 1,000 direct upstream comparisons, empty/all-gap alignments and metadata preservation.
A follow-up review reproduced an
IndexErrorwhen the same component appeared twice and needed gap removal. The existing MAF test now covers that case; three parameter combinations failed before the correction. Afterward, the full suite passed all 174 tests, Ruff/Black passed, and 341 exhaustive small alignments plus alias controls matched upstream. A representative allocation check changed the traced peak by only 56 bytes; the earlier timing benchmark was not rerun.