Skip to content

Add read_evidence module#53

Merged
timodonnell merged 2 commits intomasterfrom
add-read-evidence
Mar 30, 2015
Merged

Add read_evidence module#53
timodonnell merged 2 commits intomasterfrom
add-read-evidence

Conversation

@timodonnell
Copy link
Copy Markdown
Contributor

  • Add read_evidence module, tests, and test data. This is quite slow, and there are several optimization opportunities, but I wanted to get the API settled first.
  • Add locus module.
  • Add pysam dependency.
  • Add all_standard_nucleotides function in nucleotides module.

Review on Reviewable

 * Add read_evidence module, tests, and test data. This is quite slow, and there are several optimization opportunities, but I wanted to get the API settled first.
 * Add locus module.
 * Add pysam dependency.
 * Add `all_standard_nucleotides` function in nucleotides module.
@iskandr
Copy link
Copy Markdown
Contributor

iskandr commented Mar 30, 2015

Comments from the review on Reviewable.io


varcode/locus.py, line 30 [r1] (raw file):
Does the Python 3 vs. Python 2 difference in generator vs. list matter in how this gets used?


varcode/locus.py, line 39 [r1] (raw file):
Isn't str(self) redundant with the "%s" format string?


varcode/read_evidence.py, line 64 [r1] (raw file):
Could you add an example of what's actually in the tuple?


varcode/read_evidence.py, line 286 [r1] (raw file):
What do you think of moving this to its own file? And maybe also making pileup.py?


varcode/read_evidence.py, line 574 [r1] (raw file):
Why not use a collections.Counter?


Also address a few minor issues raised in code review.
@timodonnell
Copy link
Copy Markdown
Contributor Author

Comments from the review on Reviewable.io


varcode/locus.py, line 30 [r1] (raw file):
Yeah, I guess it depends on how it gets used. My intention is to not rely on it being a list. I'm not sure how to enforce that though.


varcode/locus.py, line 39 [r1] (raw file):
Done.


varcode/read_evidence.py, line 64 [r1] (raw file):
I have a feeling that bit may change a bunch and would rather not duplicate that info around the codebase. I'll add a pointer in the doc to the function that generates the tuple though, so someone could find out themselves easily.


varcode/read_evidence.py, line 286 [r1] (raw file):
Sure, refactored into submodule.


varcode/read_evidence.py, line 574 [r1] (raw file):
Made this clearer with a comment and by pulling out the lambda into a def.


@iskandr
Copy link
Copy Markdown
Contributor

iskandr commented Mar 30, 2015

Comments from the review on Reviewable.io


LGTM


timodonnell added a commit that referenced this pull request Mar 30, 2015
@timodonnell timodonnell merged commit b42442b into master Mar 30, 2015
@timodonnell timodonnell deleted the add-read-evidence branch March 30, 2015 22:15
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