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

new demultiplexed illumina formats #111

Merged
merged 11 commits into from
Mar 31, 2017
Merged

new demultiplexed illumina formats #111

merged 11 commits into from
Mar 31, 2017

Conversation

gregcaporaso
Copy link
Member

@gregcaporaso gregcaporaso commented Mar 28, 2017

fixes #82
fixes #98

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 93.646% when pulling f8b5b1c on gregcaporaso:issue-82 into ad67afa on qiime2:master.

…#82

This adds the formats:
 - SingleEndFastqManifestPhred33
 - SingleEndFastqManifestPhred64
 - PairedEndFastqManifestPhred33
 - PairedEndFastqManifestPhred64
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 93.069% when pulling 09c69ea on gregcaporaso:issue-82 into ad67afa on qiime2:master.

Copy link
Member

@ebolyen ebolyen left a comment

Choose a reason for hiding this comment

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

A couple of stylistic things where the DirectoryFormat API could be used a little more, but not hugely important.

The tests should use temp_dir however.

def test_parse_and_validate_manifest_expand_vars(self):
with tempfile.NamedTemporaryFile() as fh:
directory, filename = os.path.split(fh.name)
os.environ['TESTENVGWAR'] = directory
Copy link
Member

Choose a reason for hiding this comment

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

🥇


metadata = YamlFormat()
metadata.path.write_text(yaml.dump({'phred-offset': 33}))
result.metadata.write_data(metadata, YamlFormat)
Copy link
Member

Choose a reason for hiding this comment

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

Not really worth it (and it looks like the other transformer is doing the same thing as you), but you could define a transformer from object -> YamlFormat then you could drop the line just above this one with:
result.metadata.write_data({'phred-offset': 33}, YamlFormat)

Same could be done for pd.DataFrame -> FastqManifestFormat.

Basically write_data can invoke other transformers (just like Artifact.import_data)

format_,
SingleLanePerSampleSingleEndFastqDirFmt)

with tempfile.TemporaryDirectory() as tmpdir:
Copy link
Member

Choose a reason for hiding this comment

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

use self.temp_dir (derived from TestPluginBase) instead of all of these TemporaryDirectorys. temp_dir will have a reasonable name in case something goes wrong, making cleanup easier on the user.

_parse_and_validate_manifest(manifest, single_end=True)

def test_parse_and_validate_manifest_expand_vars(self):
with tempfile.NamedTemporaryFile() as fh:
Copy link
Member

Choose a reason for hiding this comment

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

Use self.temp_dir with a fixed name like manifest.txt instead.

for idx, sample_id, input_fastq_fp, direction in \
input_manifest.itertuples():
output_fastq_fn = \
_get_fastq_fn(sample_id, idx, direction)
Copy link
Member

Choose a reason for hiding this comment

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

I think you can replace this with result.sequences.path_maker(...).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 93.05% when pulling 0e4a824 on gregcaporaso:issue-82 into ad67afa on qiime2:master.

Copy link
Member

@ebolyen ebolyen left a comment

Choose a reason for hiding this comment

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

self.temp_dir.name is only the name of directory, not it's path. You need either str(self.temp_dir) or when you are joining, you can just use /.

Copy link
Member

@ebolyen ebolyen left a comment

Choose a reason for hiding this comment

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

Sorry, it's not pathlib.Path.name, it's TemporaryDirectory.name. Last review was wrong.

@ebolyen ebolyen merged commit e37a040 into qiime2:master Mar 31, 2017
@gregcaporaso gregcaporaso deleted the issue-82 branch April 3, 2017 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants