Remove redundant symbolic alt to so term hash #1075
Conversation
nakib103
requested review from
nuno-agostinho
and removed request for
nuno-agostinho
There was a problem hiding this comment.
Hi @nakib103, thanks for your PR! I put some concerns regarding your changes in the following comments.
Please tell me if you want to discuss how to best approach my suggestions. Thanks!
|
@nuno-agostinho, |
There was a problem hiding this comment.
Thanks for your PR, @nakib103! It's working as intended when testing StructuralVariantOverlap with same_type=1.
I am going to approve it, but I am concerned about moving some functions to ensembl-vep, as it makes ensembl-variation require VEP to properly test and may break other repos (web? REST?) that did not consider this requirement before.
Please check with the team if this if fine. If so, the PR is ready to be merged (after you check my small suggestions).
|
|
||
| $abbrev = "DUP:TANDEM" if $abbrev eq "TDUP"; | ||
| $abbrev = "CNV:TR" if $abbrev eq "TREP"; | ||
| $abbrev =~ s/_/:/ if $abbrev =~ /^(INS|DEL)_ME$/; |
There was a problem hiding this comment.
This is correct as of now, but is not future-proof in case other INS/DEL subtypes are supported in the future. Maybe we could change the INS/DEL to contain the ME, such as INS_ME_ALU instead of INS_ALU to be more precise.
This is a bit nitpick-y so maybe just ignore this comment. 😛
There was a problem hiding this comment.
Noted! but I think that requires a bigger change and not within the scope/objective of this PR.
Co-authored-by: Nuno Agostinho <nunodanielagostinho@gmail.com>
Reported in Ensembl/VEP_plugins#710
Related Ensembl/ensembl-vep#1643
We have hash to convert from symbolic alternative allele from VCF to SO term in several places. It is best have it in one place to avoid errors.