Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ref_allele_mismatch in liftover_expr for indel variants #273

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mkanai
Copy link
Contributor

@mkanai mkanai commented Nov 18, 2020

Hi, it seems the current ref_allele_mismatch doesn't work when the original reference allele has more than one bp. This PR fixes the issue.

Additionally, I found there are ref_allele_mismatch cases where ref/alt flips between the builds. For example, chr3:195513874:T:C (GRCh38) corresponds to 3:195240670:C:T (GRCh37).

It is particularly confusing since it looks like new_alleles == original_alleles but ref_allele_mismatch is true. Does it make sense to add a field e.g. ref_alt_flip? If so, I'd be happy to make a separate PR.

+---------------+------------+---------------+-------------+----------------+------------------+---------------------+---------------------+
| locus         | alleles    | new_locus     | new_alleles | original_locus | original_alleles | locus_fail_liftover | ref_allele_mismatch |
+---------------+------------+---------------+-------------+----------------+------------------+---------------------+---------------------+
| locus<GRCh37> | array<str> | locus<GRCh37> | array<str>  | locus<GRCh38>  | array<str>       |                bool |                bool |
+---------------+------------+---------------+-------------+----------------+------------------+---------------------+---------------------+
| 3:195236460   | ["T","C"]  | 3:195236460   | ["T","C"]   | chr3:195509651 | ["T","C"]        |               false |                true |
| 3:195236756   | ["G","C"]  | 3:195236756   | ["G","C"]   | chr3:195509947 | ["G","C"]        |               false |                true |
| 3:195237817   | ["C","A"]  | 3:195237817   | ["C","A"]   | chr3:195511010 | ["C","A"]        |               false |                true |
| 3:195240670   | ["T","C"]  | 3:195240670   | ["T","C"]   | chr3:195513874 | ["T","C"]        |               false |                true |
| 3:195242526   | ["T","C"]  | 3:195242526   | ["T","C"]   | chr3:195515729 | ["T","C"]        |               false |                true |
| 3:195244526   | ["T","C"]  | 3:195244526   | ["T","C"]   | chr3:195517728 | ["T","C"]        |               false |                true |
| 3:195245027   | ["G","A"]  | 3:195245027   | ["G","A"]   | chr3:195518229 | ["G","A"]        |               false |                true |
| 3:195245225   | ["G","A"]  | 3:195245225   | ["G","A"]   | chr3:195518427 | ["G","A"]        |               false |                true |
| 3:195246299   | ["G","A"]  | 3:195246299   | ["G","A"]   | chr3:195519502 | ["G","A"]        |               false |                true |
| 3:195251227   | ["G","C"]  | 3:195251227   | ["G","C"]   | chr3:195524429 | ["G","C"]        |               false |                true |
+---------------+------------+---------------+-------------+----------------+------------------+---------------------+---------------------+

@nawatts nawatts requested a review from ch-kr March 18, 2021 18:57
Copy link
Contributor

@ch-kr ch-kr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fully qualified to review this PR (I'm not familiar with indel liftover), but I think this could would have to left-align/normalize indels to be fully correct? (maybe @konradjk knows more or can tag someone else for review?)

@nawatts nawatts requested a review from konradjk March 18, 2021 19:31
@konradjk
Copy link
Collaborator

konradjk commented Mar 19, 2021

Sorry, I'm not sure I'm following. The examples you have are all SNPs, so I don't see how this affects indels. For the SNPs, yes, grabbing the ref context and checking for a flip is the ideal scenario, but note that it comes at the computational cost of the context lookup, so I'd advocate for a flag where you can turn it off. Edit to add: I see the lookup already exists, which I guess is fine

@konradjk
Copy link
Collaborator

Oh! I just realized this was two different suggestions (separated by "Additionally") - sorry about that! Yes, this seems sensible, though it could be worthwhile to have a documented example just so we can see what happens.

@jkgoodrich jkgoodrich marked this pull request as draft June 23, 2023 20:20
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.

3 participants