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.

Sign up for GitHub

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

Jump to bottom

[Fix] fix translation table #1485

Merged
merged 1 commit into from
Jan 15, 2020
Merged

[Fix] fix translation table #1485

merged 1 commit into from
Jan 15, 2020

Conversation

sarahet
Copy link
Member

@sarahet sarahet commented Jan 14, 2020

No description provided.

@sarahet sarahet requested a review from h-2 January 14, 2020 09:49
@sarahet
Copy link
Member Author

sarahet commented Jan 14, 2020 •
edited
Loading

A test that the table is correct now is provided here. For each dna15 triplet a vector of all possible dna4 triplets is computed and checked whether the resulting amino acids (translation in SeqAn2 as gold standard translation) are all equal. If yes, this amino acid is picked, if not X is returned. This makes it possible that in the rare case of a triplet containing a letter from the dna15 alphabet that equals more than one dna4 letter but corresponds in any case to the same amino acid, the actual amino acid can be picked instead of an unknown acid.

h-2
h-2 approved these changes Jan 14, 2020
View reviewed changes
Copy link
Member

@h-2 h-2 left a comment

Choose a reason for hiding this comment

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

Well, I am hardly going to re-compute this table by hand to verify it 😆

If you say it's correct, we'll go with it.

But maybe a test for dna4->aa27 would be helpful as that is easier to verify? Could also happen in separate PR.

include/seqan3/alphabet/aminoacid/translation_details.hpp
@@ -257,14 +255,14 @@ struct translation_table<dna15, seqan3::genetic_code::CANONICAL, void_type>
{ 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27 }, // tb?
{ 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27, 'S'_aa27 }, // tc?
{ 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27 }, // td?
{ '*'_aa27, 'X'_aa27, 'C'_aa27, 'X'_aa27, 'W'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'C'_aa27, 'X'_aa27, 'C'_aa27 }, // tg?
{ '*'_aa27, 'X'_aa27, 'C'_aa27, 'X'_aa27, 'W'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'C'_aa27, 'X'_aa27, 'X'_aa27, 'C'_aa27 }, // tg?
Copy link
Member

Choose a reason for hiding this comment

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

There is a swap between X and C here, that's intendend?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

include/seqan3/alphabet/aminoacid/translation_details.hpp
{ 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27 }, // th?
{ 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27 }, // tk?
{ 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27 }, // tm?
{ 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27 }, // tn?
{ '*'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27 }, // tr?
{ 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27 }, // ts?
{ 'L'_aa27, 'X'_aa27, 'F'_aa27, 'X'_aa27, 'X'_aa27, 'L'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'L'_aa27, 'X'_aa27, 'F'_aa27, 'X'_aa27, 'X'_aa27, 'F'_aa27 }, // tt?
{ 'L'_aa27, 'X'_aa27, 'F'_aa27, 'X'_aa27, 'L'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'X'_aa27, 'L'_aa27, 'X'_aa27, 'F'_aa27, 'X'_aa27, 'X'_aa27, 'F'_aa27 }, // tt?
Copy link
Member

Choose a reason for hiding this comment

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

L and X swap here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

smehringer
smehringer requested changes Jan 14, 2020
View reviewed changes
Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

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

Please rebase (there was a fix for master) and also update the Changelog.md

@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #1485 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1485      +/-   ##
==========================================
+ Coverage   97.56%   97.56%   +<.01%     
==========================================
  Files         235      235              
  Lines        8837     8841       +4     
==========================================
+ Hits         8622     8626       +4     
  Misses        215      215
Impacted Files Coverage Δ
include/seqan3/core/detail/type_inspection.hpp 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 218c1fa...b9cddaa. Read the comment docs.

@smehringer smehringer merged commit a1d47da into seqan:master Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h-2 h-2 h-2 approved these changes

@smehringer smehringer smehringer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sarahet @h-2 @smehringer