Add Needleman-Wunsch distance as a faster alignment metric - #725
Add Needleman-Wunsch distance as a faster alignment metric#725felixpetschko wants to merge 22 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #725 +/- ##
==========================================
- Coverage 78.29% 77.15% -1.14%
==========================================
Files 51 51
Lines 4607 4763 +156
==========================================
+ Hits 3607 3675 +68
- Misses 1000 1088 +88
🚀 New features to boost your workflow:
|
|
Hi @felixpetschko, thanks for working on this! From a technical perspective this looks all great. My main concern here is that we keep adding metrics without providing guidelines (and evidence for) which metric to use.
In what way is TCRdist specific to TCR sequences that would prevent it from using it for BCR? Or rather in what way is the alignment distance superior for BCR? My understanding would be that the main difference is that TCRdist allows only for a single gap position, while alignment allows for multiple, but does this make a big difference in practice? Also how does it compare to TCRdist in terms of speed? E.g. how long would TCRdist take on the omniscope dataset on the same hardware? |
|
Hi @grst
My intention was rather to provide a faster implementation of the existing
Actually, I was mainly focusing on performance, and I do not have proof for which metric is better in which case. My reasoning was that TCRdist's approach, with trimming from the N and C terminus and a single gap region, seems more targeted towards the TCR model. Needleman-Wunsch might be easier to justify for BCR CDR3 comparisons because it does not impose TCRdist's trimming and single-gap-region assumptions, and can handle length differences with a general global alignment.
TCRdist can run the full Omniscope COVID dataset on the same hardware in around 2.3 hours with default parameters, which makes it around 5 times faster than Needleman-Wunsch in my test run. The main reason is that, with Scirpy's default parameters, the gap position is computed by a formula and it is not necessary to try different gap positions. In contrast, Needleman-Wunsch computes the optimal global alignment that minimizes the distance. |
|
Do you think we could implement this without any user-facing changes? (would be interesting to know if in the history of scirpy anyone has ever changed these default parameters. I'd guess not). |
Yes, I will do that!
However, I think we should definitely change the default params and set them in a way such that gaps are allowed. Otherwise there is no alignment done at all. I would allow at least 2 gaps such that it's worth to even run the dynamic programming alignment algorithm. |
|
Just changing defaults is also not very good practice... so we'd at least have to warn about it. We could complement it with a "metrics" guide in the documentation that explains the pros/cons and usecases of the different metrics. |
|
Alright, then let's do it like that 👍 |
558951f to
ec86afa
Compare
|
Now I marked |
grst
left a comment
There was a problem hiding this comment.
I have a few more comments, mostly on improving the organization of the metrics submodule.
| results_iter = joblib.Parallel(return_as="generator")(delayed_jobs) | ||
| results_iter = tqdm(results_iter, total=len(delayed_jobs), desc="Computing distance blocks") | ||
| results = list(results_iter) | ||
|
|
There was a problem hiding this comment.
This would fail on joblib backends that do not support return_as="generator", e.g. dask.
See scirpy.utils._parallelize_with_joblib for a helper that addresses this.
| _metric_mat = _gpu_hamming_mat | ||
|
|
||
|
|
||
| PARASAIL_AA_ALPHABET = "ARNDCQEGHILKMFPSTWYVBZX" |
There was a problem hiding this comment.
I'd move all these definitions to the top of the file, or maybe even better, a separate submodule within the ir_dist package.
There was a problem hiding this comment.
Also, maybe worth renaming this simply to AA_ALPHABET? Or is this still specific to parasail in any way?
There was a problem hiding this comment.
... or maybe make them dataclasses, that match together alphabet and substitution matrix
@dataclass
class SubstitutionMatrix:
alphabet: str
matrix: np.ndarray
BLOSUM62 = SubstitutionMatrix(alphabet = "ARN...", matrix = np.array([...]))
TCRBLOSUM_ALPHA = SubstitutionMatrix(...)| parasail_aa_alphabet = PARASAIL_AA_ALPHABET | ||
| parasail_aa_alphabet_with_unknown = PARASAIL_AA_ALPHABET_WITH_UNKNOWN | ||
| matrix_alphabet = CANONICAL_AA_ALPHABET | ||
| blosum62_substitution_matrix = BLOSUM62_SUBSTITUTION_MATRIX | ||
| tcrblosum_alpha_substitution_matrix = TCRBLOSUM_ALPHA_SUBSTITUTION_MATRIX | ||
| tcrblosum_beta_substitution_matrix = TCRBLOSUM_BETA_SUBSTITUTION_MATRIX |
There was a problem hiding this comment.
Is there any reason for defining these as class variables instead of directly referencing the constants?
| parasail_aa_alphabet = PARASAIL_AA_ALPHABET | ||
| parasail_aa_alphabet_with_unknown = PARASAIL_AA_ALPHABET_WITH_UNKNOWN | ||
| tcrblosum_matrix_alphabet = CANONICAL_AA_ALPHABET | ||
| blosum62_substitution_matrix = BLOSUM62_SUBSTITUTION_MATRIX | ||
| blosum62_with_ambiguous_substitution_matrix = BLOSUM62_WITH_AMBIGUOUS_SUBSTITUTION_MATRIX | ||
| tcrblosum_alpha_substitution_matrix = TCRBLOSUM_ALPHA_SUBSTITUTION_MATRIX | ||
| tcrblosum_beta_substitution_matrix = TCRBLOSUM_BETA_SUBSTITUTION_MATRIX |
There was a problem hiding this comment.
Again, wouldn't it be easier to read if the constants were directly used everywhere?
| def _make_numba_substitution_matrix(self, substitution_matrix: np.ndarray, matrix_alphabet: str) -> np.ndarray: | ||
| score_matrix = np.zeros( | ||
| (len(self.parasail_aa_alphabet_with_unknown), len(self.parasail_aa_alphabet_with_unknown)), | ||
| dtype=np.int32, | ||
| ) | ||
| if substitution_matrix.shape != (len(matrix_alphabet), len(matrix_alphabet)): | ||
| raise ValueError("`substitution_matrix` must be square and match `matrix_alphabet`.") | ||
| for i, aa1 in enumerate(matrix_alphabet): | ||
| for j, aa2 in enumerate(matrix_alphabet): | ||
| score_matrix[self.parasail_aa_alphabet.index(aa1), self.parasail_aa_alphabet.index(aa2)] = ( | ||
| substitution_matrix[i, j] | ||
| ) | ||
| return score_matrix |
There was a problem hiding this comment.
Could this become a metric-agnostic helper function? I think we already have similar code in other metrics...
| """\ | ||
| FastAlignmentDistanceCalculator achieves (depending on the settings) identical results | ||
| at a higher speed. | ||
| If `gap_open == gap_extend`, use NeedlemanWunschDistanceCalculator instead. |
There was a problem hiding this comment.
| If `gap_open == gap_extend`, use NeedlemanWunschDistanceCalculator instead. | |
| If `gap_open == gap_extend` (the default), use NeedlemanWunschDistanceCalculator instead, which provides identical results while being much faster. If you actually have a use-case for affine gap penalties, please let us know by opening an issue on GitHub. |
|
|
||
| @deprecated( | ||
| """\ | ||
| If `gap_open == gap_extend`, use NeedlemanWunschDistanceCalculator instead. |
There was a problem hiding this comment.
| If `gap_open == gap_extend`, use NeedlemanWunschDistanceCalculator instead. | |
| If `gap_open == gap_extend` (the default), use NeedlemanWunschDistanceCalculator instead, which provides identical results while being much faster. If you actually have a use-case for affine gap penalties, please let us know by opening an issue on GitHub. |
|
|
||
|
|
||
| def test_needleman_wunsch_reference(): | ||
| # test needleman-wunsch against a precomputed linear-gap alignment reference |
There was a problem hiding this comment.
How has this been derived? Parasail?
|
regarding deprecations, take a look at #735 please that switches to the decorators provided by scverse-misc. |
Summary
This PR adds a Numba-optimized Needleman-Wunsch sequence distance metric via
metric="needleman_wunsch". Conceptually, this metric is very similar to the existing alignment metric, with two main differences: it is much faster for large datasets, and it uses a linear gap model, where every gap position receives the same penalty.The existing alignment metric allows gap openings and gap extensions to be penalized differently. However, this distinction is not used with the current default parameters anyway, where
gap_open == gap_extend. If the alignment metric is configured such thatgap_open == gap_extend == gap_penalty, both metrics should return equal results.The reason for using a linear gap model is mostly practical. Supporting separate gap-open and gap-extension penalties requires handling three dynamic programming matrices instead of one for each sequence pair, which adds substantial computational overhead. For CDR3 sequences, which are usually quite short, I would generally expect users to choose parameters that allow only a limited number of gap positions. For example, in my test runs I used
cutoff=10andgap_penalty=4, which allows up to two gap positions. In this setting, distinguishing between two separate single-position gaps and one adjacent two-position gap is probably not worth the additional runtime. Therefore, I do not think the restriction to a single gap penalty is a major limitation.The problem with the current alignment metric is that it is too slow for large datasets. The
fastalignmentmetric improves runtime, but it can also suffer from performance limitations and does not always return exact results. Besides that, the current default parameters do not allow gaps (cutoff=10,gap_open=gap_extend=11). In that default setting, only equal-length sequences can fall below the cutoff, so the metric effectively computes alignment distances between equal-length sequences without making use of meaningful gap placement.Therefore, I implemented the Needleman-Wunsch distance in a similar style to the Numba-optimized TCRdist CPU implementation. It supports BLOSUM62 by default and can also use TCRBLOSUM alpha/beta matrices through
base_matrix="tcrblosum". With the improved performance, I was able to run the 8 million-cell Omniscope COVID dataset with 64 CPU cores usingcutoff=10andgap_penalty=4within around 12 hours.Overall, I think this metric is a useful addition because it provides an exact alignment-based distance that can still handle larger datasets. It is more flexible than the Hamming distance, but substantially faster than the existing alignment metric. In contrast to TCRdist, it is also not specific to TCR CDR3 sequences and can therefore be used for BCR analyses as well.
The most useful default values for
gap_penaltyandcutoffare open for discussion, since the performance improvements make less restrictive parameter choices feasible. It could also be discussed whether the existing alignment metric is still needed if its additional gap parameterization and support for additional Parasail substitution matrices are not required.Main changes
NeedlemanWunschDistanceCalculatorand expose it viametric="needleman_wunsch"insequence_distandir_dist.