Repository navigation
Make remove_all_gap_columns linear and fix empty-row misalignment - #124
Open
DylanCosto wants to merge 3 commits into
Open
DylanCosto wants to merge 3 commits into
DylanCosto wants to merge 3 commits into
Conversation
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>
Collaborator
|
Thanks a lot for the contribution! I've just added type annotations for this code to the main branch to better understand this old code, and pushed here a merge commit, hope that's OK and is still correct? |
nsoranzo
reviewed
Oct 15, 2026
| complex_maf_gap.components.pop() | ||
| # Return the empty row to its fixture position for the equality check. | ||
| complex_maf_gap.components.append(complex_maf_gap.components.pop(empty_index)) | ||
| assert complex_maf_gap == complex_maf |
Collaborator
There was a problem hiding this comment.
Suggested change
| assert complex_maf_gap == complex_maf | |
| assert complex_maf_gap == complex_maf | |
| assert complex_maf_gap.text_size == complex_maf.text_size |
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
2 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.
Alignment.remove_all_gap_columns()deletes each all-gap column from every sequence list one at a time, which shifts the rest of each list on every deletion. On long alignments with many gap columns this becomes very slow (about 5 s for 80,000 columns where 90% are all-gap).This works out which columns to keep in one pass and rebuilds each component's text once. That example drops to about 0.03 s. It also returns early when some component has no gaps at all, since then no column can be all-gap.
It also fixes a bug: for an empty component, the old code added two entries to its list of sequences instead of one, so every later row was misaligned. An empty MAF row before the sequence rows could get another row's text. If the same component object appears twice in
components, it is now rebuilt only once.test_remove_all_gap_columnnow places the empty row first, in the middle and last, and also covers a repeated component. The first two placements fail onmain.