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

Add IQtree datatypes #4849

Merged
merged 12 commits into from Nov 8, 2017

Conversation

Projects
None yet
5 participants
@mtekman
Contributor

mtekman commented Oct 24, 2017

No description provided.

@galaxybot galaxybot added the triage label Oct 24, 2017

@galaxybot galaxybot added this to the 18.01 milestone Oct 24, 2017

>>> from galaxy.datatypes.sniff import get_test_fname
>>> fname = get_test_fname('example.mldist')
>>> SDF().sniff(fname)

This comment has been minimized.

@bgruening

bgruening Oct 26, 2017

Member

copy and paste mistake...

>>> fname = get_test_fname('example.mldist')
>>> SDF().sniff(fname)

This comment has been minimized.

@bgruening

bgruening Oct 26, 2017

Member

Why should the same file yield true in one case and false in an other?

@martenson

This comment has been minimized.

Member

martenson commented Oct 27, 2017

@galaxybot test this

@mtekman mtekman closed this Oct 27, 2017

@mtekman mtekman reopened this Oct 27, 2017

@mtekman

This comment has been minimized.

Contributor

mtekman commented Oct 29, 2017

merge soon please?

@martenson

This comment has been minimized.

Member

martenson commented Nov 1, 2017

@galaxybot test this

@martenson

This comment has been minimized.

Member

martenson commented Nov 1, 2017

@bgruening are your comments resolved to your liking?

@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 1, 2017

Can you move these to some place less specific - even if only to lib/galaxy/datatypes/text.py if none of the other files make sense.

@bgruening

This comment has been minimized.

Member

bgruening commented Nov 1, 2017

Yes, @mtekman addressed my comments. I'm just not sure the sniffer is stringent enough. I look to me like every nxn matrix will yield true. @mtekman meeting tomorrow?

@mtekman

This comment has been minimized.

Contributor

mtekman commented Nov 1, 2017

thanks for the reviews, and @bgruening of course

@martenson

This comment has been minimized.

Member

martenson commented Nov 2, 2017

@galaxybot test this

False
"""
with open(filename, 'r') as fio:
return fio.readline().split()[0] == "IQ-TREE"

This comment has been minimized.

@bgruening

bgruening Nov 3, 2017

Member

Can you use: fio.read(7). This will prevent reading files with a huge lines entirely into memory.

"""IQ-TREE format"""
file_ext = 'iqtree'
def __init__(self, **kwd):

This comment has been minimized.

@bgruening

bgruening Nov 3, 2017

Member

I don't think you need __init__ and init_meta at all ...

>>> fname = get_test_fname('example.iqtree')
>>> IQTree().sniff(fname)
True

This comment has been minimized.

@bgruening

bgruening Nov 3, 2017

Member

be more spare with line breaks here, one empty line is more then enough

@@ -0,0 +1,18 @@
17
LngfishAu 0.0000000 0.3462223 0.3360018 0.3702015 0.4944709 0.6022786 0.5805893 0.5922343 0.5877389 0.5273904 0.4820672 0.5280145 0.5141823 0.5375114 0.5505050 0.5532668 0.5205150

This comment has been minimized.

@bgruening

bgruening Nov 3, 2017

Member

This dataset is not used anymore.

@@ -744,6 +746,8 @@
<sniffer type="galaxy.datatypes.tabular:Sam"/>
<sniffer type="galaxy.datatypes.data:Newick"/>
<sniffer type="galaxy.datatypes.data:Nexus"/>
<sniffer type="galaxy.datatypes.data:MaximumLikelihoodDistanceMatrix"/>

This comment has been minimized.

@bgruening

bgruening Nov 3, 2017

Member

Why have you added this. You don't have a sniffer for your filetype mldist, right?

This comment has been minimized.

@mtekman

mtekman Nov 3, 2017

Contributor

sorry, fixed

@bgruening

@martenson looks good to me. But I guess we need to squash the commits.

@mtekman thanks a lot!

@martenson

This comment has been minimized.

Member

martenson commented Nov 4, 2017

@galaxybot test this

@mtekman

This comment has been minimized.

Contributor

mtekman commented Nov 5, 2017

hi thanks - @bgruening should I squash all from my side and reupload, or can you do this during the merge?

@martenson

This comment has been minimized.

Member

martenson commented Nov 6, 2017

@mtekman I can squash in merge, let me just make sure that @jmchilton is content.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 6, 2017

Yeah - can we redo this so that IQTree is not in data.py. I want that file to load fast someday. If there is no more specific place for this then text.py would be better IMO.

@mtekman

This comment has been minimized.

Contributor

mtekman commented Nov 6, 2017

will do, thanks

@mtekman

This comment has been minimized.

Contributor

mtekman commented Nov 7, 2017

request for merge @jmchilton

@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 7, 2017

You moved the datatype class but didn't update the sample datatype configuration so I don't think it will work.

@mtekman

This comment has been minimized.

Contributor

mtekman commented Nov 7, 2017

Sorry, good catch! I'm a little surprised it passed testing.

Fixed

@martenson

This comment has been minimized.

Member

martenson commented Nov 7, 2017

@galaxybot test this

@jmchilton jmchilton merged commit 3b4b97a into galaxyproject:dev Nov 8, 2017

7 checks passed

api test Build finished. 309 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 162 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 57 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
selenium test Build finished. 98 tests run, 1 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Member

jmchilton commented Nov 8, 2017

Cool - thanks for the contribution and sorry being so picky 😅.

@jmchilton jmchilton added status/review and removed status/WIP labels Nov 8, 2017

@mtekman mtekman deleted the mtekman:iqtree branch Nov 14, 2017

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