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

Add functions for converting barcode info files into codon variant tables #15

Closed
wants to merge 10 commits into from
Closed

Conversation

caelanradford
Copy link
Contributor

Add the function substitutions for determining substitutions present in a sequence compared to a wildtype sequence, and the function bc_info_to_codonvarianttable for converting barcode info files into codon variant tables. Also modified the function func_score_to_gpm to allow inputs of amino acid substitutions rather than codon.

@jbloom
Copy link
Member

jbloom commented Mar 28, 2019

@caelanradford:

The changes to func_score_to_gpm don't make sense given the documentation. The docs for a function are supposed to allow a reader to understand what it does without looking at the actual code. So adding a new option (aaSubs) that is documented in terms of how it affects codonSubsToSeq doesn't make sense, because someone can't understand how this impacts the function output unless they look at the actual code as codonSubsToSeq is a separate function called internally. So if you want to add this option to func_score_to_gpm then you need to explain what you are adding it in the context of that function.

Also, I was just looking more carefully at the doc formatting for func_score_to_gpm, and that should be cleaned up to use proper sphinx formatting. Look at the online docs; your args aren't formatted properly as you use single quotes rather than backticks. It would also probably be good to better document what the func_score_df is (i.e., comes from :meth:CodonVariantTable.func_scores for instance).

Finally, I'm not exactly sure how this happened, but this pull request will not lead to a clean git history. Despite the fact that the pull request has a title indicating that it is for converting barcode info files into variant tables, it shows commits going back to Feb 12 that do all sorts of things including adding the func_scores_to_gpm method (which is totally unrelated), adding tests for that, etc. In order to keep a clean commit history that makes it possible to understand the changes, each pull request should address just one topic that is accurately reflected in its title. Read here to understand this: https://medium.com/@fagnerbrack/one-pull-request-one-concern-e84a27dfe9f1

So I would suggest that make a new fork (or update yours) and make separate "atomic pull requests" (in the way described in the above article) that only focus on whatever specific thing that you are trying to change in that pull request.

@caelanradford caelanradford changed the base branch from master to gh-pages March 28, 2019 18:00
@caelanradford caelanradford changed the base branch from gh-pages to master March 28, 2019 18:00
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