lpantano / seqcluster

small RNA analysis from NGS data
http://seqcluster.readthedocs.io
MIT License
35 stars 17 forks source link

Helping transition to Python3 #34

Closed smoe closed 6 years ago

smoe commented 6 years ago

Hello,

This is the pull request accompanying https://github.com/lpantano/seqcluster/issues/32

nosetest3 pointed me to unichr, basestring to be unknown, and next does not exist.

I am somewhat uncertain about possible exceptions raised when readline fails, but this also holds for "next", I presume.

Many thanks and regards,

Steffen

smoe commented 6 years ago

Ouch, 2.7 tests say

INFO Done. Download https://github.com/lpantano/seqclusterViz/archive/master.zip to browse the output.
INFO It took 0.073 minutes
.Probably this will fail, you need bcbio-nextgen for many installation functions.
['seqbuster', '--sps', 'hsa', '--hairpin', '../../data/examples/miraligner/hairpin.fa', '--mirna', '../../data/examples/miraligner/miRNA.str', '--gtf', '../../data/examples/miraligner/hsa.gff3', '-o', 'test_out_mirs_fasta', '--miraligner', '../../data/examples/miraligner/sim_isomir.fa']
INFO Run seqbuster
INFO Reading ../../data/examples/miraligner/sim_isomir.fa
Traceback (most recent call last):
  File "/home/travis/install/bin/seqcluster", line 11, in <module>
    load_entry_point('seqcluster==1.2.4a8', 'console_scripts', 'seqcluster')()
  File "/home/travis/install/lib/python2.7/site-packages/seqcluster-1.2.4a8-py2.7.egg/seqcluster/command_line.py", line 40, in main
    miraligner(kwargs["args"])
  File "/home/travis/install/lib/python2.7/site-packages/seqcluster-1.2.4a8-py2.7.egg/seqcluster/seqbuster/__init__.py", line 502, in miraligner
    bam_fn = _filter_seqs(bam_fn)
  File "/home/travis/install/lib/python2.7/site-packages/seqcluster-1.2.4a8-py2.7.egg/seqcluster/seqbuster/__init__.py", line 60, in _filter_seqs
    seq = in_handle.readline().strip()
ValueError: Mixing iteration and read methods would lose data
E.INFO args(debug=True, print_debug=True, json='../../data/examples/seqcluster.json')
INFO Reading data
INFO Create databse
.EINFO Find UMI tags in read names, collapsing by UMI.
E

which I could not observe for Python 3. I'll come back with an update once my Python 2 environment is back in shape.

lpantano commented 6 years ago

Thanks for working on that! I'll wait for you then.

smoe commented 6 years ago

Hello, so, it seems like I am passing your tests. While the changes felt good, as in "if wrong then not immediately obvious that they are wrong", it felt like some are missing:

moeller@steffen-laptop-debian:~/git/debian-med/seqcluster-smoe/seqcluster$ grep -A 10 -r "for line in" | grep readline
seqcluster/function/rnafold.py:        for line in iter(process.stdout.readline, ''):
moeller@steffen-laptop-debian:~/git/debian-med/seqcluster-smoe/seqcluster$ grep -B 10 -r "for line in" | grep readline
seqcluster/libs/table.py-       header=f.readline().strip()
seqcluster/libs/inputs.py-        line = handle_in.readline().strip()
seqcluster/libs/inputs.py-        line = handle_in.readline().strip()
seqcluster/libs/do.py-        line = s.stdout.readline()
seqcluster/seqbuster/__init__.py-        in_handle.readline()
seqcluster/function/target.py-        in_handle.readline()
seqcluster/function/rnafold.py:        for line in iter(process.stdout.readline, ''):

I had a look at the above files, though, and (as the grep -B indicates), the use of the readline is before the for loop, which may just be fine.

So, curious to hear what you think of it. Maybe I could motivate the one or other extra test for that? I run into other issues with nosetest here that in part cover the problems that then travis found. That is the "missing directory one" I mentioned in https://github.com/lpantano/seqcluster/issues/32 but also something else indicating a problem with panda and python2 with my installation that seems independent from seqcluster.

lpantano commented 6 years ago

Thanks for working on this. I'll try to setup travis to run in both, python 3 and python 2.7 and try in a real data set. If that works, I think you got it right! :)

lpantano commented 6 years ago

Thanks, I merged to develop, and I'll try in real data next week. I am going on holidays, but I'll get this done when I am back.

Thanks!