-
Notifications
You must be signed in to change notification settings - Fork 78
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
saved fasta files incorrectly - Bug fixed #740
Conversation
Codecov Report
@@ Coverage Diff @@
## master #740 +/- ##
==========================================
- Coverage 88.63% 88.41% -0.22%
==========================================
Files 30 30
Lines 4777 4783 +6
Branches 46 46
==========================================
- Hits 4234 4229 -5
- Misses 540 551 +11
Partials 3 3
Continue to review full report at Codecov.
|
there is a test that fails randomly and here is the error - https://travis-ci.com/dib-lab/sourmash/jobs/236306024#L842 1. I ran it locally it passes 2. I went through other branches, it seemed like it failed in those branches randomly as well 3. the travis test passed in this commit a2ef27a and after adding two more lines in test, and no change in program at all, this commit 818e088 it failed. So, I am assuming there is some random thing going on or it doesn't like the generator! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I had one comment about adding the option to save fastas to a specific prefix or folder, but I don't think it's necessary for PR approval.
sourmash/command_compute.py
Outdated
@@ -84,7 +84,7 @@ def compute(args): | |||
parser.add_argument('-p', '--processes', default=2, type=int, | |||
help='For 10x input only (i.e input-is-10x flag is True, ' | |||
'Number of processes to use for reading 10x bam file') | |||
parser.add_argument('--save-fastas', type=str, | |||
parser.add_argument('--save-fastas', action='store_true', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! Would it be possible to provide a directory or prefix to save all the created fastas to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to take a path instead
I opened #755 to discuss coverage for |
test_cmd_signature is failing - https://travis-ci.com/dib-lab/sourmash/jobs/248344113#L536 |
Merged, as the failing test is not an issue of this PR. Thanks @pranathivemuri ! |
When saving the fasta files for each unique barcode, I was repeatedly writing the name and the sequence, for the same barcode. I.E the fasta file should have had only one > followed by barcode name. But by using >{}{} barcode inside the loop, for every sequence I was repeating the >barcode. I corrected that in this PR.
I have also updated the comments
And made save_fastas cli argument a boolean
@olgabot @ctb Sorry about the error! not ready for review yet!
make test
Did it pass the tests?make coverage
Is the new code covered?without a major version increment. Changing file formats also requires a
major version number increment.
changes were made?