protein-translation: Add canonical data. #997

sjwarner-bp · 2017-11-08T15:41:28Z

Add protein translation canonical data as per #577.

After going to implement this exercise in Java, and realising that there is not a canonical-data.json for the problem, I decided to do this. I have helped a friend complete this exercise on the Python track, and believe the Python tests to be very comprehensive, so I have used them as my main source of data.

Please let me know if this is too long for a canonical-data.json, as there are about 20 tests...

rpottsoh · 2017-11-08T15:52:01Z

Problem description.

closes #577

cmccandless

@sjwarner-bp Regarding the length, this is small compared to some (clock, for example).

cmccandless · 2017-11-08T15:48:59Z

exercises/protein-translation/canonical-data.json

+          "description": "Translate RNA strand into correct protein list",
+          "property": "translate",
+          "strand": "AUGUUUUGG",
+          "expected": ["Methionine","Phenylalanine","Tryptophan"]


The type returned by translate should be consistent; until this line, the output of translate has been a string, but now a list is expected. The python test cases use two different properties of_codon and of_rna for these; something similar would be appropriate.

Ah of course - my mistake! I'll correct that 🙂

This feels a lot like what has been discussed recently at #983. "translate_codon" seems to be an intermediate test of what the description is really asking for. The "translate_codon" tests may not belong. @petertseng, @Insti thoughts?

@rpottsoh I believe @ErikSchierboom beat you to it. See his comments.

sjwarner-bp · 2017-11-08T16:03:28Z

Oh dear - I've made changes but the Travis build has failed. Do I need to add the new property names (translate_codon,translate_rna) somewhere?

ErikSchierboom

This is already looking good! I have some points of discussion, which I've added as comments. Thanks for working on this!

ErikSchierboom · 2017-11-08T15:58:36Z

exercises/protein-translation/canonical-data.json

