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

Removed partial codon warning when testing amino acid sequences #3307

Closed
wants to merge 2 commits into from

Conversation

kaskales
Copy link
Contributor

Only changed the testing amino acid sequences so the warning isn't triggered. Amino acid sequences don't need to be divisible by 3, only codon sequences.

Trimmed three testing protein_seqs by one amino acid to prevent irrelevant partial codon warning from appearing when testing TranslationError of amino acid Seq() object.

  • I hereby agree to dual licence this and any previous contributions under both
    the Biopython License Agreement AND the BSD 3-Clause License.

  • I have read the CONTRIBUTING.rst file, have run pre-commit locally,
    and understand that AppVeyor and TravisCI will be used to confirm the Biopython unit
    tests and style checks pass with these changes.

  • I have added my name to the alphabetical contributors listings in the files
    NEWS.rst and CONTRIB.rst as part of this pull request, am listed
    already, or do not wish to be listed. (This acknowledgement is optional.)

Closes #...

…t partial codon warning from appearing when testing translation error of amino acid Seq() object.
@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #3307 into master will decrease coverage by 1.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3307      +/-   ##
==========================================
- Coverage   83.98%   82.90%   -1.09%     
==========================================
  Files         318      316       -2     
  Lines       51661    51511     -150     
==========================================
- Hits        43389    42705     -684     
- Misses       8272     8806     +534     
Impacted Files Coverage Δ
Bio/phenotype/pm_fitting.py 12.28% <0.00%> (-75.44%) ⬇️
Bio/PDB/mmtf/__init__.py 28.57% <0.00%> (-52.39%) ⬇️
Bio/codonalign/codonseq.py 44.16% <0.00%> (-44.31%) ⬇️
Bio/Phylo/_utils.py 28.57% <0.00%> (-35.72%) ⬇️
BioSQL/DBUtils.py 56.16% <0.00%> (-19.18%) ⬇️
BioSQL/BioSeqDatabase.py 68.61% <0.00%> (-9.31%) ⬇️
Bio/motifs/jaspar/db.py 3.08% <0.00%> (-5.41%) ⬇️
Bio/phenotype/phen_micro.py 83.81% <0.00%> (-2.26%) ⬇️
Bio/Phylo/BaseTree.py 88.93% <0.00%> (-0.21%) ⬇️
Bio/Seq.py 96.81% <0.00%> (-0.18%) ⬇️
... and 2 more

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 63ec6b5...3f59651. Read the comment docs.

@peterjc
Copy link
Member

peterjc commented Oct 13, 2020

I don't follow from the commit or pull request why you are changing this. If anything, it makes me wonder if there is a mistake in the tests?

@kaskales
Copy link
Contributor Author

There is no mistake in the test that I see. I was just curious and running the test_seq tests when I noticed a warning,

BiopythonWarning: Partial codon, len(sequence) not a multiple of three. Explicitly trim the sequence or add trailing N before translation. This may become an error in future.
BiopythonWarning,

that was being triggered by the test_translation_on_proteins test while assertRaises(TranslationError) was being caught.

I just thought it might make the test pass cleaner if the warning wasn't being triggered, since it seems to be aimed for codon sequences and not about the amino acids being tested. Shortening the test amino acid sequences doesn't change any of the other tests.

@MarkusPiotrowski
Copy link
Contributor

From a quick inspection it looks to me the reason that the test_translation_on_proteins passes in the first place is because a TranslationError is thrown for the wrong reason, i.e. that a 'codon' of three amino acids is not found in the table.

biopython/Bio/Seq.py

Lines 2407 to 2410 in 63ec6b5

else:
raise CodonTable.TranslationError(
f"Codon '{codon}' is invalid"
) from None

Since we cannot distinguish between nucleic and protein sequences in the Seq object any longer, the test which presumably has tested before that a protein cannot be translated, has become obsolete?

Copy link
Member

@peterjc peterjc left a comment

Choose a reason for hiding this comment

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

Thanks @kaskales, test_translation_on_proteins was the scope I wanted clarified. Your change makes sense to me.

@MarkusPiotrowski Since the Seq object no longer distinguishes nucleotide versus protein, a protein/peptide "AAA" can be translated just fine but "MED" fails as it does not make sense as a codon. The test motivation still makes sense to me.

Perhaps the length not a multiple of three warning should happen later in the translation (allowing the exceptions to trigger first). Also perhaps if using to_stop=True the warning should be skipped too (if an in-frame stop is found)?

@MarkusPiotrowski
Copy link
Contributor

@MarkusPiotrowski Since the Seq object no longer distinguishes nucleotide versus protein, a protein/peptide "AAA" can be translated just fine but "MED" fails as it does not make sense as a codon. The test motivation still makes sense to me.

Exactly. Because protein sequences may be translatable, this test, described as "Check translation fails on a protein." doesn't make sense any longer. If the sequence was "ATG" or "MAS", the test wouldn't fail.
There is another test, test_translation_of_invalid_codons, which could cover the "MED" case.

@peterjc
Copy link
Member

peterjc commented Oct 14, 2020

OK, if test_translation_of_invalid_codons covers it we can drop test_translation_of_invalid_codons 👍

@kaskales
Copy link
Contributor Author

Should I delete the test_translation_on_proteins, and push it to this PR, or make a new one for this?

@peterjc
Copy link
Member

peterjc commented Oct 14, 2020

Whichever is easier for you.

@kaskales
Copy link
Contributor Author

Are there any additional changes I should make for this PR?

Copy link
Member

@peterjc peterjc left a comment

Choose a reason for hiding this comment

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

Over to @MarkusPiotrowski for second opinion (and merge)?

@peterjc
Copy link
Member

peterjc commented Jan 1, 2024

This was resolved another way in #3446, so belated closing this now. Sorry @kaskales for not following up on this sooner.

@peterjc peterjc closed this Jan 1, 2024
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.

None yet

3 participants