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

Remove redundant symbolic alt to so term hash #1075

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

nakib103
Copy link
Contributor

@nakib103 nakib103 commented Mar 21, 2024

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.

Copy link
Contributor

@nuno-agostinho nuno-agostinho left a comment

Choose a reason for hiding this comment

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

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!

modules/Bio/EnsEMBL/Variation/Utils/Config.pm Show resolved Hide resolved
modules/Bio/EnsEMBL/Variation/Utils/VEP.pm Outdated Show resolved Hide resolved
modules/Bio/EnsEMBL/Variation/Utils/VEP.pm Outdated Show resolved Hide resolved
modules/Bio/EnsEMBL/Variation/Utils/VEP.pm Outdated Show resolved Hide resolved
@nakib103 nakib103 changed the base branch from postreleasefix/112 to main May 1, 2024 09:46
@nakib103 nakib103 marked this pull request as draft May 1, 2024 09:46
@nakib103 nakib103 added the e113 label May 1, 2024
@nakib103 nakib103 added e114 and removed e113 labels Aug 20, 2024
@nakib103
Copy link
Contributor Author

@nuno-agostinho,
The unit test on one before the last take accounts the change in ensembl-variation and passed. You can ignore the unit test failure in the last commit.

@nakib103 nakib103 marked this pull request as ready for review August 21, 2024 13:26
Copy link
Contributor

@nuno-agostinho nuno-agostinho left a comment

Choose a reason for hiding this comment

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

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).

modules/Bio/EnsEMBL/Variation/Utils/VEP.pm Outdated Show resolved Hide resolved

$abbrev = "DUP:TANDEM" if $abbrev eq "TDUP";
$abbrev = "CNV:TR" if $abbrev eq "TREP";
$abbrev =~ s/_/:/ if $abbrev =~ /^(INS|DEL)_ME$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

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. 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted! but I think that requires a bigger change and not within the scope/objective of this PR.

Co-authored-by: Nuno Agostinho <[email protected]>
@dglemos dglemos assigned dglemos and unassigned nuno-agostinho Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants