search

brentp / geneimpacts

prioritize effects of variant annotations from VEP, SnpEff, et al.
MIT License
32 stars 15 forks source link

use HGVS.p for aa_change (ANN) #4

Closed bpow closed 8 years ago

bpow commented 8 years ago

This isn't the exact same as how aa_change has been represented in the past (it uses HGVS representation like p.Arg457His instead of R457H, but they are semantically the same.

To unify representations would require removing the p. prefix and changing three-letter AA codes to single letter AA codes.

brentp commented 8 years ago

good idea, what do you think about keeping 

        if 'Amino_Acid_change' in self.effects:
            return self.effects['Amino_Acid_change']

after your addition? Then it can handle the older snpEff as well.

brentp commented 8 years ago

... or placing that snippet inside of OldSnpEff.aa_change

bpow commented 8 years ago

Could go either way (include 2nd 'if' statement in SnpEff.aa_change or override in OldSnpEff.aa_change) depending on how strictly SnpEff should be with regards to expecting specifically ANN-formatted changes. I agree that should go back in there somewhere to prevent the parsing of the old format from breaking. I hadn't realized that the OldSnpEff didn't already have an aa_change property,

brentp commented 8 years ago

OldSnpEff inherits from SnpEff, so it had it implicitly. I think it should go in OldSnpEff.aa_change. If you don't beat me to it, I'll add it and a test in the next few days.

bpow commented 8 years ago

OK, rebased and updated the pull request with the ability for OldSnpEff to populate aa_change as well.

Frustratingly, some versions of SnpEff use Amino_Acid_Change and some use Amino_Acid_change, so I added the ability to parse from both...

Tests also updated/included.

brentp commented 8 years ago

cheers!