Skip to content
This repository was archived by the owner on May 2, 2024. It is now read-only.

Conversation

@zhaoc1
Copy link
Collaborator

@zhaoc1 zhaoc1 commented Jan 17, 2018

There are four major parts in this pull request.

(1) Update the unit test codes for testing config_file as an option
(2) Add the benchmark work from Celeste
(3) Use python from the environment.
(4) Add single-end reads as an option: used for stitched paired-ends reads

Copy link
Member

@kylebittinger kylebittinger left a comment

Choose a reason for hiding this comment

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

Just a few minor tips.

args.reverse_reads.close()

if args.reverse_reads is None:
rev_fp = str(None)
Copy link
Member

Choose a reason for hiding this comment

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

Recommend not converting to string here. In Python, it is most often best to use None itself to represent a null value. Later on in the decontaminate() method, you can do this:

if rev_fp is not None:

instead of:

if rev_fp != str(None):

@@ -1,3 +1,3 @@
#!/usr/bin/python
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to remove the scripts directory, and instead have setup.py make the scripts for us. If we make this switch, Python will wire up the correct interpreter for the scripts it creates.

For an example, see the console_scripts argument here:
https://github.com/kylebittinger/unassigner/blob/master/setup.py

# This program always returns non-zero exit status
self.args.extend(["--method", "bwa"])
self.args.extend(["--bwa_fp", "false"])
config_file = tempfile.NamedTemporaryFile(suffix=".json")
Copy link
Member

Choose a reason for hiding this comment

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

Config files are probably more trouble than they're worth. Let's not remove the config files now, but just regard this as a sign that things will be much easier once we do.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants