Basic implementation of molecular_weight function #341
Conversation
There was a problem hiding this comment.
Thank you for your contribution, @Alessio-Agustini !
I have a lot of comments to improve the code. Don't let that discourage you from an otherwise good PR.
If you have any questions, please reach out here or on the Julia Slack and I'll explain what I mean in more detail.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #341 +/- ##
==========================================
- Coverage 90.87% 85.95% -4.93%
==========================================
Files 31 30 -1
Lines 2400 3018 +618
==========================================
+ Hits 2181 2594 +413
- Misses 219 424 +205
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
First of all, thanks for the detailed feedback. I am still learning julia and it was really helpful. I made the requested changes and also added strand_number as new keyword for the NucSeq implementation of the molecular_weight() function. For the moment the function only supports double as strand_number for DNA sequences. |
|
I just finished with the second round of feedback corrections. As stated before, I am really thankful for the comments, I am still learning and this was a good exercise. |
|
Great!
|
|
I made the docstring for the functions and some basic tests in src/weight.jl. Probably better test would be better, but I don't have much experience with the test package. If you have any suggestions, I would be glad to read them. I don't know how to change/made documentations, so I would be glad, if you could do that. I also corrected a minor error with the compensation value for phosphate as 5' group. Now it is + 18.06. The nucleotide already have the phosphate in that position and would be only required to account for the water, because in the function there is: length(Nucseq) * water, instead of length ((Nucseq) -1 )* water. With the new value, the calculated weights are compatible with the ones of the listed website tool, which is mentioned in the code as comment. |
* Place the source comment of each molecule where it is defined * Use Runic.jl to automatically format this file
This is more readable than using the literal "18.02"
This avoids loading twice from the array. Maybe the compiler would optimise this, but it's safer to do this optimisation manually.
|
Thanks again! The PR is almost complete. I'll make a series of small changes to get it though. You can read the commit comments to follow along if you want to learn why and how |
* Switch from LongSequence to BioSequence to broaden the types of sequences
that this signature accepts
* Switch from DNAAlphabet{4} to <:DNAAlphabet (and similar for RNA alphabets) to
also allow 2-bit alphabet sequences
* Switch strandedness kwarg to a bool, since we only want to covert two cases
* Restrict five_terminal_state to Symbol
* The internal _molecular_weight function does not need default values
Thank you for your contribution!
If you have any questions about your PR, or need help completing it, you can ping the maintainers of this repository, who will be happy to help if they can find time.
You can optionally use the following checklist when you work on your PR:
Modifications:
*added src/molecular-weight.jl
*modified src/BioSequences.jl to also load the new file
*recently added the previously mentioned implementation of the function for ssDNA and ssRNA
WIP
*Still working in the necessary additions into test/
*Still missing changes in the CHANGELOG.md