Skip to content
This repository has been archived by the owner on Nov 9, 2023. It is now read-only.

Join paired ends updates #1726

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

mikerobeson
Copy link
Member

Moved function write_synced_barcodes_fastq and its test code to format.py and test_format.py respectively as per #1243.

Also:

  • Updated to skbio.io code.
  • removed test_join_paired_ends.py as all relevant test code is now in test_format.py

@ghost
Copy link

ghost commented Nov 17, 2014

Build results will soon be (or already are) available at: http://ci.qiime.org/job/qiime-github-pr/1256/

@ghost
Copy link

ghost commented Nov 17, 2014

Build results will soon be (or already are) available at: http://ci.qiime.org/job/qiime-github-pr/1258/

@gregcaporaso gregcaporaso self-assigned this Dec 3, 2014
help=' Input format of fastq data. Can be: \'illumina1.3\' or'+
' \'illumina1.8\'. Only used if using the \'-b\' option.' +
' Output will always be \'illumina1.8\'. [default: %default] '+
'For more info see: http://en.wikipedia.org/wiki/FASTQ_format',
Copy link
Contributor

Choose a reason for hiding this comment

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

@gregcaporaso
Copy link
Contributor

retest this please

…updated help documentation in join_paired_ends.py script
@mikerobeson
Copy link
Member Author

Done. I went through and updated broken code to relevant skbio code in:

  • test_format.py
  • format.py

The updated code in these two files pass all unit tests on my local machine.

I've also updated the link to the skbio fastq format information as requested in 'join_paired_ends.py'

@ghost
Copy link

ghost commented Dec 3, 2014

Build results will soon be (or already are) available at: http://ci.qiime.org/job/qiime-github-pr/1278/

@jairideout jairideout added this to the QIIME 1.9.0 milestone Dec 8, 2014
@@ -601,7 +604,8 @@ def write_Fasta_from_name_seq_pairs(name_seqs, fh):
raise ValueError("Need open file handle to write to.")

for (name, seq) in name_seqs:
fh.write("%s\n" % BiologicalSequence(seq, id=name).to_fasta())
#fh.write("%s\n" % BiologicalSequence(seq, id=name).to_fasta())
Copy link
Member

Choose a reason for hiding this comment

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

remove this comment

@ghost
Copy link

ghost commented Dec 18, 2014

Build results will soon be (or already are) available at: http://ci.qiime.org/job/qiime-github-pr/1437/

@jairideout
Copy link
Member

retest this please

@ghost
Copy link

ghost commented Dec 19, 2014

Build results will soon be (or already are) available at: http://ci.qiime.org/job/qiime-github-pr/1439/

@gregcaporaso
Copy link
Contributor

@mikerobeson, thanks for these edits. @jairideout and I are working on prepping 1.9.0 now, and decided that we're going to not merge this for now, and re-assign for the 2.0.0. Our motivation for this is that this would be the only code in QIIME 1.9.0 that uses the new skbio fastq parsers, and it's not a good idea for the code base to be using different parsers for the same file format due to possible inconsistencies. We'll integrate these changes when we port all of the fastq parsing in QIIME to use the new parsers.

Sorry we didn't notice the issue with porting to the new skbio parsers sooner. As always, we really appreciate your contributions to QIIME so thanks again!

@gregcaporaso gregcaporaso modified the milestones: QIIME 2.0.0, QIIME 1.9.0 Dec 22, 2014
@mikerobeson
Copy link
Member Author

Was I not supposed to use skbio? I used skbio because the test code warned me of deprecated code and specified that I use skbio. :-(

@gregcaporaso
Copy link
Contributor

Sorry for the confusion, we're just concerned about having two different parsers in different parts of the codebase because unfortunately we didn't get around to updating the rest of QIIME to use the new parsers. So it's great that it is updated for it, we're mostly just concerned about possible differences with the rest of QIIME (so it's really our fault, not yours, that we're concerned about merging this).

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.

4 participants