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.

Sign up for GitHub

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

Jump to bottom

PerSampleDNAIterators doesn't take comments into account #129

Closed
jakereps opened this issue Jul 7, 2017 · 0 comments
Closed

PerSampleDNAIterators doesn't take comments into account #129

jakereps opened this issue Jul 7, 2017 · 0 comments
Assignees
@jairideout @maxvonhippel
Labels
type:bug Something is wrong.

Comments

@jakereps
Copy link
Member

jakereps commented Jul 7, 2017

SingleLanePerSampleSingleEndFastqDirFmt and SingleLanePerSamplePairedEndFastqDirFmt -> PerSampleDNAIterators transformers don't take the MANIFEST comments into account and crash when attempting to view the artifact as an iterator.

In [1]: import qiime2
In [2]: from q2_types.per_sample_sequences import PerSampleDNAIterators
In [3]: a = qiime2.Artifact.load('20170626_1/demux.qza')
In [4]: a.view(PerSampleDNAIterators)    
...
~/Developer/mc3/envs/biota/lib/python3.5/site-packages/q2_types/per_sample_sequences/_transformer.py in _1(dirfmt) 
     44     next(fh)
     45     for line in fh:
---> 46         sample_id, filename, _ = line.split(',')
     47         filepath = str(dirfmt.path / filename)
     48         result[sample_id] = skbio.io.read(filepath, format='fastq',

ValueError: not enough values to unpack (expected 3, got 1)

Autogenerated MANIFEST file:
image
@jakereps jakereps mentioned this issue Jul 7, 2017
Closed
@jairideout jairideout added the type:bug Something is wrong. label Jul 10, 2017
maxvonhippel added a commit to maxvonhippel/q2-types that referenced this issue Jul 21, 2017
@maxvonhippel
ENH/BUG/MAINT: support comments/blank lines and better validation in …
ee1ca12 
…"per-sample sequence" formats

All changes in this commit are related to the "per-sample sequence" formats.

Several changes:

- Comments and blank lines are now supported in "per-sample sequence" manifest formats, including sniffers and transformers.
- Duplicate samples are now additionally detected and disallowed in "relative path" manifest formats (previously this validation only happened in the "absolute path" manifest formats).
- Error messages reported by transformers are both more precise and accurate.
- Refactored sniffer and transformer code to reduce redundancy and perform consistent validation and parsing in all sniffers/transformers.
- Added unit tests, increased coverage to 100% for "per-sample sequence" formats, and made existing unit tests and test data more robust.

Fixes qiime2#126.
Fixes qiime2#129.
Fixes qiime2#132.

Pair-programmed with @jairideout.
@maxvonhippel maxvonhippel mentioned this issue Jul 21, 2017
Merged
thermokarst pushed a commit that referenced this issue Jul 24, 2017
@maxvonhippel @thermokarst
ENH/BUG/MAINT: support comments/blank lines and better validation in …
407a91f 
…"per-sample sequence" formats (#136)

All changes in this commit are related to the "per-sample sequence" formats.

Several changes:

- Comments and blank lines are now supported in "per-sample sequence" manifest formats, including sniffers and transformers.
- Duplicate samples are now additionally detected and disallowed in "relative path" manifest formats (previously this validation only happened in the "absolute path" manifest formats).
- Error messages reported by transformers are both more precise and accurate.
- Refactored sniffer and transformer code to reduce redundancy and perform consistent validation and parsing in all sniffers/transformers.
- Added unit tests, increased coverage to 100% for "per-sample sequence" formats, and made existing unit tests and test data more robust.

Fixes #126.
Fixes #129.
Fixes #132.

Pair-programmed with @jairideout.
jairideout added a commit to jairideout/q2-types that referenced this issue Aug 16, 2017
@jairideout
BUG: remove PerSampleDNAIterators and PerSamplePairedDNAIterators
a522714 
Removes `q2_types.per_sample_sequences.PerSampleDNAIterators` and `q2_types.per_sample_sequences.PerSamplePairedDNAIterators` view types, along with their corresponding transformers. As part of discussion in qiime2#129 and qiime2#130 it was found that these view types never actually worked and weren't being used anywhere. Even getting them to work raises another problem: file descriptor limit.

New view types can be added in the future that avoid the file descriptor limit.
@jairideout jairideout mentioned this issue Aug 16, 2017
Merged
thermokarst pushed a commit that referenced this issue Aug 17, 2017
@jairideout @thermokarst
BUG: remove PerSampleDNAIterators and PerSamplePairedDNAIterators (#141)
b03db2d 
Removes `q2_types.per_sample_sequences.PerSampleDNAIterators` and `q2_types.per_sample_sequences.PerSamplePairedDNAIterators` view types, along with their corresponding transformers. As part of discussion in #129 and #130 it was found that these view types never actually worked and weren't being used anywhere. Even getting them to work raises another problem: file descriptor limit.

New view types can be added in the future that avoid the file descriptor limit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jairideout jairideout

@maxvonhippel maxvonhippel

Labels
type:bug Something is wrong.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

BUG: Handle MANIFEST comments in per sample transformer jakereps/q2-types
3 participants
@jairideout @jakereps @maxvonhippel