+    {
+      "description": "Translate input RNA sequences into proteins",
+      "comments": [
+        " Returns the name of the protein if given RNA is valid, "


Maybe lose the extra space in the beginning of the string?

ErikSchierboom · 2017-11-08T15:58:42Z

exercises/protein-translation/canonical-data.json

+      "description": "Translate input RNA sequences into proteins",
+      "comments": [
+        " Returns the name of the protein if given RNA is valid, "
+      , " else throws an error. "


Maybe lose the extra space in the beginning of the string?

ErikSchierboom · 2017-11-08T16:01:29Z

exercises/protein-translation/canonical-data.json

+      "cases": [
+        {
+          "description": "Methionine RNA sequence is identified",
+          "property": "translate_codon",


Lately, we have tried to have move away from testing intermediate results in the canonical data, and only test end results. For this canonical data, I would suggest to do the same and only have a single translate property to test. The existing translate_codon property tests (like this one) would then use the translate property and have as its expected value an array with a single value.

I like this approach better.

I didn't think of this, but it makes much more sense. I'll work on that.

ErikSchierboom · 2017-11-08T16:02:08Z

exercises/protein-translation/canonical-data.json

+        },
+        {
+          "description": "Translate RNA strand into correct protein list",
+          "property": "translate_rna",


In keeping in line with my comment above on testing only one property, I would suggest renaming this to translate.

cmccandless · 2017-11-08T16:08:51Z

@sjwarner-bp Regarding the failed Travis build, this from the canonical-schema:

"property":
        { "description": "A letters-only, lowerCamelCase property name"
        , "type"       : "string"
        , "pattern"    : "^[a-z]+([A-Z][a-z]+)*[A-Z]?$"
        },

sjwarner-bp · 2017-11-08T16:10:12Z

That makes sense - I'll keep this in mind for future.

ErikSchierboom · 2017-11-08T16:10:19Z

exercises/protein-translation/canonical-data.json

+          "expected": ["Methionine","Phenylalanine"]
+        },
+        {
+          "description": "Translation stops if codon present 2",


"Translation stops if codon" -> "Translation stops if STOP codon present 2"

Maybe describe how this case differs from the first one?

ErikSchierboom · 2017-11-08T16:12:53Z

exercises/protein-translation/canonical-data.json

+          "expected": ["Phenylalanine"]
+        },
+        {
+          "description": "Phenylalanine RNA sequence 2 is identified",


I am wondering if there is another way to discern between the different Phenylalanine types? They are currently named "Phenylalanine RNA sequence 1" and "Phenylalanine RNA sequence 2", but maybe there is a more descriptive name? If there isn't, just ignore this :)

From the problem description, I couldn't see a particularly good way of differentiating the two from each other - maybe if someone knows biology they could chime in, otherwise this might be as good as we can get it.

ErikSchierboom · 2017-11-08T16:13:35Z

exercises/protein-translation/canonical-data.json

+      ],
+      "cases": [
+        {
+          "description": "Methionine RNA sequence is identified",


I think the " is identified" postfix can be removed, as it doesn't add much extra information I think. But maybe someone else disagrees?

That makes sense to me - concise is good!

cmccandless · 2017-11-08T16:15:04Z

exercises/protein-translation/canonical-data.json

@@ -118,13 +118,13 @@
          "expected": ["Methionine","Phenylalanine","Tryptophan"]
        },
        {
-          "description": "Translation stops if STOP codon present 1",
+          "description": "Translation stops if STOP codon present in short string",
          "property": "translate",
          "strand": "AUGUUUUAA",
          "expected": ["Methionine","Phenylalanine"]


STOP is included in the above test cases where it is the only codon; either the above cases should expected empty lists as a result or STOP should be included in the results here.

cmccandless · 2017-11-08T16:15:57Z

exercises/protein-translation/canonical-data.json

@@ -118,13 +118,13 @@
          "expected": ["Methionine","Phenylalanine","Tryptophan"]
        },
        {
-          "description": "Translation stops if STOP codon present 1",
+          "description": "Translation stops if STOP codon present in short string",


Perhaps Translation stops if STOP codon at end of sequence vs below Translation stops if STOP codon in middle of sequence?

or
Translation stops when STOP codon encountered

Still need to distinguish between this case and the next... perhaps Translation stops when STOP codon encountered at end of sequence vs Translation stops when STOP codon encountered in middle of sequence?

cmccandless · 2017-11-08T16:16:21Z

exercises/protein-translation/canonical-data.json

          "property": "translate",
          "strand": "AUGUUUUAA",
          "expected": ["Methionine","Phenylalanine"]
        },
        {
-          "description": "Translation stops if codon present 2",
+          "description": "Translation stops if codon present in longer string",


Per my above comment, perhaps a change in wording?

rpottsoh

👍 looks good! Thanks @sjwarner-bp!

cmccandless · 2017-11-08T16:19:54Z

Love how fast this one's going!

@sjwarner-bp please see my latest comments, I'm afraid they might've been covered up by the flurry of activity.

sjwarner-bp · 2017-11-08T16:22:24Z

I think I got them - I've added 'STOP' to the end of each list where a 'STOP' codon is involved.

I also amended the description for the test of early termination - hope this was right! 🙂

rpottsoh · 2017-11-08T16:33:12Z

exercises/protein-translation/canonical-data.json

@@ -118,7 +118,7 @@
          "expected": ["Methionine","Phenylalanine","Tryptophan"]
        },
        {
-          "description": "Translation stops if STOP codon present in short string",
+          "description": "Translation stops if STOP codon at end of sequence",
          "property": "translate",
          "strand": "AUGUUUUAA",
          "expected": ["Methionine","Phenylalanine", "STOP"]


Should "STOP" be included in "expected"? The stop example in the README didn't include stop in the resulting protein.

This was added because of @cmccandless point (above) regarding consistency between tests. If "STOP" is expected in the tests for "STOP" codons, it might make sense to include them in the list of expected proteins?

Equally, it might not make too much sense - the Python track does not include them in protein lists, and only identifies them if they are input as a single codon.

What do we think? I'd be interested in hearing which way people think makes most sense and still remains consistent.

Perhaps then:

"strand": "UAA", "expected": ""

or update the README to list the STOP protein in the stop example....

I think it would be better to expect an empty list in the case of "expected": ["STOP"]

Ok, empty list coming up!

rpottsoh

👍

Insti

You all seem to be doing a good job on this 👍 keep it up.

Insti · 2017-11-08T22:47:48Z

exercises/protein-translation/canonical-data.json

+          "expected": ["Tryptophan","Cysteine","Tyrosine"]
+        },
+        {
+          "description": "Test invalid codons",


Don't test invalid codons.

See also: #902

Good point. Maybe remove this test?

Insti · 2017-11-08T22:52:15Z

exercises/protein-translation/canonical-data.json

+      "cases": [
+        {
+          "description": "Methionine RNA sequence",
+          "property": "translate",


What about proteins as the property?

Insti · 2017-11-08T22:56:11Z

exercises/protein-translation/canonical-data.json

+          "expected": []
+        },
+        {
+          "description": "STOP codon RNA sequence 3",


I'd like to see tests for more of the simple STOP codon sequences.
UAGUGG
UGGUAG
UGGUAGUGG

Insti · 2017-11-08T22:57:03Z

exercises/protein-translation/canonical-data.json

+        {
+          "description": "Translation stops if codon in middle of sequence",
+          "property": "translate",
+          "strand": "UGGUGUUAUUAAUGGUUU",


Put some thought put into which sequences are easiest for humans to read and distinguish.

…existing stop codon tests, remove testing for invalid data.

ErikSchierboom · 2017-11-10T11:12:32Z

Merged. Thanks @sjwarner-bp! 🎉

Add protein translation canonical data as per exercism#577.

aed9084

rpottsoh changed the title ~~Add protein translation canonical data as per #577.~~ protein-translation: Add canonical data. Nov 8, 2017

cmccandless suggested changes Nov 8, 2017

View reviewed changes

Update properties to reflect different return types.

4c5b8e8

ErikSchierboom requested changes Nov 8, 2017

View reviewed changes

Update to test only one property, and fix formatting.

34c34a1

cmccandless approved these changes Nov 8, 2017

View reviewed changes

ErikSchierboom requested changes Nov 8, 2017

View reviewed changes

Update test description for clarity.

be571bd

ErikSchierboom requested changes Nov 8, 2017

View reviewed changes

cmccandless suggested changes Nov 8, 2017

View reviewed changes

rpottsoh approved these changes Nov 8, 2017

View reviewed changes

Reduced length of test descriptions.

135bcbe

Update tests including 'STOP' codons for consistency.

a3e63c7

Update 'STOP' test description

91095fa

cmccandless approved these changes Nov 8, 2017

View reviewed changes

rpottsoh reviewed Nov 8, 2017

View reviewed changes

Update 'STOP' codon tests to expect an empty list.

a0be642

rpottsoh reviewed Nov 8, 2017

View reviewed changes

cmccandless approved these changes Nov 8, 2017

View reviewed changes

Insti reviewed Nov 8, 2017

View reviewed changes

Rename 'property', add a larger range of STOP codon tests, rearrange …

630c53d

…existing stop codon tests, remove testing for invalid data.

ErikSchierboom approved these changes Nov 9, 2017

View reviewed changes

Insti approved these changes Nov 9, 2017

View reviewed changes

sjwarner-bp mentioned this pull request Nov 9, 2017

Protein Translation: Implement problem in Java language stream. exercism/java#985

Merged

ErikSchierboom merged commit 5c2b2b5 into exercism:master Nov 10, 2017

ErikSchierboom mentioned this pull request Nov 10, 2017

protein-translation: Implement canonical-data.json #577

Closed

cmccandless mentioned this pull request Nov 21, 2017

Update protein-translation tests exercism/python#1115

Merged

protein-translation: Add canonical data. #997

protein-translation: Add canonical data. #997

Conversation

sjwarner-bp commented Nov 8, 2017

rpottsoh commented Nov 8, 2017 • edited Loading

cmccandless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpottsoh Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjwarner-bp commented Nov 8, 2017

ErikSchierboom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmccandless commented Nov 8, 2017

sjwarner-bp commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpottsoh left a comment

Choose a reason for hiding this comment

cmccandless commented Nov 8, 2017

sjwarner-bp commented Nov 8, 2017

Choose a reason for hiding this comment

sjwarner-bp Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpottsoh left a comment

Choose a reason for hiding this comment

Insti left a comment

Choose a reason for hiding this comment

Insti Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSchierboom commented Nov 10, 2017

rpottsoh commented Nov 8, 2017 •

edited

Loading

rpottsoh Nov 8, 2017 •

edited

Loading

sjwarner-bp Nov 8, 2017 •

edited

Loading

Insti Nov 8, 2017 •

edited

Loading