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

Added protein translation practice exercise #693

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

Conversation

golanor
Copy link
Contributor

@golanor golanor commented Jan 25, 2024

Here is a suggestion for the protein-translation exercise in Julia using string macros. Let me know what you think.

@cmcaine
Copy link
Contributor

cmcaine commented Jan 28, 2024

Hi, thanks for your interest. Please rebase and make the tests pass, then ping reviewers again.

@golanor
Copy link
Contributor Author

golanor commented Jan 30, 2024

Hi @cmcaine ,
There seems to be an issue with some versions of Julia when running this test. Do you suggest I change the test so that it would always pass, or should we try fixing the runner so there won't be a difference? It works for 1, and for the nightly (and 1.10 on my machine)

@cmcaine
Copy link
Contributor

cmcaine commented Jan 30, 2024

Julia 1.6 is the current LTS release, so I think we should still support it. The bug is probably because the macros are being expanded at different times on different releases, which might have been done deliberately or not.

We could fix that with eval (@test_throws TranslationError eval(:(@macroexpand rna"foo"))), but I think we should require the user to write a function that the string macro is then expected to call. I think this is usually the better option, and is definitely the better option in this case where it is very plausible that a user would want to find the translations of strings that are not known at compile time.

How does that sound to you?

Copy link
Contributor

@cmcaine cmcaine left a comment

Choose a reason for hiding this comment

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

as noted above

@golanor
Copy link
Contributor Author

golanor commented Jan 30, 2024

I made the changes, but then the string macro is a bit redundant, isn't it?

@golanor golanor requested a review from cmcaine January 30, 2024 13:19
@@ -0,0 +1,51 @@
codon_protein_dict = Dict(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
codon_protein_dict = Dict(
const codon_protein_dict = Dict(

Comment on lines 1 to 8
macro rna_str()
# I'm a ribosome macro!
end


function rna_translator()
# I'm a ribosome function!
end
Copy link
Contributor

@cmcaine cmcaine Jan 30, 2024

Choose a reason for hiding this comment

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

Reordering these to encourage the student to work on the function first.

Also adding a link to the docs because macros are confusing.

Suggested change
macro rna_str()
# I'm a ribosome macro!
end
function rna_translator()
# I'm a ribosome function!
end
function rna_translator()
# I'm a ribosome function!
end
# Read the manual for more information on string macros:
# https://docs.julialang.org/en/v1/manual/metaprogramming/#meta-non-standard-string-literals
macro rna_str()
# I'm a ribosome macro!
end

@cmcaine
Copy link
Contributor

cmcaine commented Jan 30, 2024

The macro serves the same purpose as other string macros:

  1. Mostly a convenient/pretty shorthand
  2. Also a way to ensure a computation can happen at compile-time (good for e.g. Regex so that the moderately expensive PCRE regex compilation step can be done at the same time as the Julia compilation step).

What do you want the student to learn from the exercise?

I suggest changing all instances of rna"..." in the tests to rna_translator("...") and adding a couple of tests of the macro at the end.

Reason: the function interface is easier to test and understand for the student.

Alternatively, we could remove the macro version entirely.

@golanor
Copy link
Contributor Author

golanor commented Feb 2, 2024

I wanted to teach both macro writing and creating an exception. I'll settle for only learning how to create a new exception.

@golanor golanor requested a review from cmcaine February 2, 2024 06:29
@@ -0,0 +1,3 @@
function rna_translator(rna::String)
Copy link
Contributor

Choose a reason for hiding this comment

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

Function names should describe an action, not an object, according to the Julia style guide and general practice.

How about one of these names? Presented in my preference order.

Suggested change
function rna_translator(rna::String)
function rna_to_amino_acids(rna::String)
Suggested change
function rna_translator(rna::String)
function rna_to_protein(rna::String)
Suggested change
function rna_translator(rna::String)
function translate_rna(rna::String)

@cmcaine
Copy link
Contributor

cmcaine commented Feb 6, 2024

Feel free to put the string macro back in as a bonus question, if you like. The rot13 practice exercise has an example of this.

@golanor golanor requested a review from cmcaine February 7, 2024 08:52
@golanor
Copy link
Contributor Author

golanor commented Feb 7, 2024

Renamed and added the macro as a bonus question

@golanor
Copy link
Contributor Author

golanor commented Feb 24, 2024

Can we merge this?

@cmcaine
Copy link
Contributor

cmcaine commented Feb 25, 2024

Sorry, I missed the updates. I'll try to take a look this week. Please ping me again if I don't.

Copy link
Contributor

@cmcaine cmcaine left a comment

Choose a reason for hiding this comment

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

Note to self. Last two commiys look good. Like the rename. Recheck the diff with master before merge.

@golanor
Copy link
Contributor Author

golanor commented Mar 7, 2024

ping @cmcaine

@golanor
Copy link
Contributor Author

golanor commented Apr 3, 2024

@cmcaine Can we merge?

Copy link
Contributor

@cmcaine cmcaine 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 the reminder.

What do you think of these suggestions?

config.json Outdated Show resolved Hide resolved
exercises/practice/protein-translation/runtests.jl Outdated Show resolved Hide resolved
@golanor
Copy link
Contributor Author

golanor commented Apr 19, 2024

I comitted the suggested changes

@cmcaine
Copy link
Contributor

cmcaine commented Apr 19, 2024

Cool. The example.jl will need to be changed to throw Argument error.

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.

2 participants